* RE: randconfig build error with next-20140909, in drivers/net/ethernet/broadcom/cnic.c
From: Anish Bhatt @ 2014-09-09 23:36 UTC (permalink / raw)
To: Michael Chan
Cc: Randy Dunlap, eddie.wai@broadcom.com, Jim Davis, Stephen Rothwell,
linux-next, linux-kernel, David S. Miller, netdev,
James Bottomley
In-Reply-To: <1410304835.7520.27.camel@LTIRV-MCHAN1.corp.ad.broadcom.com>
> -----Original Message-----
> From: Michael Chan [mailto:mchan@broadcom.com]
> Sent: Tuesday, September 9, 2014 4:21 PM
> To: Anish Bhatt
> Cc: Randy Dunlap; eddie.wai@broadcom.com; Jim Davis; Stephen Rothwell;
> linux-next; linux-kernel; David S. Miller; netdev; James Bottomley
> Subject: RE: randconfig build error with next-20140909, in
> drivers/net/ethernet/broadcom/cnic.c
>
> On Tue, 2014-09-09 at 23:16 +0000, Anish Bhatt wrote:
> > > It would be really good if SCSI_NETLINK depended on NET instead of
> selected NET.
> > > We shouldn't have kconfig symbols that use 'select' on entire
> subsystems.
> >
> > As a test, I was able to fix this by this approach : change
> > SCSI_NETLINK to depend on NET instead of selecting NET, and replacing
> > 'select NETDEVICES ETHERNET NET_VENDOR_BROADCOM CNIC' to
> 'depends on
> > CNIC' in bnx2i & bnx2fc. (as CNIC already has a dependency on those).
> > This removes the need to check for IPV6 in bnx2i/fc Kconfigs as well.
> >
> > The side effect is that you won't see BNX2 drivers unless CNIC is
> > selected, similar for SCSI_NETLINK as well, not sure if there are any other
> implications.
>
> Right. I think it is more user-friendly for BNX2I and BNX2FC to select CNIC.
> CNIC is not useful by itself and it makes more sense to select it automatically
> when BNX2I/BNX2FC are enabled.
>
Ok, it can work leaving 'select CNIC' as is, the key is changing the dependency
for SCSI_NETLINK from 'select NET' to 'depends on NET' and is basically what
fixes the issue with make. removing 'select NETDEVICES ETHERNET
NET_VENDOR_BROADCOM' from the bnx2i/fc modules is just cleanup.
bnx2i/bnx2fc need a check on IPV6 if we leave 'select CNIC' as is.
I have a patch for this ready, but I don't know too much about SCSI_NETLINK,
so maybe some else can chime in if this is a good idea ?
-Anish
^ permalink raw reply
* Re: linux-next: build failure after merge of the wireless-next tree
From: Stephen Rothwell @ 2014-09-09 23:28 UTC (permalink / raw)
To: David Miller, netdev
Cc: John W. Linville, linux-next, linux-kernel, Jade Bilkey,
Guenter Roeck, Geert Uytterhoeven, Mark Brown
In-Reply-To: <20140908184253.74ff5470@canb.auug.org.au>
[-- Attachment #1: Type: text/plain, Size: 2002 bytes --]
Hi Dave,
On Mon, 8 Sep 2014 18:42:53 +1000 Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>
> Hi John,
>
> After merging the wireless-next tree, today's linux-next build (powerpc
> allyesconfig) failed like this:
>
> drivers/net/wireless/ath/ath5k/debug.c: In function 'open_file_eeprom':
> drivers/net/wireless/ath/ath5k/debug.c:933:2: error: implicit declaration of function 'vmalloc' [-Werror=implicit-function-declaration]
> buf = vmalloc(eesize);
> ^
> drivers/net/wireless/ath/ath5k/debug.c:933:6: warning: assignment makes pointer from integer without a cast
> buf = vmalloc(eesize);
> ^
> drivers/net/wireless/ath/ath5k/debug.c:960:2: error: implicit declaration of function 'vfree' [-Werror=implicit-function-declaration]
> vfree(buf);
> ^
>
> Caused by commit db906eb2101b ("ath5k: added debugfs file for dumping
> eeprom"). Also reported by Guenter Roeck.
>
> I have used Geert Uytterhoeven's suggested fix of including vmalloc.h
> and so added this patch for today:
>
> From: Stephen Rothwell <sfr@canb.auug.org.au>
> Date: Mon, 8 Sep 2014 18:39:23 +1000
> Subject: [PATCH] ath5k: fix debugfs addition
>
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Suggested-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
> ---
> drivers/net/wireless/ath/ath5k/debug.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/wireless/ath/ath5k/debug.c b/drivers/net/wireless/ath/ath5k/debug.c
> index 30e4e1fd4b04..399728618fb9 100644
> --- a/drivers/net/wireless/ath/ath5k/debug.c
> +++ b/drivers/net/wireless/ath/ath5k/debug.c
> @@ -62,6 +62,7 @@
>
> #include <linux/export.h>
> #include <linux/moduleparam.h>
> +#include <linux/vmalloc.h>
>
> #include <linux/seq_file.h>
> #include <linux/list.h>
> --
> 2.1.0
This fix patch is now needed in the net-next tree ...
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* RE: randconfig build error with next-20140909, in drivers/net/ethernet/broadcom/cnic.c
From: Michael Chan @ 2014-09-09 23:20 UTC (permalink / raw)
To: Anish Bhatt
Cc: Randy Dunlap, eddie.wai@broadcom.com, Jim Davis, Stephen Rothwell,
linux-next, linux-kernel, David S. Miller, netdev,
James Bottomley
In-Reply-To: <525DB349B3FB5444AE057A887CB2A8D88E3E42@nice.asicdesigners.com>
On Tue, 2014-09-09 at 23:16 +0000, Anish Bhatt wrote:
> > It would be really good if SCSI_NETLINK depended on NET instead of selected NET.
> > We shouldn't have kconfig symbols that use 'select' on entire subsystems.
>
> As a test, I was able to fix this by this approach : change SCSI_NETLINK to depend
> on NET instead of selecting NET, and replacing 'select NETDEVICES ETHERNET
> NET_VENDOR_BROADCOM CNIC' to 'depends on CNIC' in bnx2i & bnx2fc. (as CNIC
> already has a dependency on those). This removes the need to check for IPV6 in
> bnx2i/fc Kconfigs as well.
>
> The side effect is that you won't see BNX2 drivers unless CNIC is selected, similar
> for SCSI_NETLINK as well, not sure if there are any other implications.
Right. I think it is more user-friendly for BNX2I and BNX2FC to select
CNIC. CNIC is not useful by itself and it makes more sense to select it
automatically when BNX2I/BNX2FC are enabled.
^ permalink raw reply
* Re: [PATCH net-next] sfc: Convert the normal transmit complete path to dev_consume_skb_any()
From: Eric Dumazet @ 2014-09-09 23:18 UTC (permalink / raw)
To: Rick Jones; +Cc: netdev, davem, sshah, linux-net-drivers
In-Reply-To: <20140909214327.D474F290042C@tardy>
On Tue, 2014-09-09 at 14:43 -0700, Rick Jones wrote:
> From: Rick Jones <rick.jones2@hp.com>
>
> Convert the normal transmit completion path from dev_kfree_skb_any()
> to dev_consume_skb_any() to help keep dropped packet profiling
> meaningful.
>
> Signed-off-by: Rick Jones <rick.jones2@hp.com>
>
> ---
>
> Compile tested only. Also a fixup to make scripts/checkpatch.pl
> happy.
>
> diff --git a/drivers/net/ethernet/sfc/tx.c b/drivers/net/ethernet/sfc/tx.c
> index 65c220f..3206098 100644
> --- a/drivers/net/ethernet/sfc/tx.c
> +++ b/drivers/net/ethernet/sfc/tx.c
> @@ -78,7 +78,7 @@ static void efx_dequeue_buffer(struct efx_tx_queue *tx_queue,
> if (buffer->flags & EFX_TX_BUF_SKB) {
> (*pkts_compl)++;
> (*bytes_compl) += buffer->skb->len;
> - dev_kfree_skb_any((struct sk_buff *) buffer->skb);
> + dev_consume_skb_any((struct sk_buff *)buffer->skb);
> netif_vdbg(tx_queue->efx, tx_done, tx_queue->efx->net_dev,
> "TX queue %d transmission id %x complete\n",
> tx_queue->queue, tx_queue->read_count);
Right, but please remove this ugly cast while you are at it ;)
^ permalink raw reply
* RE: randconfig build error with next-20140909, in drivers/net/ethernet/broadcom/cnic.c
From: Anish Bhatt @ 2014-09-09 23:16 UTC (permalink / raw)
To: Randy Dunlap, Michael Chan, eddie.wai@broadcom.com
Cc: Jim Davis, Stephen Rothwell, linux-next, linux-kernel,
David S. Miller, netdev, James Bottomley
In-Reply-To: <540F8582.50703@infradead.org>
> It would be really good if SCSI_NETLINK depended on NET instead of selected NET.
> We shouldn't have kconfig symbols that use 'select' on entire subsystems.
As a test, I was able to fix this by this approach : change SCSI_NETLINK to depend
on NET instead of selecting NET, and replacing 'select NETDEVICES ETHERNET
NET_VENDOR_BROADCOM CNIC' to 'depends on CNIC' in bnx2i & bnx2fc. (as CNIC
already has a dependency on those). This removes the need to check for IPV6 in
bnx2i/fc Kconfigs as well.
The side effect is that you won't see BNX2 drivers unless CNIC is selected, similar
for SCSI_NETLINK as well, not sure if there are any other implications.
-Anish
^ permalink raw reply
* [PATCH next] net: use kfree_skb_list() helper in more places
From: Florian Westphal @ 2014-09-09 23:08 UTC (permalink / raw)
To: netdev; +Cc: Florian Westphal
Signed-off-by: Florian Westphal <fw@strlen.de>
---
net/ipv6/ip6_output.c | 6 +-----
net/xfrm/xfrm_output.c | 6 +-----
2 files changed, 2 insertions(+), 10 deletions(-)
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index b7a3e7b..2e6a0db 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -701,11 +701,7 @@ int ip6_fragment(struct sk_buff *skb, int (*output)(struct sk_buff *))
return 0;
}
- while (frag) {
- skb = frag->next;
- kfree_skb(frag);
- frag = skb;
- }
+ kfree_skb_list(frag);
IP6_INC_STATS(net, ip6_dst_idev(&rt->dst),
IPSTATS_MIB_FRAGFAILS);
diff --git a/net/xfrm/xfrm_output.c b/net/xfrm/xfrm_output.c
index c51e8f7b..499d6c1 100644
--- a/net/xfrm/xfrm_output.c
+++ b/net/xfrm/xfrm_output.c
@@ -166,11 +166,7 @@ static int xfrm_output_gso(struct sk_buff *skb)
err = xfrm_output2(segs);
if (unlikely(err)) {
- while ((segs = nskb)) {
- nskb = segs->next;
- segs->next = NULL;
- kfree_skb(segs);
- }
+ kfree_skb_list(nskb);
return err;
}
--
1.8.1.5
^ permalink raw reply related
* Re: [RFC v2 3/6] kthread: warn on kill signal if not OOM
From: Tejun Heo @ 2014-09-09 23:03 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Lennart Poettering, Kay Sievers, Dmitry Torokhov,
Greg Kroah-Hartman, Wu Zhangjin, Takashi Iwai, Arjan van de Ven,
linux-kernel@vger.kernel.org, Oleg Nesterov, hare, Andrew Morton,
Tetsuo Handa, Joseph Salisbury, Benjamin Poirier,
Santosh Rastapur, One Thousand Gnomes, Tim Gardner,
Pierre Fersing, Nagalakshmi Nandigama, Praveen Krishnamoorthy,
Sreekanth
In-Reply-To: <20140909032529.GJ11706@mtj.dyndns.org>
On Tue, Sep 09, 2014 at 12:25:29PM +0900, Tejun Heo wrote:
> Hello,
>
> On Mon, Sep 08, 2014 at 08:19:12PM -0700, Luis R. Rodriguez wrote:
> > On the systemd side of things it should enable this sysctl and for
> > older kernels what should it do?
>
> Supposing the change is backported via -stable, it can try to set the
> sysctl on all kernels. If the knob doesn't exist, the fix is not
> there and nothing can be done about it.
The more I think about it, the more I think this should be a
per-insmod instance thing rather than a system-wide switch. Currently
the kernel param code doesn't allow a generic param outside the ones
specified by the module itself but adding support for something like
driver.async_load=1 shouldn't be too difficult, applying that to
existing systems shouldn't be much more difficult than a system-wide
switch, and it'd be siginificantly cleaner than fiddling with driver
blacklist.
Thanks.
--
tejun
^ permalink raw reply
* Re: [RFC v2 3/6] kthread: warn on kill signal if not OOM
From: Dmitry Torokhov @ 2014-09-09 23:01 UTC (permalink / raw)
To: James Bottomley
Cc: Tejun Heo, Luis R. Rodriguez, Lennart Poettering, Kay Sievers,
Greg Kroah-Hartman, Wu Zhangjin, Takashi Iwai, Arjan van de Ven,
linux-kernel@vger.kernel.org, Oleg Nesterov, hare, Andrew Morton,
Tetsuo Handa, Joseph Salisbury, Benjamin Poirier,
Santosh Rastapur, One Thousand Gnomes, Tim Gardner,
Pierre Fersing, Nagalakshmi Nandigama, Praveen Krishnamoorthy
In-Reply-To: <1410302783.13298.50.camel@jarvis.lan>
On Tuesday, September 09, 2014 03:46:23 PM James Bottomley wrote:
> On Wed, 2014-09-10 at 07:41 +0900, Tejun Heo wrote:
> >
> > The thing is that we have to have dynamic mechanism to listen for
> > device attachments no matter what and such mechanism has been in place
> > for a long time at this point. The synchronous wait simply doesn't
> > serve any purpose anymore and kinda gets in the way in that it makes
> > it a possibly extremely slow process to tell whether loading of a
> > module succeeded or not because the wait for the initial round of
> > probe is piggybacked.
>
> OK, so we just fire and forget in userland ... why bother inventing an
> elaborate new infrastructure in the kernel to do exactly what
>
> modprobe <mod> &
>
> would do?
Just so we do not forget: we also want the no-modules case to also be able
to probe asynchronously so that a slow device does not stall kernel booting.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: randconfig build error with next-20140909, in drivers/net/ethernet/broadcom/cnic.c
From: Randy Dunlap @ 2014-09-09 22:56 UTC (permalink / raw)
To: Anish Bhatt, Michael Chan, eddie.wai@broadcom.com
Cc: Jim Davis, Stephen Rothwell, linux-next, linux-kernel,
David S. Miller, netdev, James Bottomley
In-Reply-To: <525DB349B3FB5444AE057A887CB2A8D88E3CEC@nice.asicdesigners.com>
On 09/09/14 14:08, Anish Bhatt wrote:
>
>> Adding depends on IPV6 || IPV6=n doesn't work for SCSI_BNX2X_FCOE, but
>> works for SCSI_BNX2_ISCSI?
>
> It fixes the config issue for both, but with FCOE; make scripts will complain about recursive dependencies with the following :
>
> scripts/kconfig/conf --silentoldconfig Kconfig
> net/Kconfig:5:error: recursive dependency detected!
> net/Kconfig:5: symbol NET is selected by SCSI_NETLINK
> drivers/scsi/Kconfig:43: symbol SCSI_NETLINK is selected by SCSI_FC_ATTRS
> drivers/scsi/Kconfig:258: symbol SCSI_FC_ATTRS is selected by LIBFC
> drivers/scsi/Kconfig:586: symbol LIBFC is selected by SCSI_BNX2X_FCOE
> drivers/scsi/bnx2fc/Kconfig:1: symbol SCSI_BNX2X_FCOE depends on IPV6
> net/ipv6/Kconfig:6: symbol IPV6 depends on NET
It would be really good if SCSI_NETLINK depended on NET instead of selected NET.
We shouldn't have kconfig symbols that use 'select' on entire subsystems.
--
~Randy
^ permalink raw reply
* Re: [RFC v2 3/6] kthread: warn on kill signal if not OOM
From: Tejun Heo @ 2014-09-09 22:52 UTC (permalink / raw)
To: James Bottomley
Cc: Luis R. Rodriguez, Lennart Poettering, Kay Sievers,
Dmitry Torokhov, Greg Kroah-Hartman, Wu Zhangjin, Takashi Iwai,
Arjan van de Ven, linux-kernel@vger.kernel.org, Oleg Nesterov,
hare, Andrew Morton, Tetsuo Handa, Joseph Salisbury,
Benjamin Poirier, Santosh Rastapur, One Thousand Gnomes,
Tim Gardner, Pierre Fersing, Nagalakshmi Nandigama,
Praveen Krishnamoorthy <pravee
In-Reply-To: <1410302783.13298.50.camel@jarvis.lan>
Hello, James.
On Tue, Sep 09, 2014 at 03:46:23PM -0700, James Bottomley wrote:
> If you want the return of an individual device probe a log scraper gives
> it to you ... and nothing else does currently. The advantage of the
> prink in dd.c is that it's standard for everything and can be scanned
> for ... if you take that out, you'll get complaints about the lack of
> standard messages (you'd be surprised at the number of enterprise
> monitoring systems that actually do log scraping).
Why would a log scaper care about which task is printing the messages?
The printk can stay there. There's nothing wrong with it. Log
scapers tend to be asynchronous in nature but if a log scraper wants
to operate synchronously for whatever reason, it can simply not turn
on async probing.
> OK, so we just fire and forget in userland ... why bother inventing an
> elaborate new infrastructure in the kernel to do exactly what
>
> modprobe <mod> &
>
> would do?
I think the argument there is that the issuer wants to know whether
such operations succeeded or not and wants to report and record the
result and possibly take other actions in response. We're currently
mixing wait and error reporting for one type of operation with wait
for another. I'm not saying it's a fatal flaw or anything but it can
get in the way.
Thanks.
--
tejun
^ permalink raw reply
* Re: [RFC v2 3/6] kthread: warn on kill signal if not OOM
From: James Bottomley @ 2014-09-09 22:46 UTC (permalink / raw)
To: Tejun Heo
Cc: Luis R. Rodriguez, Lennart Poettering, Kay Sievers,
Dmitry Torokhov, Greg Kroah-Hartman, Wu Zhangjin, Takashi Iwai,
Arjan van de Ven, linux-kernel@vger.kernel.org, Oleg Nesterov,
hare, Andrew Morton, Tetsuo Handa, Joseph Salisbury,
Benjamin Poirier, Santosh Rastapur, One Thousand Gnomes,
Tim Gardner, Pierre Fersing, Nagalakshmi Nandigama,
Praveen Krishnamoorthy <pravee
In-Reply-To: <20140909224143.GB3154@mtj.dyndns.org>
On Wed, 2014-09-10 at 07:41 +0900, Tejun Heo wrote:
> Hello,
>
> On Tue, Sep 09, 2014 at 03:26:02PM -0700, James Bottomley wrote:
> > > We no longer report back error on probe failure on module load.
> >
> > Yes, we do; for every probe failure of a device on a driver we'll print
> > a warning (see drivers/base/dd.c). Now if someone is proposing we
> > should report this in a better fashion, that's probably a good idea, but
> > I must have missed that patch.
>
> We can do printks all the same from anywhere. There's nothing special
> about printing from the module loading thread. The only way to
> actually take advantage of the synchronisity would be propagating
> error return to the waiting issuer, which we used to do but no longer
> can.
If you want the return of an individual device probe a log scraper gives
it to you ... and nothing else does currently. The advantage of the
prink in dd.c is that it's standard for everything and can be scanned
for ... if you take that out, you'll get complaints about the lack of
standard messages (you'd be surprised at the number of enterprise
monitoring systems that actually do log scraping).
> > > It
> > > used to make sense to indicate error for module load on probe failure
> > > when the hardware was a lot simpler and drivers did their own device
> > > enumeration. With the current bus / device setup, it doesn't make any
> > > sense and driver core silently suppresses all probe failures. There's
> > > nothing the probing thread can monitor anymore.
> >
> > Except the length of time taken to probe. That seems to be what systemd
> > is interested in, hence this whole thread, right?
>
> No, systemd in this case isn't interested in the time taken to probe
> at all. It is expecting module load to just do that - load the
> module. Modern userlands, systemd or not, no longer depend on or make
> use of the wait.
So what's the problem? it can just fire and forget; that's what fork()
is for.
> > But that's nothing to do with sync or async. Nowadays we register a
> > driver, the driver may bind to multiple devices. If one of those
> > devices encounters an error during probe, we just report the fact in
> > dmesg and move on. The module_init thread currently returns when all
> > the probe routines for all enumerated devices have been called, so
> > module_init has no indication of any failures (because they might be
> > mixed with successes); successes are indicated as the device appears but
> > we have nothing other than the kernel log to indicate the failures. How
> > does moving to async probing alter this? It doesn't as far as I can
> > see, except that module_init returns earlier but now we no longer have
> > an indication of when the probe completes, so we have to add yet another
> > mechanism to tell us if we're interested in that. I really don't see
> > what this buys us.
>
> The thing is that we have to have dynamic mechanism to listen for
> device attachments no matter what and such mechanism has been in place
> for a long time at this point. The synchronous wait simply doesn't
> serve any purpose anymore and kinda gets in the way in that it makes
> it a possibly extremely slow process to tell whether loading of a
> module succeeded or not because the wait for the initial round of
> probe is piggybacked.
OK, so we just fire and forget in userland ... why bother inventing an
elaborate new infrastructure in the kernel to do exactly what
modprobe <mod> &
would do?
James
^ permalink raw reply
* Re: Macvlan WARNiNGS about duplicate sysfs filenames (Was [GIT] Networking)
From: Cong Wang @ 2014-09-09 22:43 UTC (permalink / raw)
To: Andres Freund, Alexander Y. Fomichev
Cc: David Miller, Linus Torvalds, Andrew Morton, netdev,
linux-kernel@vger.kernel.org
In-Reply-To: <20140908212514.GA2348@awork2.anarazel.de>
On Mon, Sep 8, 2014 at 2:25 PM, Andres Freund <andres@anarazel.de> wrote:
> Hi,
>
> (don't have netdev archived, thus answering here, sorry)
>
> On 2014-09-07 16:41:09 -0700, David Miller wrote:
>> Alexander Y. Fomichev (1):
>> net: prevent of emerging cross-namespace symlinks
>
Since you are quoting this change, are you saying it causes
the following kernel warning?
> I'm seeing WARNINGs like:
> [ 1005.269134] ------------[ cut here ]------------
> [ 1005.269148] WARNING: CPU: 6 PID: 4213 at fs/sysfs/dir.c:31 sysfs_warn_dup+0x64/0x80()
> [ 1005.269150] sysfs: cannot create duplicate filename '/devices/pci0000:00/0000:00:1c.4/0000:03:00.0/net/eth0/upper_mv-eth0'
Is there a network device named upper_mv-eth0 existed in your system
before you created macvlan?
^ permalink raw reply
* Re: [RFC v2 3/6] kthread: warn on kill signal if not OOM
From: Tejun Heo @ 2014-09-09 22:41 UTC (permalink / raw)
To: James Bottomley
Cc: Luis R. Rodriguez, Lennart Poettering, Kay Sievers,
Dmitry Torokhov, Greg Kroah-Hartman, Wu Zhangjin, Takashi Iwai,
Arjan van de Ven, linux-kernel@vger.kernel.org, Oleg Nesterov,
hare, Andrew Morton, Tetsuo Handa, Joseph Salisbury,
Benjamin Poirier, Santosh Rastapur, One Thousand Gnomes,
Tim Gardner, Pierre Fersing, Nagalakshmi Nandigama,
Praveen Krishnamoorthy <pravee
In-Reply-To: <1410301562.13298.35.camel@jarvis.lan>
Hello,
On Tue, Sep 09, 2014 at 03:26:02PM -0700, James Bottomley wrote:
> > We no longer report back error on probe failure on module load.
>
> Yes, we do; for every probe failure of a device on a driver we'll print
> a warning (see drivers/base/dd.c). Now if someone is proposing we
> should report this in a better fashion, that's probably a good idea, but
> I must have missed that patch.
We can do printks all the same from anywhere. There's nothing special
about printing from the module loading thread. The only way to
actually take advantage of the synchronisity would be propagating
error return to the waiting issuer, which we used to do but no longer
can.
> > It
> > used to make sense to indicate error for module load on probe failure
> > when the hardware was a lot simpler and drivers did their own device
> > enumeration. With the current bus / device setup, it doesn't make any
> > sense and driver core silently suppresses all probe failures. There's
> > nothing the probing thread can monitor anymore.
>
> Except the length of time taken to probe. That seems to be what systemd
> is interested in, hence this whole thread, right?
No, systemd in this case isn't interested in the time taken to probe
at all. It is expecting module load to just do that - load the
module. Modern userlands, systemd or not, no longer depend on or make
use of the wait.
> But that's nothing to do with sync or async. Nowadays we register a
> driver, the driver may bind to multiple devices. If one of those
> devices encounters an error during probe, we just report the fact in
> dmesg and move on. The module_init thread currently returns when all
> the probe routines for all enumerated devices have been called, so
> module_init has no indication of any failures (because they might be
> mixed with successes); successes are indicated as the device appears but
> we have nothing other than the kernel log to indicate the failures. How
> does moving to async probing alter this? It doesn't as far as I can
> see, except that module_init returns earlier but now we no longer have
> an indication of when the probe completes, so we have to add yet another
> mechanism to tell us if we're interested in that. I really don't see
> what this buys us.
The thing is that we have to have dynamic mechanism to listen for
device attachments no matter what and such mechanism has been in place
for a long time at this point. The synchronous wait simply doesn't
serve any purpose anymore and kinda gets in the way in that it makes
it a possibly extremely slow process to tell whether loading of a
module succeeded or not because the wait for the initial round of
probe is piggybacked.
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH net-next v1 2/2] bonding: Simplify the xmit function for modes that use xmit_hash
From: Mahesh Bandewar @ 2014-09-09 22:41 UTC (permalink / raw)
To: Nikolay Aleksandrov
Cc: Jay Vosburgh, Veaceslav Falico, Andy Gospodarek, David Miller,
netdev, Eric Dumazet, Maciej Zenczykowski
In-Reply-To: <CAF2d9jhXEMZF-W+wYns4NCiJ1W2LjDoNEmO2X-3791DmbYaDkg@mail.gmail.com>
On Sat, Sep 6, 2014 at 10:33 PM, Mahesh Bandewar <maheshb@google.com> wrote:
> On Sat, Sep 6, 2014 at 4:02 AM, Nikolay Aleksandrov <nikolay@redhat.com> wrote:
>> On 09/06/2014 08:35 AM, Mahesh Bandewar wrote:
>>> Earlier change to use usable slave array for TLB mode had an additional
>>> performance advantage. So extending the same logic to all other modes
>>> that use xmit-hash for slave selection (viz 802.3AD, and XOR modes).
>>> Also consolidating this with the earlier TLB change.
>>>
>>> The main idea is to build the usable slaves array in the control path
>>> and use that array for slave selection during xmit operation.
>>>
>>> Measured performance in a setup with a bond of 4x1G NICs with 200
>>> instances of netperf for the modes involved (3ad, xor, tlb)
>>> cmd: netperf -t TCP_RR -H <TargetHost> -l 60 -s 5
>>>
>>> Mode TPS-Before TPS-After
>>>
>>> 802.3ad : 468,694 493,101
>>> TLB (lb=0): 392,583 392,965
>>> XOR : 475,696 484,517
>>>
>>> Signed-off-by: Mahesh Bandewar <maheshb@google.com>
>>> ---
>>> v1:
>>> (a) If bond_update_slave_arr() fails to allocate memory, it will overwrite
>>> the slave that need to be removed.
>>> (b) Freeing of array will assign NULL (to handle bond->down to bond->up
>>> transition gracefully.
>>> (c) Change from pr_debug() to pr_err() if bond_update_slave_arr() returns
>>> failure.
>>> (d) XOR: bond_update_slave_arr() will consider mii-mon, arp-mon cases and
>>> will populate the array even if these parameters are not used.
>>> (e) 3AD: Should handle the ad_agg_selection_logic correctly.
>>>
>>> drivers/net/bonding/bond_3ad.c | 79 ++++-----------------
>>> drivers/net/bonding/bond_alb.c | 45 +-----------
>>> drivers/net/bonding/bond_alb.h | 8 ---
>>> drivers/net/bonding/bond_main.c | 150 ++++++++++++++++++++++++++++++++++++----
>>> drivers/net/bonding/bonding.h | 8 +++
>>> 5 files changed, 161 insertions(+), 129 deletions(-)
>>>
>> Hi Mahesh,
>> From my last posts I revisited the bond_3ad_state_machine_handler() case
>> and I think I was wrong that the machine state lock would protect you since
>> it's different for every port so if the machine handler runs for port X and
>> something else executes for port Y without bond->lock for writing - race.
>> As I said in my last post primary_reselect is an ideal case for that even
>> though it's not used in 3ad, it can be altered and to cause a reselect of
>> the active slave thus rebuilding the slave_arr. Of course this is
>> theoretical, but I'd prefer not to have such bugs in the first place.
>> More notes below.
>>
> I'm not going to claim that I understood all the locking scenarios in
> all modes but your attempt to simplify these locks is a step in right
> direction and hopefully we can do all these operations under RTNL.
>
>>> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
>>> index ee2c73a9de39..ba05c83d5d83 100644
>>> --- a/drivers/net/bonding/bond_3ad.c
>>> +++ b/drivers/net/bonding/bond_3ad.c
>>> @@ -1579,6 +1579,8 @@ static void ad_agg_selection_logic(struct aggregator *agg)
>>> __disable_port(port);
>>> }
>>> }
>>> + if (bond_update_slave_arr(bond, NULL))
>>> + pr_err("Failed to build slave-array for 3ad mode.\n");
>>> }
>>>
>>> /* if the selected aggregator is of join individuals
>>> @@ -1717,6 +1719,8 @@ static void ad_enable_collecting_distributing(struct port *port)
>>> port->actor_port_number,
>>> port->aggregator->aggregator_identifier);
>>> __enable_port(port);
>>> + if (bond_update_slave_arr(port->slave->bond, NULL))
>>> + pr_err("Failed to build slave-array for 3ad mode.\n");
>>> }
>>> }
>>>
>>> @@ -1733,6 +1737,8 @@ static void ad_disable_collecting_distributing(struct port *port)
>>> port->actor_port_number,
>>> port->aggregator->aggregator_identifier);
>>> __disable_port(port);
>>> + if (bond_update_slave_arr(port->slave->bond, NULL))
>>> + pr_err("Failed to build slave-array for 3ad mode.\n");
>>> }
>>> }
>>>
>>> @@ -1917,6 +1923,9 @@ void bond_3ad_unbind_slave(struct slave *slave)
>>> __update_lacpdu_from_port(port);
>>> ad_lacpdu_send(port);
>>>
>>> + if (bond_update_slave_arr(bond, slave))
>>> + pr_err("Failed to build slave-array for 3AD mode.\n");
>>> +
>>> /* check if this aggregator is occupied */
>>> if (aggregator->lag_ports) {
>>> /* check if there are other ports related to this aggregator
>>> @@ -2311,6 +2320,9 @@ void bond_3ad_handle_link_change(struct slave *slave, char link)
>>> */
>>> port->sm_vars |= AD_PORT_BEGIN;
>>>
>>> + if (bond_update_slave_arr(slave->bond, NULL))
>>> + pr_err("Failed to build slave-array for 3ad mode.\n");
>>> +
>>> __release_state_machine_lock(port);
>>> }
>>>
>>> @@ -2407,73 +2419,6 @@ int bond_3ad_get_active_agg_info(struct bonding *bond, struct ad_info *ad_info)
>>> return ret;
>>> }
>>>
>>> -int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev)
>>> -{
>>> - struct bonding *bond = netdev_priv(dev);
>>> - struct slave *slave, *first_ok_slave;
>>> - struct aggregator *agg;
>>> - struct ad_info ad_info;
>>> - struct list_head *iter;
>>> - int slaves_in_agg;
>>> - int slave_agg_no;
>>> - int agg_id;
>>> -
>>> - if (__bond_3ad_get_active_agg_info(bond, &ad_info)) {
>>> - netdev_dbg(dev, "__bond_3ad_get_active_agg_info failed\n");
>>> - goto err_free;
>>> - }
>>> -
>>> - slaves_in_agg = ad_info.ports;
>>> - agg_id = ad_info.aggregator_id;
>>> -
>>> - if (slaves_in_agg == 0) {
>>> - netdev_dbg(dev, "active aggregator is empty\n");
>>> - goto err_free;
>>> - }
>>> -
>>> - slave_agg_no = bond_xmit_hash(bond, skb) % slaves_in_agg;
>>> - first_ok_slave = NULL;
>>> -
>>> - bond_for_each_slave_rcu(bond, slave, iter) {
>>> - agg = SLAVE_AD_INFO(slave)->port.aggregator;
>>> - if (!agg || agg->aggregator_identifier != agg_id)
>>> - continue;
>>> -
>>> - if (slave_agg_no >= 0) {
>>> - if (!first_ok_slave && bond_slave_can_tx(slave))
>>> - first_ok_slave = slave;
>>> - slave_agg_no--;
>>> - continue;
>>> - }
>>> -
>>> - if (bond_slave_can_tx(slave)) {
>>> - bond_dev_queue_xmit(bond, skb, slave->dev);
>>> - goto out;
>>> - }
>>> - }
>>> -
>>> - if (slave_agg_no >= 0) {
>>> - netdev_err(dev, "Couldn't find a slave to tx on for aggregator ID %d\n",
>>> - agg_id);
>>> - goto err_free;
>>> - }
>>> -
>>> - /* we couldn't find any suitable slave after the agg_no, so use the
>>> - * first suitable found, if found.
>>> - */
>>> - if (first_ok_slave)
>>> - bond_dev_queue_xmit(bond, skb, first_ok_slave->dev);
>>> - else
>>> - goto err_free;
>>> -
>>> -out:
>>> - return NETDEV_TX_OK;
>>> -err_free:
>>> - /* no suitable interface, frame not sent */
>>> - dev_kfree_skb_any(skb);
>>> - goto out;
>>> -}
>>> -
>>> int bond_3ad_lacpdu_recv(const struct sk_buff *skb, struct bonding *bond,
>>> struct slave *slave)
>>> {
>>> diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
>>> index 73c21e233131..334d92127baf 100644
>>> --- a/drivers/net/bonding/bond_alb.c
>>> +++ b/drivers/net/bonding/bond_alb.c
>>> @@ -200,7 +200,6 @@ static int tlb_initialize(struct bonding *bond)
>>> static void tlb_deinitialize(struct bonding *bond)
>>> {
>>> struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
>>> - struct tlb_up_slave *arr;
>>>
>>> _lock_tx_hashtbl_bh(bond);
>>>
>>> @@ -208,10 +207,6 @@ static void tlb_deinitialize(struct bonding *bond)
>>> bond_info->tx_hashtbl = NULL;
>>>
>>> _unlock_tx_hashtbl_bh(bond);
>>> -
>>> - arr = rtnl_dereference(bond_info->slave_arr);
>>> - if (arr)
>>> - kfree_rcu(arr, rcu);
>>> }
>>>
>>> static long long compute_gap(struct slave *slave)
>>> @@ -1409,39 +1404,9 @@ out:
>>> return NETDEV_TX_OK;
>>> }
>>>
>>> -static int bond_tlb_update_slave_arr(struct bonding *bond,
>>> - struct slave *skipslave)
>>> -{
>>> - struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
>>> - struct slave *tx_slave;
>>> - struct list_head *iter;
>>> - struct tlb_up_slave *new_arr, *old_arr;
>>> -
>>> - new_arr = kzalloc(offsetof(struct tlb_up_slave, arr[bond->slave_cnt]),
>>> - GFP_ATOMIC);
>>> - if (!new_arr)
>>> - return -ENOMEM;
>>> -
>>> - bond_for_each_slave(bond, tx_slave, iter) {
>>> - if (!bond_slave_can_tx(tx_slave))
>>> - continue;
>>> - if (skipslave == tx_slave)
>>> - continue;
>>> - new_arr->arr[new_arr->count++] = tx_slave;
>>> - }
>>> -
>>> - old_arr = rtnl_dereference(bond_info->slave_arr);
>>> - rcu_assign_pointer(bond_info->slave_arr, new_arr);
>>> - if (old_arr)
>>> - kfree_rcu(old_arr, rcu);
>>> -
>>> - return 0;
>>> -}
>>> -
>>> int bond_tlb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
>>> {
>>> struct bonding *bond = netdev_priv(bond_dev);
>>> - struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
>>> struct ethhdr *eth_data;
>>> struct slave *tx_slave = NULL;
>>> u32 hash_index;
>>> @@ -1462,9 +1427,9 @@ int bond_tlb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
>>> hash_index & 0xFF,
>>> skb->len);
>>> } else {
>>> - struct tlb_up_slave *slaves;
>>> + struct bond_up_slave *slaves;
>>>
>>> - slaves = rcu_dereference(bond_info->slave_arr);
>>> + slaves = rcu_dereference(bond->slave_arr);
>>> if (slaves && slaves->count)
>>> tx_slave = slaves->arr[hash_index %
>>> slaves->count];
>>> @@ -1733,10 +1698,6 @@ void bond_alb_deinit_slave(struct bonding *bond, struct slave *slave)
>>> rlb_clear_slave(bond, slave);
>>> }
>>>
>>> - if (bond_is_nondyn_tlb(bond))
>>> - if (bond_tlb_update_slave_arr(bond, slave))
>>> - pr_err("Failed to build slave-array for TLB mode.\n");
>>> -
>>> }
>>>
>>> /* Caller must hold bond lock for read */
>>> @@ -1762,7 +1723,7 @@ void bond_alb_handle_link_change(struct bonding *bond, struct slave *slave, char
>>> }
>>>
>>> if (bond_is_nondyn_tlb(bond)) {
>>> - if (bond_tlb_update_slave_arr(bond, NULL))
>>> + if (bond_update_slave_arr(bond, NULL))
>>> pr_err("Failed to build slave-array for TLB mode.\n");
>>> }
>>> }
>>> diff --git a/drivers/net/bonding/bond_alb.h b/drivers/net/bonding/bond_alb.h
>>> index aaeac61d03cf..5fc76c01636c 100644
>>> --- a/drivers/net/bonding/bond_alb.h
>>> +++ b/drivers/net/bonding/bond_alb.h
>>> @@ -139,20 +139,12 @@ struct tlb_slave_info {
>>> */
>>> };
>>>
>>> -struct tlb_up_slave {
>>> - unsigned int count;
>>> - struct rcu_head rcu;
>>> - struct slave *arr[0];
>>> -};
>>> -
>>> struct alb_bond_info {
>>> struct tlb_client_info *tx_hashtbl; /* Dynamically allocated */
>>> spinlock_t tx_hashtbl_lock;
>>> u32 unbalanced_load;
>>> int tx_rebalance_counter;
>>> int lp_counter;
>>> - /* -------- non-dynamic tlb mode only ---------*/
>>> - struct tlb_up_slave __rcu *slave_arr; /* Up slaves */
>>> /* -------- rlb parameters -------- */
>>> int rlb_enabled;
>>> struct rlb_client_info *rx_hashtbl; /* Receive hash table */
>>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>>> index f0f5eab0fab1..43f066539dab 100644
>>> --- a/drivers/net/bonding/bond_main.c
>>> +++ b/drivers/net/bonding/bond_main.c
>>> @@ -1413,6 +1413,10 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
>>> dev_mc_add(slave_dev, lacpdu_multicast);
>>> }
>>>
>>> + if (BOND_MODE(bond) == BOND_MODE_XOR &&
>>> + bond_update_slave_arr(bond, NULL))
>>> + pr_err("Failed to build slave-array for XOR mode.\n");
>>> +
>> ^^^^^^^^^^^^^^
>> 2 issues here:
>> 1. a little bit after this you can find the following switch:
>> switch (BOND_MODE(bond)) {
>>
>> that is meant for specific mode handling, I don't think you need to add
>> additional "if" here.
>>
>> 2. Why do you rebuild here, bond_master_upper_dev_link() hasn't been called
>> yet so the new slave isn't visible yet.
>>
> You are right! It's useless here and I'll remove it.
>
>>> res = vlan_vids_add_by_dev(slave_dev, bond_dev);
>>> if (res) {
>>> netdev_err(bond_dev, "Couldn't add bond vlan ids to %s\n",
>>> @@ -1692,6 +1696,11 @@ static int __bond_release_one(struct net_device *bond_dev,
>>> /* Inform AD package of unbinding of slave. */
>>> if (BOND_MODE(bond) == BOND_MODE_8023AD)
>>> bond_3ad_unbind_slave(slave);
>>> + else if (BOND_MODE(bond) == BOND_MODE_XOR ||
>>> + bond_is_nondyn_tlb(bond)) {
>>> + if (bond_update_slave_arr(bond, slave))
>>> + pr_err("Failed to build slave-array.\n");
>>> + }
>> Documentation/CodingStyle:
>> both branches must use braces.
>>
> Will do.
>>>
>>> write_unlock_bh(&bond->lock);
>>>
>>> @@ -2009,6 +2018,10 @@ static void bond_miimon_commit(struct bonding *bond)
>>> bond_alb_handle_link_change(bond, slave,
>>> BOND_LINK_UP);
>>>
>>> + if (BOND_MODE(bond) == BOND_MODE_XOR &&
>>> + bond_update_slave_arr(bond, NULL))
>>> + pr_err("Failed to build slave-array for XOR mode.\n");
>>> +
>>> if (!bond->curr_active_slave ||
>>> (slave == bond->primary_slave))
>>> goto do_failover;
>>> @@ -2037,6 +2050,10 @@ static void bond_miimon_commit(struct bonding *bond)
>>> bond_alb_handle_link_change(bond, slave,
>>> BOND_LINK_DOWN);
>>>
>>> + if (BOND_MODE(bond) == BOND_MODE_XOR &&
>>> + bond_update_slave_arr(bond, NULL))
>>> + pr_err("Failed to build slave-array for XOR mode.\n");
>>> +
>>> if (slave == rcu_access_pointer(bond->curr_active_slave))
>>> goto do_failover;
>>>
>>> @@ -2500,6 +2517,9 @@ static void bond_loadbalance_arp_mon(struct work_struct *work)
>>>
>>> if (slave_state_changed) {
>>> bond_slave_state_change(bond);
>>> + if (BOND_MODE(bond) == BOND_MODE_XOR &&
>>> + bond_update_slave_arr(bond, NULL))
>>> + pr_err("Failed to build slave-array for XOR mode.\n");
>>> } else if (do_failover) {
>>> /* the bond_select_active_slave must hold RTNL
>>> * and curr_slave_lock for write.
>>> @@ -2893,11 +2913,14 @@ static int bond_slave_netdev_event(unsigned long event,
>>> if (old_duplex != slave->duplex)
>>> bond_3ad_adapter_duplex_changed(slave);
>>> }
>>> + if (BOND_MODE(bond) == BOND_MODE_XOR &&
>>> + bond_update_slave_arr(bond, NULL))
>>> + pr_err("Failed to build slave-array for XOR mode.\n");
>>> break;
>>> case NETDEV_DOWN:
>>> - /*
>>> - * ... Or is it this?
>>> - */
>>> + if (BOND_MODE(bond) == BOND_MODE_XOR &&
>>> + bond_update_slave_arr(bond, NULL))
>>> + pr_err("Failed to build slave-array for XOR mode.\n");
>> ^^^^^^^^^^^^^^^^
>> In the case of a netdev event (up/down) does this only affect XOR mode ?
>> You could be right, just wanted to make sure we're not missing something :-)
>>
> There is no mode specific stuff for XOR mode and link events do not
> trigger anything for this mode. So the array stays stale and thats
> bad. The situation is different if the miimon or arpmon is used. But
> if someone tries to use this mode without arp/mii-mon, then these
> slave device event will have to be used to update the usable
> slave-array. I think 3ad handles it correctly, but now thinking about
> it, I need to check how TLB mode handles it.
>
OK TLB does not handle it either. With miimon=0, there is nothing that
would trigger the update. Actually it wasn't an issue earlier but with
this new mode (TLB with tlb_dynamic_lb = 0) I introduced this issue.
Will fix it in the next update.
>
>>> break;
>>> case NETDEV_CHANGEMTU:
>>> /*
>>> @@ -3143,12 +3166,17 @@ static int bond_open(struct net_device *bond_dev)
>>> bond_3ad_initiate_agg_selection(bond, 1);
>>> }
>>>
>>> + if (BOND_MODE(bond) == BOND_MODE_XOR &&
>>> + bond_update_slave_arr(bond, NULL))
>>> + pr_err("Failed to build slave-array for XOR mode.\n");
>>> +
>>> return 0;
>>> }
>>>
>>> static int bond_close(struct net_device *bond_dev)
>>> {
>>> struct bonding *bond = netdev_priv(bond_dev);
>>> + struct bond_up_slave *arr;
>>>
>>> bond_work_cancel_all(bond);
>>> bond->send_peer_notif = 0;
>>> @@ -3156,6 +3184,12 @@ static int bond_close(struct net_device *bond_dev)
>>> bond_alb_deinitialize(bond);
>>> bond->recv_probe = NULL;
>>>
>>> + arr = rtnl_dereference(bond->slave_arr);
>>> + if (arr) {
>>> + kfree_rcu(arr, rcu);
>>> + RCU_INIT_POINTER(bond->slave_arr, NULL);
>>> + }
>>> +
>> ^^^^^^^^
>> Why do this in the first place ? I mean I could easily release a slave
>> while the bond is down and rebuild the slave_arr.
>>
> If you do bond down the slave array is free-ed here, but next time
> when the bond up operation is performed, the slave array will be
> rebuilt. In that code, the logic always dereferences the earlier array
> and since it's non-NULL, this might end-up in double-free situation.
> So to avoid that I'm assigning NULL after the free.
>
>> One more issue that I just saw is that you might be leaking memory as
>> ndo_uninit() is called for a device after dev_close_many() so you'll free
>> the array here, but bond_uninit() calls __bond_release_slave and will
>> rebuild it.
>>
> Shouldn't __bond_release_slave() be called before closing the bond()?
> I'll have to check the code, but if you are right, then this is not
> the correct place for this free operation and probably the better
> place would be the bond_ununit() in that case.
>
>>> return 0;
>>> }
>>>
>>> @@ -3684,15 +3718,108 @@ static int bond_xmit_activebackup(struct sk_buff *skb, struct net_device *bond_d
>>> return NETDEV_TX_OK;
>>> }
>>>
>>> -/* In bond_xmit_xor() , we determine the output device by using a pre-
>>> - * determined xmit_hash_policy(), If the selected device is not enabled,
>>> - * find the next active slave.
>>> +/* Build the usable slaves array in control path for modes that use xmit-hash
>>> + * to determine the slave interface -
>>> + * (a) BOND_MODE_8023AD
>>> + * (b) BOND_MODE_XOR
>>> + * (c) BOND_MODE_TLB && tlb_dynamic_lb == 0
>>> */
>>> -static int bond_xmit_xor(struct sk_buff *skb, struct net_device *bond_dev)
>>> +int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave)
>>> {
>>> - struct bonding *bond = netdev_priv(bond_dev);
>>> + struct slave *slave;
>>> + struct list_head *iter;
>>> + struct bond_up_slave *new_arr, *old_arr;
>>> + int slaves_in_agg;
>>> + int agg_id = 0;
>>> + int ret = 0;
>>> +
>>> + new_arr = kzalloc(offsetof(struct bond_up_slave, arr[bond->slave_cnt]),
>>> + GFP_ATOMIC);
>>> + if (!new_arr) {
>>> + ret = -ENOMEM;
>>> + goto out;
>>> + }
>>> + if (BOND_MODE(bond) == BOND_MODE_8023AD) {
>>> + struct ad_info ad_info;
>>>
>>> - bond_xmit_slave_id(bond, skb, bond_xmit_hash(bond, skb) % bond->slave_cnt);
>>> + if (bond_3ad_get_active_agg_info(bond, &ad_info)) {
>>> + pr_debug("bond_3ad_get_active_agg_info failed\n");
>>> + kfree_rcu(new_arr, rcu);
>>> + ret = -EINVAL;
>>> + goto out;
>>> + }
>>> + slaves_in_agg = ad_info.ports;
>>> + agg_id = ad_info.aggregator_id;
>>> + }
>>> + bond_for_each_slave(bond, slave, iter) {
>>> + if (BOND_MODE(bond) == BOND_MODE_8023AD) {
>>> + struct aggregator *agg;
>>> +
>>> + agg = SLAVE_AD_INFO(slave)->port.aggregator;
>>> + if (!agg || agg->aggregator_identifier != agg_id)
>>> + continue;
>>> + }
>>> + if (!bond_slave_can_tx(slave))
>>> + continue;
>>> + if (skipslave == slave)
>>> + continue;
>>> + new_arr->arr[new_arr->count++] = slave;
>>> + }
>>> +
>>> + old_arr = rcu_dereference_protected(bond->slave_arr,
>>> + lockdep_rtnl_is_held() ||
>>> + lockdep_is_held(&bond->lock) ||
>>> + lockdep_is_held(&bond->curr_slave_lock));
>>> + rcu_assign_pointer(bond->slave_arr, new_arr);
>>> + if (old_arr)
>>> + kfree_rcu(old_arr, rcu);
>>> +
>>> +out:
>>> + if (ret != 0 && skipslave) {
>>> + int idx;
>>> +
>>> + /* Rare situation where caller has asked to skip a specific
>>> + * slave but allocation failed (most likely!). In this sitation
>>> + * overwrite the skipslave entry in the array with the last
>>> + * entry from the array to avoid a situation where the xmit
>>> + * path may choose this to-be-skipped slave to send a packet
>>> + * out.
>>> + */
>>> + rcu_read_lock();
>> ^^^^^^^^^^^^^^
>> RCU ?
>>
> Shouldn't the array manipulation (the overwrite operation) be
> performed with rcu-lock? May be I'm wrong!
>
>>> + old_arr = rcu_dereference_protected(bond->slave_arr,
>>> + lockdep_is_held(&bond->lock));
>> ^^^^^^^^
>> Only bond->lock ? This doesn't make any sense.
>>
> The only possibility here is from the __bond_release_one() because of
> the skipslave and that path uses bond->lock.
>
>>> + for (idx = 0; idx < old_arr->count; idx++) {
>>> + if (skipslave == old_arr->arr[idx]) {
>>> + if (idx != old_arr->count - 1)
>> You can drop the "if" and remove one level of indentation, if idx == count
>> - 1, then it'll overwrite itself (i.e. nothing) but count will still go down.
>> But I think there's a potential bigger problem here as in the case of
>> failure count might drop down to 0 but some transmitter might be pass the
>> check and at the modulus part and if count is re-fetched we might end up
>> with a div by zero.
>>
> __bond_release_one() uses write_lock_bh(). Isn't that sufficient to
> prevent a potential xmitter from getting into that mode?
>
>
>>> + old_arr->arr[idx] =
>>> + old_arr->arr[old_arr->count-1];
>>> + old_arr->count--;
>>> + break;
>>> + }
>>> + }
>>> + rcu_read_unlock();
>>> + }
>>> + return ret;
>>> +}
>>> +
>>> +/* Use this Xmit function for 3AD as well as XOR modes. The current
>>> + * usable slave array is formed in the control path. The xmit function
>>> + * just calculates hash and sends the packet out.
>>> + */
>>> +int bond_3ad_xor_xmit(struct sk_buff *skb, struct net_device *dev)
>>> +{
>>> + struct bonding *bond = netdev_priv(dev);
>>> + struct slave *slave;
>>> + struct bond_up_slave *slaves;
>>> +
>>> + slaves = rcu_dereference(bond->slave_arr);
>>> + if (slaves && slaves->count) {
>>> + slave = slaves->arr[bond_xmit_hash(bond, skb) % slaves->count];
>>> + bond_dev_queue_xmit(bond, skb, slave->dev);
>>> + } else {
>>> + dev_kfree_skb_any(skb);
>>> + atomic_long_inc(&dev->tx_dropped);
>>> + }
>>>
>>> return NETDEV_TX_OK;
>>> }
>>> @@ -3794,12 +3921,11 @@ static netdev_tx_t __bond_start_xmit(struct sk_buff *skb, struct net_device *dev
>>> return bond_xmit_roundrobin(skb, dev);
>>> case BOND_MODE_ACTIVEBACKUP:
>>> return bond_xmit_activebackup(skb, dev);
>>> + case BOND_MODE_8023AD:
>>> case BOND_MODE_XOR:
>>> - return bond_xmit_xor(skb, dev);
>>> + return bond_3ad_xor_xmit(skb, dev);
>>> case BOND_MODE_BROADCAST:
>>> return bond_xmit_broadcast(skb, dev);
>>> - case BOND_MODE_8023AD:
>>> - return bond_3ad_xmit_xor(skb, dev);
>>> case BOND_MODE_ALB:
>>> return bond_alb_xmit(skb, dev);
>>> case BOND_MODE_TLB:
>>> diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
>>> index aace510d08d1..4a6195c0de60 100644
>>> --- a/drivers/net/bonding/bonding.h
>>> +++ b/drivers/net/bonding/bonding.h
>>> @@ -177,6 +177,12 @@ struct slave {
>>> struct kobject kobj;
>>> };
>>>
>>> +struct bond_up_slave {
>>> + unsigned int count;
>>> + struct rcu_head rcu;
>>> + struct slave *arr[0];
>>> +};
>>> +
>>> /*
>>> * Link pseudo-state only used internally by monitors
>>> */
>>> @@ -196,6 +202,7 @@ struct bonding {
>>> struct slave __rcu *curr_active_slave;
>>> struct slave __rcu *current_arp_slave;
>>> struct slave *primary_slave;
>>> + struct bond_up_slave __rcu *slave_arr; /* Array of usable slaves */
>>> bool force_primary;
>>> s32 slave_cnt; /* never change this value outside the attach/detach wrappers */
>>> int (*recv_probe)(const struct sk_buff *, struct bonding *,
>>> @@ -527,6 +534,7 @@ const char *bond_slave_link_status(s8 link);
>>> struct bond_vlan_tag *bond_verify_device_path(struct net_device *start_dev,
>>> struct net_device *end_dev,
>>> int level);
>>> +int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave);
>>>
>>> #ifdef CONFIG_PROC_FS
>>> void bond_create_proc_entry(struct bonding *bond);
>>>
>>
^ permalink raw reply
* Re: [RFC v2 3/6] kthread: warn on kill signal if not OOM
From: James Bottomley @ 2014-09-09 22:26 UTC (permalink / raw)
To: Tejun Heo
Cc: Luis R. Rodriguez, Lennart Poettering, Kay Sievers,
Dmitry Torokhov, Greg Kroah-Hartman, Wu Zhangjin, Takashi Iwai,
Arjan van de Ven, linux-kernel@vger.kernel.org, Oleg Nesterov,
hare, Andrew Morton, Tetsuo Handa, Joseph Salisbury,
Benjamin Poirier, Santosh Rastapur, One Thousand Gnomes,
Tim Gardner, Pierre Fersing, Nagalakshmi Nandigama,
Praveen Krishnamoorthy <pravee
In-Reply-To: <20140909214240.GA3154@mtj.dyndns.org>
On Wed, 2014-09-10 at 06:42 +0900, Tejun Heo wrote:
> Hey, James.
>
> On Tue, Sep 09, 2014 at 12:35:46PM -0700, James Bottomley wrote:
> > I don't have very strong views on this one. However, I've got to say
> > from a systems point of view that if the desire is to flag when the
> > module is having problems, probing and initializing synchronously in a
> > thread spawned by init which the init process can watchdog and thus can
> > flash up warning messages seems to be more straightforwards than an
> > elaborate asynchronous mechanism with completion signalling which
> > achieves the same thing in a more complicated (and thus bug prone)
> > fashion.
>
> We no longer report back error on probe failure on module load.
Yes, we do; for every probe failure of a device on a driver we'll print
a warning (see drivers/base/dd.c). Now if someone is proposing we
should report this in a better fashion, that's probably a good idea, but
I must have missed that patch.
> It
> used to make sense to indicate error for module load on probe failure
> when the hardware was a lot simpler and drivers did their own device
> enumeration. With the current bus / device setup, it doesn't make any
> sense and driver core silently suppresses all probe failures. There's
> nothing the probing thread can monitor anymore.
Except the length of time taken to probe. That seems to be what systemd
is interested in, hence this whole thread, right?
> In that sense, we already separated out device probing from module
> loading simply because the hardware reality mandated it and we have
> dynamic mechanisms to listen for device probes exactly for the same
> reason, so I think it makes sense to separate out the waiting too, at
> least in the long term. In a modern dynamic setup, the waits are
> essentially arbitrary and doesn't buy us anything.
But that's nothing to do with sync or async. Nowadays we register a
driver, the driver may bind to multiple devices. If one of those
devices encounters an error during probe, we just report the fact in
dmesg and move on. The module_init thread currently returns when all
the probe routines for all enumerated devices have been called, so
module_init has no indication of any failures (because they might be
mixed with successes); successes are indicated as the device appears but
we have nothing other than the kernel log to indicate the failures. How
does moving to async probing alter this? It doesn't as far as I can
see, except that module_init returns earlier but now we no longer have
an indication of when the probe completes, so we have to add yet another
mechanism to tell us if we're interested in that. I really don't see
what this buys us.
James
^ permalink raw reply
* Re: [PATCH 1/1] net:socket: set msg_namelen to 0 if msg_name is passed as NULL in msghdr struct from userland.
From: Ani Sinha @ 2014-09-09 22:14 UTC (permalink / raw)
To: Ani Sinha, fruggeri, David Miller, netdev, fenner, travisb,
Hannes Frederic Sowa, matthew.leach, Eric Dumazet
In-Reply-To: <1410212999-11013-1-git-send-email-ani@arista.com>
Any feedback on this patch? Any concerns?
On Mon, Sep 8, 2014 at 2:49 PM, Ani Sinha <ani@arista.com> wrote:
> Linux manpage for recvmsg and sendmsg calls does not explicitly mention setting msg_namelen to 0 when
> msg_name passed set as NULL. When developers don't set msg_namelen member in msghdr, it might contain garbage
> value which will fail the validation check and sendmsg and recvmsg calls from kernel will return EINVAL. This will
> break old binaries and any code for which there is no access to source code.
> To fix this, we set msg_namelen to 0 when msg_name is passed as NULL from userland.
>
> Signed-off-by: Ani Sinha <ani@arista.com>
> ---
> net/socket.c | 3 +++
> 1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/net/socket.c b/net/socket.c
> index 95ee7d8..457be6a 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -1997,6 +1997,9 @@ static int copy_msghdr_from_user(struct msghdr *kmsg,
> if (copy_from_user(kmsg, umsg, sizeof(struct msghdr)))
> return -EFAULT;
>
> + if (kmsg->msg_name == NULL)
> + kmsg->msg_namelen = 0;
> +
> if (kmsg->msg_namelen < 0)
> return -EINVAL;
>
> --
> 1.7.4.4
>
^ permalink raw reply
* Re: iwlwifi no authentication with AP - Re: pull request: wireless-next 2014-09-08
From: Vadim Kochan @ 2014-09-09 22:07 UTC (permalink / raw)
To: Oliver Hartkopp
Cc: Emmanuel Grumbach, John W. Linville, linux-wireless,
netdev@vger.kernel.org
In-Reply-To: <540F61D0.9000704@hartkopp.net>
I have the same issue with iwlwifi.
On Tue, Sep 9, 2014 at 11:23 PM, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> On 09.09.2014 22:02, Emmanuel Grumbach wrote:
>> On Tue, Sep 9, 2014 at 10:46 PM, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
>>> Hello John, all,
>>>
>>> on my i7 Laptop with iwlwifi the latest net-next does not connect to my access point:
>>
>> Are you sure this wasn't the case before?
>
> Indeed I moved to net-next right today - as I also had the LED lockdep splat
> on 3.17-rcX with X < 4.
>
> So it can be that this issue was already introduced at the beginning of this
> net-next cycle. Don't know, sorry. The latest 3.17-rc4 from Linus' tree works
> fine. I'm running a Debian unstable here (in both cases).
>
>> Can you bisect?
>
> Not really. I'm running the kernel on my bare machine - and I just wanted to
> go to bed right now ...
>
> So I will be able to test patches tomorrow morning again.
>
> Tnx for your fast feedback,
> Oliver
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [RFC v2 3/6] kthread: warn on kill signal if not OOM
From: Jiri Kosina @ 2014-09-09 22:00 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: James Bottomley, Tejun Heo, Lennart Poettering, Kay Sievers,
Dmitry Torokhov, Greg Kroah-Hartman, Wu Zhangjin, Takashi Iwai,
Arjan van de Ven, linux-kernel@vger.kernel.org, Oleg Nesterov,
hare, Andrew Morton, Tetsuo Handa, Joseph Salisbury,
Benjamin Poirier, Santosh Rastapur, One Thousand Gnomes,
Tim Gardner, Pierre Fersing, Nagalakshmi Nandigama
In-Reply-To: <CAB=NE6WGAMvffGDTkN5x5Cd+YOtD-Cmo3v7_8FmJ5cvCRbFEpw@mail.gmail.com>
On Tue, 9 Sep 2014, Luis R. Rodriguez wrote:
> By design it seems systemd should not allow worker processes to block
> indefinitely and in fact it currently uses the same timeout for all
> types of worker processes.
And I whole-heartedly believe this is something that fundamentally needs
to be addressed in systemd, not in the kernel.
This aproach is actually introducing a user-visible regressions. Look, for
example, exec() never times out. Therefore if your system is on its knees,
heavily overloaded (or completely broken), you are likely to be able to
`reboot' it, because exec("/sbin/reboot") ultimately succeeds.
But with all the timeouts, dbus, "Failed to issue method call: Did
not receive a reply" messages, this is getting close to impossible.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: Question about synchronize_net() in AF_PACKET close()
From: David Miller @ 2014-09-09 22:00 UTC (permalink / raw)
To: martin; +Cc: netdev, paulmck, stephen
In-Reply-To: <CAN8CM3zo3bMxZV_UvywCPNjJrMwMA2-+POfY8cR429iodHRc+A@mail.gmail.com>
From: Martin Kelly <martin@martingkelly.com>
Date: Tue, 9 Sep 2014 14:44:56 -0700
> In net/packet/af_packet.c, I noticed the following few lines within
> packet_release:
>
> synchronize_net();
> /*
> * Now the socket is dead. No more input will appear.
> */
> sock_orphan(sk);
> sock->sk = NULL;
>
> From testing and code analysis, I have found that it appears to be safe
> to move sock_orphan and sock->sk = NULL before the synchronize_net()
> call like so:
>
> /*
> * Now the socket is dead. No more input will appear.
> */
> sock_orphan(sk);
> sock->sk = NULL;
> synchronize_net();
>
> Could some RCU and/or networking experts chime in about whether this a
> safe operation? For all I know, there is some deep, fundamental reason
> why those lines are in the order they are. On the other hand, perhaps
> there is not.
The synchronize_net() is also there to protect against the prot hook
which can run asynchronously from the core packet input path on any
cpu.
You probably want to reference commit:
commit 808f5114a9206fee855117d416440e1071ab375c
Author: stephen hemminger <shemminger@vyatta.com>
Date: Mon Feb 22 07:57:18 2010 +0000
packet: convert socket list to RCU (v3)
which put the synchronize_net() there in the first place.
^ permalink raw reply
* Question about synchronize_net() in AF_PACKET close()
From: Martin Kelly @ 2014-09-09 21:44 UTC (permalink / raw)
To: netdev; +Cc: Paul E. McKenney, Stephen Hemminger
Hi,
I have been reading the code in net/packet/af_packet.c in order to
optimize the runtime for raw socket close(). In certain cases, close()
can take a long time to return because of the synchronize_net()
overhead. In pursuit of speeding up close(), I have been testing a patch
that defers the socket buffer memory release via call_rcu() in order to
make close() return faster. However, I hit a tricky RCU question for
which I don't have the answer and wanted to know if any RCU/networking
experts could provide some guidance.
In net/packet/af_packet.c, I noticed the following few lines within
packet_release:
synchronize_net();
/*
* Now the socket is dead. No more input will appear.
*/
sock_orphan(sk);
sock->sk = NULL;
>From testing and code analysis, I have found that it appears to be safe
to move sock_orphan and sock->sk = NULL before the synchronize_net()
call like so:
/*
* Now the socket is dead. No more input will appear.
*/
sock_orphan(sk);
sock->sk = NULL;
synchronize_net();
Could some RCU and/or networking experts chime in about whether this a
safe operation? For all I know, there is some deep, fundamental reason
why those lines are in the order they are. On the other hand, perhaps
there is not.
Thanks,
Martin
^ permalink raw reply
* [PATCH net-next] sfc: Convert the normal transmit complete path to dev_consume_skb_any()
From: Rick Jones @ 2014-09-09 21:43 UTC (permalink / raw)
To: netdev; +Cc: davem, sshah, linux-net-drivers
From: Rick Jones <rick.jones2@hp.com>
Convert the normal transmit completion path from dev_kfree_skb_any()
to dev_consume_skb_any() to help keep dropped packet profiling
meaningful.
Signed-off-by: Rick Jones <rick.jones2@hp.com>
---
Compile tested only. Also a fixup to make scripts/checkpatch.pl
happy.
diff --git a/drivers/net/ethernet/sfc/tx.c b/drivers/net/ethernet/sfc/tx.c
index 65c220f..3206098 100644
--- a/drivers/net/ethernet/sfc/tx.c
+++ b/drivers/net/ethernet/sfc/tx.c
@@ -78,7 +78,7 @@ static void efx_dequeue_buffer(struct efx_tx_queue *tx_queue,
if (buffer->flags & EFX_TX_BUF_SKB) {
(*pkts_compl)++;
(*bytes_compl) += buffer->skb->len;
- dev_kfree_skb_any((struct sk_buff *) buffer->skb);
+ dev_consume_skb_any((struct sk_buff *)buffer->skb);
netif_vdbg(tx_queue->efx, tx_done, tx_queue->efx->net_dev,
"TX queue %d transmission id %x complete\n",
tx_queue->queue, tx_queue->read_count);
^ permalink raw reply related
* Re: [RFC v2 3/6] kthread: warn on kill signal if not OOM
From: Tejun Heo @ 2014-09-09 21:42 UTC (permalink / raw)
To: James Bottomley
Cc: Luis R. Rodriguez, Lennart Poettering, Kay Sievers,
Dmitry Torokhov, Greg Kroah-Hartman, Wu Zhangjin, Takashi Iwai,
Arjan van de Ven, linux-kernel@vger.kernel.org, Oleg Nesterov,
hare, Andrew Morton, Tetsuo Handa, Joseph Salisbury,
Benjamin Poirier, Santosh Rastapur, One Thousand Gnomes,
Tim Gardner, Pierre Fersing, Nagalakshmi Nandigama,
Praveen Krishnamoorthy <pravee
In-Reply-To: <1410291346.13298.16.camel@jarvis.lan>
Hey, James.
On Tue, Sep 09, 2014 at 12:35:46PM -0700, James Bottomley wrote:
> I don't have very strong views on this one. However, I've got to say
> from a systems point of view that if the desire is to flag when the
> module is having problems, probing and initializing synchronously in a
> thread spawned by init which the init process can watchdog and thus can
> flash up warning messages seems to be more straightforwards than an
> elaborate asynchronous mechanism with completion signalling which
> achieves the same thing in a more complicated (and thus bug prone)
> fashion.
We no longer report back error on probe failure on module load. It
used to make sense to indicate error for module load on probe failure
when the hardware was a lot simpler and drivers did their own device
enumeration. With the current bus / device setup, it doesn't make any
sense and driver core silently suppresses all probe failures. There's
nothing the probing thread can monitor anymore.
In that sense, we already separated out device probing from module
loading simply because the hardware reality mandated it and we have
dynamic mechanisms to listen for device probes exactly for the same
reason, so I think it makes sense to separate out the waiting too, at
least in the long term. In a modern dynamic setup, the waits are
essentially arbitrary and doesn't buy us anything.
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH net-next 0/6] bonding: get rid of bond->lock
From: David Miller @ 2014-09-09 21:41 UTC (permalink / raw)
To: nikolay; +Cc: netdev, vfalico, j.vosburgh, andy
In-Reply-To: <540F59B2.5080406@redhat.com>
From: Nikolay Aleksandrov <nikolay@redhat.com>
Date: Tue, 09 Sep 2014 21:49:06 +0200
> On 09/09/2014 08:57 PM, David Miller wrote:
>> drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c: In function ‘cxgb4_inet6addr_handler’:
>> drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c:4393:3: error: ‘struct bonding’ has no member named ‘lock’
>> drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c:4407:3: error: ‘struct bonding’ has no member named ‘lock’
>>
>> Please test build with allmodconfig.
>>
> Wow, my bad! Sorry about that, I didn't expect it.
I have to say I didn't expect this either :)
^ permalink raw reply
* [PATCH net-next v2 7/7] bonding: remove last users of bond->lock and bond->lock itself
From: Nikolay Aleksandrov @ 2014-09-09 21:17 UTC (permalink / raw)
To: netdev; +Cc: hariprasad, vfalico, j.vosburgh, andy, davem, Nikolay Aleksandrov
In-Reply-To: <1410297423-26624-1-git-send-email-nikolay@redhat.com>
The usage of bond->lock in bond_main.c was completely unnecessary as it
didn't help to sync with anything, most of the spots already had RTNL.
Since there're no more users of bond->lock, remove it.
Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
---
drivers/net/bonding/bond_main.c | 39 ---------------------------------------
drivers/net/bonding/bonding.h | 8 ++------
2 files changed, 2 insertions(+), 45 deletions(-)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 629037f79213..b43b2df9e5d1 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -3105,7 +3105,6 @@ static int bond_open(struct net_device *bond_dev)
struct slave *slave;
/* reset slave->backup and slave->inactive */
- read_lock(&bond->lock);
if (bond_has_slaves(bond)) {
read_lock(&bond->curr_slave_lock);
bond_for_each_slave(bond, slave, iter) {
@@ -3120,7 +3119,6 @@ static int bond_open(struct net_device *bond_dev)
}
read_unlock(&bond->curr_slave_lock);
}
- read_unlock(&bond->lock);
bond_work_init_all(bond);
@@ -3175,7 +3173,6 @@ static struct rtnl_link_stats64 *bond_get_stats(struct net_device *bond_dev,
memset(stats, 0, sizeof(*stats));
- read_lock_bh(&bond->lock);
bond_for_each_slave(bond, slave, iter) {
const struct rtnl_link_stats64 *sstats =
dev_get_stats(slave->dev, &temp);
@@ -3206,7 +3203,6 @@ static struct rtnl_link_stats64 *bond_get_stats(struct net_device *bond_dev,
stats->tx_heartbeat_errors += sstats->tx_heartbeat_errors;
stats->tx_window_errors += sstats->tx_window_errors;
}
- read_unlock_bh(&bond->lock);
return stats;
}
@@ -3246,13 +3242,11 @@ static int bond_do_ioctl(struct net_device *bond_dev, struct ifreq *ifr, int cmd
if (mii->reg_num == 1) {
mii->val_out = 0;
- read_lock(&bond->lock);
read_lock(&bond->curr_slave_lock);
if (netif_carrier_ok(bond->dev))
mii->val_out = BMSR_LSTATUS;
read_unlock(&bond->curr_slave_lock);
- read_unlock(&bond->lock);
}
return 0;
@@ -3428,21 +3422,6 @@ static int bond_change_mtu(struct net_device *bond_dev, int new_mtu)
netdev_dbg(bond_dev, "bond=%p, new_mtu=%d\n", bond, new_mtu);
- /* Can't hold bond->lock with bh disabled here since
- * some base drivers panic. On the other hand we can't
- * hold bond->lock without bh disabled because we'll
- * deadlock. The only solution is to rely on the fact
- * that we're under rtnl_lock here, and the slaves
- * list won't change. This doesn't solve the problem
- * of setting the slave's MTU while it is
- * transmitting, but the assumption is that the base
- * driver can handle that.
- *
- * TODO: figure out a way to safely iterate the slaves
- * list, but without holding a lock around the actual
- * call to the base driver.
- */
-
bond_for_each_slave(bond, slave, iter) {
netdev_dbg(bond_dev, "s %p c_m %p\n",
slave, slave->dev->netdev_ops->ndo_change_mtu);
@@ -3517,21 +3496,6 @@ static int bond_set_mac_address(struct net_device *bond_dev, void *addr)
if (!is_valid_ether_addr(sa->sa_data))
return -EADDRNOTAVAIL;
- /* Can't hold bond->lock with bh disabled here since
- * some base drivers panic. On the other hand we can't
- * hold bond->lock without bh disabled because we'll
- * deadlock. The only solution is to rely on the fact
- * that we're under rtnl_lock here, and the slaves
- * list won't change. This doesn't solve the problem
- * of setting the slave's hw address while it is
- * transmitting, but the assumption is that the base
- * driver can handle that.
- *
- * TODO: figure out a way to safely iterate the slaves
- * list, but without holding a lock around the actual
- * call to the base driver.
- */
-
bond_for_each_slave(bond, slave, iter) {
netdev_dbg(bond_dev, "slave %p %s\n", slave, slave->dev->name);
res = dev_set_mac_address(slave->dev, addr);
@@ -3857,7 +3821,6 @@ static int bond_ethtool_get_settings(struct net_device *bond_dev,
* the true receive or transmit bandwidth (not all modes are symmetric)
* this is an accurate maximum.
*/
- read_lock(&bond->lock);
bond_for_each_slave(bond, slave, iter) {
if (bond_slave_can_tx(slave)) {
if (slave->speed != SPEED_UNKNOWN)
@@ -3868,7 +3831,6 @@ static int bond_ethtool_get_settings(struct net_device *bond_dev,
}
}
ethtool_cmd_speed_set(ecmd, speed ? : SPEED_UNKNOWN);
- read_unlock(&bond->lock);
return 0;
}
@@ -3931,7 +3893,6 @@ void bond_setup(struct net_device *bond_dev)
struct bonding *bond = netdev_priv(bond_dev);
/* initialize rwlocks */
- rwlock_init(&bond->lock);
rwlock_init(&bond->curr_slave_lock);
bond->params = bonding_defaults;
diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
index c798561a6f01..78c461abaa09 100644
--- a/drivers/net/bonding/bonding.h
+++ b/drivers/net/bonding/bonding.h
@@ -83,7 +83,7 @@
* @pos: current slave
* @iter: list_head * iterator
*
- * Caller must hold bond->lock
+ * Caller must hold RTNL
*/
#define bond_for_each_slave(bond, pos, iter) \
netdev_for_each_lower_private((bond)->dev, pos, iter)
@@ -185,11 +185,8 @@ struct slave {
/*
* Here are the locking policies for the two bonding locks:
*
- * 1) Get bond->lock when reading/writing slave list.
+ * 1) Get rcu_read_lock when reading or RTNL when writing slave list.
* 2) Get bond->curr_slave_lock when reading/writing bond->curr_active_slave.
- * (It is unnecessary when the write-lock is put with bond->lock.)
- * 3) When we lock with bond->curr_slave_lock, we must lock with bond->lock
- * beforehand.
*/
struct bonding {
struct net_device *dev; /* first - useful for panic debug */
@@ -200,7 +197,6 @@ struct bonding {
s32 slave_cnt; /* never change this value outside the attach/detach wrappers */
int (*recv_probe)(const struct sk_buff *, struct bonding *,
struct slave *);
- rwlock_t lock;
rwlock_t curr_slave_lock;
u8 send_peer_notif;
u8 igmp_retrans;
--
1.9.3
^ permalink raw reply related
* [PATCH net-next v2 6/7] bonding: options: remove bond->lock usage
From: Nikolay Aleksandrov @ 2014-09-09 21:17 UTC (permalink / raw)
To: netdev; +Cc: hariprasad, vfalico, j.vosburgh, andy, davem, Nikolay Aleksandrov
In-Reply-To: <1410297423-26624-1-git-send-email-nikolay@redhat.com>
We're safe to remove the bond->lock use from the arp targets because
arp_rcv_probe no longer acquires bond->lock, only rcu_read_lock.
Also setting the primary slave is safe because noone uses the bond->lock
as a syncing mechanism for that anymore.
Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
---
drivers/net/bonding/bond_options.c | 19 +------------------
1 file changed, 1 insertion(+), 18 deletions(-)
diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
index 7c9e176baecc..534c0600484e 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -955,14 +955,7 @@ static int _bond_option_arp_ip_target_add(struct bonding *bond, __be32 target)
static int bond_option_arp_ip_target_add(struct bonding *bond, __be32 target)
{
- int ret;
-
- /* not to race with bond_arp_rcv */
- write_lock_bh(&bond->lock);
- ret = _bond_option_arp_ip_target_add(bond, target);
- write_unlock_bh(&bond->lock);
-
- return ret;
+ return _bond_option_arp_ip_target_add(bond, target);
}
static int bond_option_arp_ip_target_rem(struct bonding *bond, __be32 target)
@@ -991,9 +984,6 @@ static int bond_option_arp_ip_target_rem(struct bonding *bond, __be32 target)
netdev_info(bond->dev, "Removing ARP target %pI4\n", &target);
- /* not to race with bond_arp_rcv */
- write_lock_bh(&bond->lock);
-
bond_for_each_slave(bond, slave, iter) {
targets_rx = slave->target_last_arp_rx;
for (i = ind; (i < BOND_MAX_ARP_TARGETS-1) && targets[i+1]; i++)
@@ -1004,8 +994,6 @@ static int bond_option_arp_ip_target_rem(struct bonding *bond, __be32 target)
targets[i] = targets[i+1];
targets[i] = 0;
- write_unlock_bh(&bond->lock);
-
return 0;
}
@@ -1013,11 +1001,8 @@ void bond_option_arp_ip_targets_clear(struct bonding *bond)
{
int i;
- /* not to race with bond_arp_rcv */
- write_lock_bh(&bond->lock);
for (i = 0; i < BOND_MAX_ARP_TARGETS; i++)
_bond_options_arp_ip_target_set(bond, i, 0, 0);
- write_unlock_bh(&bond->lock);
}
static int bond_option_arp_ip_targets_set(struct bonding *bond,
@@ -1081,7 +1066,6 @@ static int bond_option_primary_set(struct bonding *bond,
struct slave *slave;
block_netpoll_tx();
- read_lock(&bond->lock);
write_lock_bh(&bond->curr_slave_lock);
p = strchr(primary, '\n');
@@ -1120,7 +1104,6 @@ static int bond_option_primary_set(struct bonding *bond,
out:
write_unlock_bh(&bond->curr_slave_lock);
- read_unlock(&bond->lock);
unblock_netpoll_tx();
return 0;
--
1.9.3
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox