From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nikolay Aleksandrov Subject: Re: [PATCH net-next 1/5] bonding: convert to list API and replace bond's custom list Date: Wed, 31 Jul 2013 21:00:35 +0200 Message-ID: <51F95ED3.3020104@redhat.com> References: <1375283553-32070-1-git-send-email-nikolay@redhat.com> <1375283553-32070-2-git-send-email-nikolay@redhat.com> <20130731113717.32f48d71@nehalam.linuxnetplumber.net> <51F95B21.5020004@redhat.com> <20130731115657.4262a7fe@nehalam.linuxnetplumber.net> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: netdev@vger.kernel.org, andy@greyhouse.net, davem@davemloft.net, fubar@us.ibm.com To: Stephen Hemminger Return-path: Received: from mx1.redhat.com ([209.132.183.28]:65071 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754297Ab3GaTAm (ORCPT ); Wed, 31 Jul 2013 15:00:42 -0400 In-Reply-To: <20130731115657.4262a7fe@nehalam.linuxnetplumber.net> Sender: netdev-owner@vger.kernel.org List-ID: On 07/31/2013 08:56 PM, Stephen Hemminger wrote: > On Wed, 31 Jul 2013 20:44:49 +0200 > Nikolay Aleksandrov wrote: > >> On 07/31/2013 08:37 PM, Stephen Hemminger wrote: >>> On Wed, 31 Jul 2013 17:12:29 +0200 >>> Nikolay Aleksandrov wrote: >>> >>>> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c >>>> index 390061d..80e288c 100644 >>>> --- a/drivers/net/bonding/bond_3ad.c >>>> +++ b/drivers/net/bonding/bond_3ad.c >>>> @@ -143,10 +143,13 @@ static inline struct bonding *__get_bond_by_port(struct port *port) >>>> */ >>>> static inline struct port *__get_first_port(struct bonding *bond) >>>> { >>>> + struct slave *first_slave; >>>> + >>>> if (bond->slave_cnt == 0) >>>> return NULL; >>>> + first_slave = bond_first_slave(bond); >>> >>> As Jay said, it would be be better to have bond_first_slave return >>> NULL (if no slaves), and eliminate slave_cnt. It would also fix >>> a race here between slave_cnt and all slave's being removed. >>> >> Hi Stephen, >> First off - thank you for the review. >> >> What do you mean by eliminate slave_cnt ? >> We need it for various calculations throughout the bonding. >> There's no race here because read_lock(&bond->lock) is held every time this >> is called and slave_cnt can change only under write_lock of the same lock, >> the 3ad code is not yet converted to RCU. >> >> Nik > > I would hope the goal would be to eliminate all read_lock's and allow > it to be totally RCU based. > > You could then reduce slave_cnt to being only accessed by under a spin_lock > when doing management actions. > Yes, that is the end goal. And my end-implementation does just that - removes curr_slave_lock completely and makes bond->lock a spinlock. But for now we need it for the parts that aren't converted, as I said in the beginning I'm doing the conversion gradually and there will be a special series that takes care of 3ad mode as there are many potential problems for RCU there because of its current design.