From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nikolay Aleksandrov Subject: Re: [PATCH net v2] bonding: fix div by zero while enslaving and transmitting Date: Wed, 17 Sep 2014 13:08:19 +0200 Message-ID: <54196BA3.4020505@redhat.com> References: <5413097B.9080802@redhat.com> <1410536298-8022-1-git-send-email-nikolay@redhat.com> <541926EB.4010809@huawei.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Cc: Eric Dumazet , Andy Gospodarek , Jay Vosburgh , Veaceslav Falico To: Ding Tianhong , netdev@vger.kernel.org Return-path: Received: from mx1.redhat.com ([209.132.183.28]:38262 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754993AbaIQLJA (ORCPT ); Wed, 17 Sep 2014 07:09:00 -0400 In-Reply-To: <541926EB.4010809@huawei.com> Sender: netdev-owner@vger.kernel.org List-ID: On 17/09/14 08:15, Ding Tianhong wrote: > On 2014/9/12 23:38, Nikolay Aleksandrov wrote: >> The problem is that the slave is first linked and slave_cnt is >> incremented afterwards leading to a div by zero in the modes that use it >> as a modulus. What happens is that in bond_start_xmit() >> bond_has_slaves() is used to evaluate further transmission and it becomes >> true after the slave is linked in, but when slave_cnt is used in the xmit >> path it is still 0, so fetch it once and transmit based on that. Since >> it is used only in round-robin and XOR modes, the fix is only for them. >> Thanks to Eric Dumazet for pointing out the fault in my first try to fix >> this. >> > > Hi, I think no need to add more checks in the xmit fast path, why not add a barrier to make > sure the slave_cnt inc to 1 before access it. > > + /* Increment slave_cnt before linking in the slave so we won't end up in > + * bond_start_xmit with bond_has_slaves() true and slave_cnt == 0. > + */ > + bond->slave_cnt++; > + wmb(); > > I think it looks more efficiency, sorry for reply so late. > > Regards > Ding > > Hi Ding, You should re-read Eric's comment to my first fix. In my first attempt I moved the increment before the slave linking which does rcu_assign_pointer() which implies a full memory barrier, IIRC. The issue is that this fixes the writer side and makes sure the increment is visible before linking the slave, but I missed that on the reader side (bond_start_xmit()) we don't have any barriers, so the CPU is free to do whatever it likes with the access to slave_cnt F.e. it can fetch it before the slave list. Now, this fix shouldn't be felt much performance-wise since the likely() hint will be correct 99% of the time because the situation where slave_cnt is not in sync is only in a very short period of time while enslaving and releasing slaves. If you'd like to further remove this one check - you could. You can fetch slave_cnt only once in bond_start_xmit() and use that as a check for further transmitting instead of empty slave list but you must pass down the fetched value to the xmitting functions, that is you should not re-fetch it, so it'd probably require you to add additional parameter to all modes' xmit functions so you can pass it down from bond_start_xmit(). Since only 2 modes actually use slave_cnt I don't think that is necessary. In any case net should be merged with net-next first. Cheers, Nik