* Re: [PATCH net-next v2 2/2] Re-check for a VIO_DESC_READY data descriptor after short udelay()
From: Sowmini Varadhan @ 2014-09-06 21:02 UTC (permalink / raw)
To: David Miller; +Cc: david.stevens, netdev
In-Reply-To: <20140905134758.GB1256@oracle.com>
On (09/05/14 09:47), Sowmini Varadhan wrote:
> > The memory barrier exists in order to make sure the cookies et al. are
> > globally visible before the VIO_DESC_READY. We don't want stores to
> > be reordered such that the VIO_DESC_READY is seen too early.
>
> Ok, though David (dls) was just pointing out that a rmb() might
> be missing in vnet_walk_rx_one() before checking for READY descriptor
Stared at this a bit over the last two days, checked
the documentation, discussed with dls offline - looks like
(a) the rmb() thing was mostly a red-herring/fud
(b) we do need the wmb()
The wmb() part is working correctly as designed:
The producer will do
/* code to set up cookies */
wmb(); /* makes sure above changes are committed */
d->hdr.state = VIO_DESC_READY;
the consumer will do
if (desc->hdr.state != VIO_DESC_READY)
return 1;
err = vnet_rx_one(port, desc->size, desc->cookies, desc->ncookies);
:
desc->hdr.state = VIO_DESC_DONE;
So the vnet_rx_one() will only use valid cookie information at
all times.
This allows the code to correctly able to read multiple READY descriptors
for a single LDC trigger, which it already does today.
(and it would be needlessly inefficient to clamp this down to
only one descriptor read per LDC-start in the vnet_rx())
So what (if any) is the outstanding question about wmb() at this
point?
--Sowmini
^ permalink raw reply
* Re: [net-next 0/2][pull request] Intel Wired LAN Driver Updates 2014-09-06
From: David Miller @ 2014-09-06 19:30 UTC (permalink / raw)
To: jeffrey.t.kirsher; +Cc: netdev, nhorman, sassmann, jogreene
In-Reply-To: <1410006842-26517-1-git-send-email-jeffrey.t.kirsher@intel.com>
From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Sat, 6 Sep 2014 05:34:00 -0700
> This series contains updates to e1000 and igb.
>
> Krzysztof provides a patch to cleanup the coding style in e1000 to quiet
> checkpatch.pl warnings.
>
> Todd adds two boolean flags to igb to allow for changes in the
> advertised EEE speeds from ethtool.
>
> The following are changes since commit cfecec56ae7c7c40f23fbdac04acee027ca3bd66:
> mlx4: only pull headers into skb head
> and are available in the git repository at:
> git://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/net-next master
Pulled, thanks Jeff.
^ permalink raw reply
* Re: [PATCH net-next] tcp: remove obsolete comment about TCP_SKB_CB(skb)->when in tcp_fragment()
From: David Miller @ 2014-09-06 19:29 UTC (permalink / raw)
To: ncardwell; +Cc: edumazet, ycheng, netdev
In-Reply-To: <1410016700-9145-1-git-send-email-ncardwell@google.com>
From: Neal Cardwell <ncardwell@google.com>
Date: Sat, 6 Sep 2014 11:18:20 -0400
> The TCP_SKB_CB(skb)->when field no longer exists as of recent change
> 7faee5c0d514 ("tcp: remove TCP_SKB_CB(skb)->when"). And in any case,
> tcp_fragment() is called on already-transmitted packets from the
> __tcp_retransmit_skb() call site, so copying timestamps of any kind
> in this spot is quite sensible.
>
> Signed-off-by: Neal Cardwell <ncardwell@google.com>
> Reported-by: Yuchung Cheng <ycheng@google.com>
Applied, thanks Neal.
^ permalink raw reply
* Re: [PATCH] bluetooth: btusb: Fix issue with suspend
From: Greg KH @ 2014-09-06 19:19 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Larry Finger, Gustavo F. Padovan, Johan Hedberg,
BlueZ development, Network Development, Champion Chen, Stable
In-Reply-To: <9F1659DE-61C2-4CA9-8696-16F120273154@holtmann.org>
On Sat, Sep 06, 2014 at 12:12:44PM -0700, Marcel Holtmann wrote:
> Hi Larry,
>
> >> one other thing. Please let maintainers decide when a patch is
> >> useful for stable and when not. Patches need to go upstream first
> >> anyway. People spamming stable@vger.kernel.org with patches that
> >> are not reviewed yet is pretty much a waste of time.
> >
> > Sorry. In the wireless tree, the patch author suggests that the
> > patch is suitable for stable. If the maintainer disagrees, he strips
> > the Cc to stable. It seems that you do it the other way. In either
> > case, I do not think the stable patches are picked up by the
> > maintainers of stable versions until the patch hits mainline.
>
> I have no idea what the stable@vger.kernel.org guys do with patches
> that are not yet upstream.
I ignore them :)
> However it is a waste of their time sending stuff to their mailing
> list that has not even seen a maintainer review. You can just mention
> it in the comment section that this might qualify for stable.
It doesn't bother me at all for the cc:, they are easy to filter away,
and it keeps me informed as to where patches will be coming from in the
future, and helps to track down things that get dropped.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH] bluetooth: btusb: Fix issue with suspend
From: Marcel Holtmann @ 2014-09-06 19:12 UTC (permalink / raw)
To: Larry Finger
Cc: Gustavo F. Padovan, Johan Hedberg, BlueZ development,
Network Development, Champion Chen, Stable
In-Reply-To: <540B5B03.9070203@lwfinger.net>
Hi Larry,
>> one other thing. Please let maintainers decide when a patch is useful for stable and when not. Patches need to go upstream first anyway. People spamming stable@vger.kernel.org with patches that are not reviewed yet is pretty much a waste of time.
>
> Sorry. In the wireless tree, the patch author suggests that the patch is suitable for stable. If the maintainer disagrees, he strips the Cc to stable. It seems that you do it the other way. In either case, I do not think the stable patches are picked up by the maintainers of stable versions until the patch hits mainline.
I have no idea what the stable@vger.kernel.org guys do with patches that are not yet upstream. However it is a waste of their time sending stuff to their mailing list that has not even seen a maintainer review. You can just mention it in the comment section that this might qualify for stable.
Regards
Marcel
^ permalink raw reply
* [PATCH V2] bluetooth: btusb: Fix issue with suspend
From: Larry Finger @ 2014-09-06 19:06 UTC (permalink / raw)
To: Marcel Holtmann, Gustavo Padovan, Johan Hedberg
Cc: linux-bluetooth-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA, Champion Chen, Larry Finger
From: Champion Chen <champion_chen-kXabqFNEczNtrwSWzY7KCg@public.gmane.org>
Suspend could fail for some platforms because
btusb_suspend==> btusb_stop_traffic ==> usb_kill_anchored_urbs,
When btusb_bulk_complete returns before system suspend and resubmits an urb,
the system cannot enter suspend state.
Signed-off-by: Champion Chen <champion_chen-kXabqFNEczNtrwSWzY7KCg@public.gmane.org>
Signed-off-by: Larry Finger <Larry.Finger-tQ5ms3gMjBLk1uMJSBkQmQ@public.gmane.org>
---
V2 - Fix incorrect test as noted by Marcel.
---
Johan,
To help Champion with the process, I have formatted the patch in
the correct manner. I hope I understand the issue correctly and
stated it in a coherent manner in the commit message.
Larry
---
drivers/bluetooth/btusb.c | 9 +++++++++
1 file changed, 9 insertions(+)
Index: linux-2.6/drivers/bluetooth/btusb.c
===================================================================
--- linux-2.6.orig/drivers/bluetooth/btusb.c
+++ linux-2.6/drivers/bluetooth/btusb.c
@@ -330,6 +330,9 @@ static void btusb_intr_complete(struct u
BT_ERR("%s corrupted event packet", hdev->name);
hdev->stat.err_rx++;
}
+ } else if (urb->status == -ENOENT) {
+ /* Avoid suspend failed when usb_kill_urb */
+ return;
}
if (!test_bit(BTUSB_INTR_RUNNING, &data->flags))
@@ -418,6 +421,9 @@ static void btusb_bulk_complete(struct u
BT_ERR("%s corrupted ACL packet", hdev->name);
hdev->stat.err_rx++;
}
+ } else if (urb->status == -ENOENT) {
+ /* Avoid suspend failed when usb_kill_urb */
+ return;
}
if (!test_bit(BTUSB_BULK_RUNNING, &data->flags))
@@ -512,6 +518,9 @@ static void btusb_isoc_complete(struct u
hdev->stat.err_rx++;
}
}
+ } else if (urb->status == -ENOENT) {
+ /* Avoid suspend failed when usb_kill_urb */
+ return;
}
if (!test_bit(BTUSB_ISOC_RUNNING, &data->flags))
^ permalink raw reply
* Re: [PATCH] bluetooth: btusb: Fix issue with suspend
From: Larry Finger @ 2014-09-06 19:05 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Gustavo F. Padovan, Johan Hedberg, BlueZ development,
Network Development, Champion Chen, Stable
In-Reply-To: <D5B40282-A085-4B47-8F7B-27480CAB6B19-kz+m5ild9QBg9hUCZPvPmw@public.gmane.org>
On 09/06/2014 12:35 PM, Marcel Holtmann wrote:
> Hi Larry,
>
>
> one other thing. Please let maintainers decide when a patch is useful for stable and when not. Patches need to go upstream first anyway. People spamming stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org with patches that are not reviewed yet is pretty much a waste of time.
Sorry. In the wireless tree, the patch author suggests that the patch is
suitable for stable. If the maintainer disagrees, he strips the Cc to stable. It
seems that you do it the other way. In either case, I do not think the stable
patches are picked up by the maintainers of stable versions until the patch hits
mainline.
Larry
^ permalink raw reply
* Re: Performance regression on kernels 3.10 and newer
From: Eric Dumazet @ 2014-09-06 18:21 UTC (permalink / raw)
To: David Miller, Nicolas Dichtel; +Cc: therbert, alexander.h.duyck, netdev
In-Reply-To: <1410021507.11872.35.camel@edumazet-glaptop2.roam.corp.google.com>
On Sat, 2014-09-06 at 09:38 -0700, Eric Dumazet wrote:
> On Sat, 2014-09-06 at 08:46 -0700, Eric Dumazet wrote:
>
> > Works fine on IPv4, not on IPv6, I will submit a v2 (with a proper title
> > btw)
>
> Oh well, we first need to fix IPv6 early demux.
>
> Current net-next kernel profile
>
> + 20.95% netserver [kernel.kallsyms] [k] dst_release
> + 19.33% netserver [kernel.kallsyms] [k] ip6_pol_route.isra.46
> + 11.75% netserver [kernel.kallsyms] [k] inet6_sk_rx_dst_set
> + 3.72% netserver [kernel.kallsyms] [k] ip6_input_finish
>
> So we repeat over and over calls to inet6_sk_rx_dst_set(),
> something is surely wrong.
>
Nicolas, it seems that after your commit
(6f3118b571b8a4c06c7985dc3172c3526cb86253 "ipv6: use net->rt_genid to
check dst validity") IP early demux is broken.
Basically, we now hit the following condition in ip6_dst_check()
if (rt->rt6i_genid != rt_genid_ipv6(dev_net(rt->dst.dev)))
In my traces, rt->rt6i_genid stays at 2, and the rt_genid_ipv6()
increments every time a route is added/deleted.
So we keep calling dst_release(), and inet6_sk_rx_dst_set(),
but the route we keep installing is already wrong, as its genid is too
old.
Does following fix make any sense ?
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index f74b0417bd60..a139a16298e3 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -947,9 +947,10 @@ restart:
nrt = rt6_alloc_cow(rt, &fl6->daddr, &fl6->saddr);
else if (!(rt->dst.flags & DST_HOST))
nrt = rt6_alloc_clone(rt, &fl6->daddr);
- else
+ else {
+ rt->rt6i_genid = rt_genid_ipv6(net);
goto out2;
-
+ }
ip6_rt_put(rt);
rt = nrt ? : net->ipv6.ip6_null_entry;
^ permalink raw reply related
* Re: [PATCH net-next 2/3] net: bpf: arm: address randomize and write protect JIT code
From: Mircea Gherzan @ 2014-09-06 17:36 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: davem, ast, netdev
In-Reply-To: <1409996567-2170-3-git-send-email-dborkman@redhat.com>
2014-09-06 11:42 GMT+02:00 Daniel Borkmann <dborkman@redhat.com>:
> This is the ARM variant for 314beb9bcab ("x86: bpf_jit_comp: secure bpf
> jit against spraying attacks").
>
> It is now possible to implement it due to commits 75374ad47c64 ("ARM: mm:
> Define set_memory_* functions for ARM") and dca9aa92fc7c ("ARM: add
> DEBUG_SET_MODULE_RONX option to Kconfig") which added infrastructure for
> this facility.
>
> Thus, this patch makes sure the BPF generated JIT code is marked RO, as
> other kernel text sections, and also lets the generated JIT code start
> at a pseudo random offset instead on a page boundary. The holes are filled
> with illegal instructions.
>
> JIT tested on armv7hl with BPF test suite.
>
> Reference: http://mainisusuallyafunction.blogspot.com/2012/11/attacking-hardened-linux-systems-with.html
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
> Cc: Mircea Gherzan <mgherzan@gmail.com>
> ---
> arch/arm/net/bpf_jit_32.c | 32 ++++++++++++++++++++++++++------
> 1 file changed, 26 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm/net/bpf_jit_32.c b/arch/arm/net/bpf_jit_32.c
> index a76623b..2d1a5b9 100644
> --- a/arch/arm/net/bpf_jit_32.c
> +++ b/arch/arm/net/bpf_jit_32.c
> @@ -12,7 +12,6 @@
> #include <linux/compiler.h>
> #include <linux/errno.h>
> #include <linux/filter.h>
> -#include <linux/moduleloader.h>
> #include <linux/netdevice.h>
> #include <linux/string.h>
> #include <linux/slab.h>
> @@ -174,6 +173,15 @@ static inline bool is_load_to_a(u16 inst)
> }
> }
>
> +static void jit_fill_hole(void *area, unsigned int size)
> +{
> + /* Insert illegal UND instructions. */
> + u32 *ptr, fill_ins = 0xe7ffffff;
> + /* We are guaranteed to have aligned memory. */
> + for (ptr = area; size >= sizeof(u32); size -= sizeof(u32))
> + *ptr++ = fill_ins;
> +}
> +
> static void build_prologue(struct jit_ctx *ctx)
> {
> u16 reg_set = saved_regs(ctx);
> @@ -859,9 +867,11 @@ b_epilogue:
>
> void bpf_jit_compile(struct bpf_prog *fp)
> {
> + struct bpf_binary_header *header;
> struct jit_ctx ctx;
> unsigned tmp_idx;
> unsigned alloc_size;
> + u8 *target_ptr;
>
> if (!bpf_jit_enable)
> return;
> @@ -897,13 +907,15 @@ void bpf_jit_compile(struct bpf_prog *fp)
> /* there's nothing after the epilogue on ARMv7 */
> build_epilogue(&ctx);
> #endif
> -
> alloc_size = 4 * ctx.idx;
> - ctx.target = module_alloc(alloc_size);
> - if (unlikely(ctx.target == NULL))
> + header = bpf_jit_binary_alloc(alloc_size, &target_ptr,
> + 4, jit_fill_hole);
> + if (header == NULL)
> goto out;
>
> + ctx.target = (u32 *) target_ptr;
> ctx.idx = 0;
> +
> build_prologue(&ctx);
> build_body(&ctx);
> build_epilogue(&ctx);
> @@ -919,6 +931,7 @@ void bpf_jit_compile(struct bpf_prog *fp)
> /* there are 2 passes here */
> bpf_jit_dump(fp->len, alloc_size, 2, ctx.target);
>
> + set_memory_ro((unsigned long)header, header->pages);
> fp->bpf_func = (void *)ctx.target;
> fp->jited = 1;
> out:
> @@ -928,8 +941,15 @@ out:
>
> void bpf_jit_free(struct bpf_prog *fp)
> {
> - if (fp->jited)
> - module_free(NULL, fp->bpf_func);
> + unsigned long addr = (unsigned long)fp->bpf_func & PAGE_MASK;
> + struct bpf_binary_header *header = (void *)addr;
> +
> + if (!fp->jited)
> + goto free_filter;
> +
> + set_memory_rw(addr, header->pages);
> + bpf_jit_binary_free(header);
>
> +free_filter:
> bpf_prog_unlock_free(fp);
> }
Acked-by: Mircea Gherzan <mgherzan@gmail.com>
^ permalink raw reply
* Re: [PATCH] bluetooth: btusb: Fix issue with suspend
From: Marcel Holtmann @ 2014-09-06 17:35 UTC (permalink / raw)
To: Larry Finger
Cc: Gustavo F. Padovan, Johan Hedberg, BlueZ development,
Network Development, Champion Chen, Stable
In-Reply-To: <540B4159.9000003-tQ5ms3gMjBLk1uMJSBkQmQ@public.gmane.org>
Hi Larry,
>>> Suspend could fail for some platforms because
>>> btusb_suspend==> btusb_stop_traffic ==> usb_kill_anchored_urbs,
>>>
>>> When btusb_bulk_complete returns before system suspend and resubmits an urb,
>>> the system cannot enter suspend state.
>>>
>>> Signed-off-by: Champion Chen <champion_chen-kXabqFNEczNtrwSWzY7KCg@public.gmane.org>
>>> Signed-off-by: Larry Finger <Larry.Finger-tQ5ms3gMjBLk1uMJSBkQmQ@public.gmane.org>
>>> Cc: Stable <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
>>> ---
>>> Johan,
>>>
>>> To help Champion with the process, I have formatted the patch in
>>> the correct manner. I hope I understand the issue correctly and
>>> stated it in a coherent manner in the commit message.
>>>
>>> Larry
>>> ---
>>> drivers/bluetooth/btusb.c | 9 +++++++++
>>> 1 file changed, 9 insertions(+)
>>>
>>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>>> index 292c38e..45a7211 100644
>>> --- a/drivers/bluetooth/btusb.c
>>> +++ b/drivers/bluetooth/btusb.c
>>> @@ -330,6 +330,9 @@ static void btusb_intr_complete(struct urb *urb)
>>> BT_ERR("%s corrupted event packet", hdev->name);
>>> hdev->stat.err_rx++;
>>> }
>>> + } else if (urb->status == -ENOENT) {
>>> + /* Avoid suspend failed when usb_kill_urb */
>>> + return;
>>> }
>>>
>>> if (!test_bit(BTUSB_INTR_RUNNING, &data->flags))
>>> @@ -417,6 +420,9 @@ static void btusb_bulk_complete(struct urb *urb)
>>> urb->actual_length) < 0) {
>>> BT_ERR("%s corrupted ACL packet", hdev->name);
>>> hdev->stat.err_rx++;
>>> + } else if (urb->status == -ENOENT) {
>>> + /* Avoid suspend failed when usb_kill_urb */
>>> + return;
>>> }
>>
>> this one is utterly bogus. Either urb->status is 0 or it is -ENOENT, but it will not be both. I think you put it to the wrong if clause.
>
> Thanks for the review. Obviously, you are correct. A revised patch will be sent soon.
one other thing. Please let maintainers decide when a patch is useful for stable and when not. Patches need to go upstream first anyway. People spamming stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org with patches that are not reviewed yet is pretty much a waste of time.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH] bluetooth: btusb: Fix issue with suspend
From: Larry Finger @ 2014-09-06 17:16 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Gustavo F. Padovan, Johan Hedberg, linux-bluetooth, netdev,
Champion Chen, Stable
In-Reply-To: <6DF81E54-681F-4B18-95C7-F4684FF19786@holtmann.org>
On 09/06/2014 12:02 PM, Marcel Holtmann wrote:
> Hi Larry,
>
>> Suspend could fail for some platforms because
>> btusb_suspend==> btusb_stop_traffic ==> usb_kill_anchored_urbs,
>>
>> When btusb_bulk_complete returns before system suspend and resubmits an urb,
>> the system cannot enter suspend state.
>>
>> Signed-off-by: Champion Chen <champion_chen@realsil.com.cn>
>> Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
>> Cc: Stable <stable@vger.kernel.org>
>> ---
>> Johan,
>>
>> To help Champion with the process, I have formatted the patch in
>> the correct manner. I hope I understand the issue correctly and
>> stated it in a coherent manner in the commit message.
>>
>> Larry
>> ---
>> drivers/bluetooth/btusb.c | 9 +++++++++
>> 1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>> index 292c38e..45a7211 100644
>> --- a/drivers/bluetooth/btusb.c
>> +++ b/drivers/bluetooth/btusb.c
>> @@ -330,6 +330,9 @@ static void btusb_intr_complete(struct urb *urb)
>> BT_ERR("%s corrupted event packet", hdev->name);
>> hdev->stat.err_rx++;
>> }
>> + } else if (urb->status == -ENOENT) {
>> + /* Avoid suspend failed when usb_kill_urb */
>> + return;
>> }
>>
>> if (!test_bit(BTUSB_INTR_RUNNING, &data->flags))
>> @@ -417,6 +420,9 @@ static void btusb_bulk_complete(struct urb *urb)
>> urb->actual_length) < 0) {
>> BT_ERR("%s corrupted ACL packet", hdev->name);
>> hdev->stat.err_rx++;
>> + } else if (urb->status == -ENOENT) {
>> + /* Avoid suspend failed when usb_kill_urb */
>> + return;
>> }
>
> this one is utterly bogus. Either urb->status is 0 or it is -ENOENT, but it will not be both. I think you put it to the wrong if clause.
Thanks for the review. Obviously, you are correct. A revised patch will be sent
soon.
The code sent by Champion is now posted at
http://github.com/lwfinger/rtl8723au_bt/new. It works for everyone that has
tested it.
Larry
^ permalink raw reply
* Re: [PATCH] bluetooth: btusb: Fix issue with suspend
From: Marcel Holtmann @ 2014-09-06 17:02 UTC (permalink / raw)
To: Larry Finger
Cc: Gustavo F. Padovan, Johan Hedberg, linux-bluetooth, netdev,
Champion Chen, Stable
In-Reply-To: <1409930194-6318-1-git-send-email-Larry.Finger@lwfinger.net>
Hi Larry,
> Suspend could fail for some platforms because
> btusb_suspend==> btusb_stop_traffic ==> usb_kill_anchored_urbs,
>
> When btusb_bulk_complete returns before system suspend and resubmits an urb,
> the system cannot enter suspend state.
>
> Signed-off-by: Champion Chen <champion_chen@realsil.com.cn>
> Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
> Cc: Stable <stable@vger.kernel.org>
> ---
> Johan,
>
> To help Champion with the process, I have formatted the patch in
> the correct manner. I hope I understand the issue correctly and
> stated it in a coherent manner in the commit message.
>
> Larry
> ---
> drivers/bluetooth/btusb.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 292c38e..45a7211 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -330,6 +330,9 @@ static void btusb_intr_complete(struct urb *urb)
> BT_ERR("%s corrupted event packet", hdev->name);
> hdev->stat.err_rx++;
> }
> + } else if (urb->status == -ENOENT) {
> + /* Avoid suspend failed when usb_kill_urb */
> + return;
> }
>
> if (!test_bit(BTUSB_INTR_RUNNING, &data->flags))
> @@ -417,6 +420,9 @@ static void btusb_bulk_complete(struct urb *urb)
> urb->actual_length) < 0) {
> BT_ERR("%s corrupted ACL packet", hdev->name);
> hdev->stat.err_rx++;
> + } else if (urb->status == -ENOENT) {
> + /* Avoid suspend failed when usb_kill_urb */
> + return;
> }
this one is utterly bogus. Either urb->status is 0 or it is -ENOENT, but it will not be both. I think you put it to the wrong if clause.
> }
>
> @@ -512,6 +518,9 @@ static void btusb_isoc_complete(struct urb *urb)
> hdev->stat.err_rx++;
> }
> }
> + } else if (urb->status == -ENOENT) {
> + /* Avoid suspend failed when usb_kill_urb */
> + return;
> }
>
> if (!test_bit(BTUSB_ISOC_RUNNING, &data->flags))
Regards
Marcel
^ permalink raw reply
* Re: Performance regression on kernels 3.10 and newer
From: Eric Dumazet @ 2014-09-06 16:38 UTC (permalink / raw)
To: David Miller; +Cc: therbert, alexander.h.duyck, netdev
In-Reply-To: <1410018384.11872.33.camel@edumazet-glaptop2.roam.corp.google.com>
On Sat, 2014-09-06 at 08:46 -0700, Eric Dumazet wrote:
> Works fine on IPv4, not on IPv6, I will submit a v2 (with a proper title
> btw)
Oh well, we first need to fix IPv6 early demux.
Current net-next kernel profile
+ 20.95% netserver [kernel.kallsyms] [k] dst_release
+ 19.33% netserver [kernel.kallsyms] [k] ip6_pol_route.isra.46
+ 11.75% netserver [kernel.kallsyms] [k] inet6_sk_rx_dst_set
+ 3.72% netserver [kernel.kallsyms] [k] ip6_input_finish
So we repeat over and over calls to inet6_sk_rx_dst_set(),
something is surely wrong.
^ permalink raw reply
* Re: [PATCH net-next 3/3] net: bpf: be friendly to kmemcheck
From: Alexei Starovoitov @ 2014-09-06 16:09 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: David S. Miller, Network Development, Mikulas Patocka
In-Reply-To: <1409996567-2170-4-git-send-email-dborkman@redhat.com>
On Sat, Sep 6, 2014 at 2:42 AM, Daniel Borkmann <dborkman@redhat.com> wrote:
> Reported by Mikulas Patocka, kmemcheck currently barks out a
> false positive since we don't have special kmemcheck annotation
> for bitfields used in bpf_prog structure.
>
> We currently have jited:1, len:31 and thus when accessing len
> while CONFIG_KMEMCHECK enabled, kmemcheck throws a warning that
> we're reading uninitialized memory.
>
> As we don't need the whole bit universe for pages member, we
> can just split it to u16 and use a bool flag for jited instead
> of a bitfield.
>
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> Cc: Alexei Starovoitov <ast@plumgrid.com>
> ---
> arch/arm/net/bpf_jit_32.c | 2 +-
> arch/mips/net/bpf_jit.c | 2 +-
> arch/powerpc/net/bpf_jit_comp.c | 2 +-
> arch/s390/net/bpf_jit_comp.c | 2 +-
> arch/sparc/net/bpf_jit_comp.c | 2 +-
> arch/x86/net/bpf_jit_comp.c | 2 +-
> include/linux/filter.h | 6 +++---
> net/core/filter.c | 2 +-
> 8 files changed, 10 insertions(+), 10 deletions(-)
This one also looks good. Thanks!
Acked-by: Alexei Starovoitov <ast@plumgrid.com>
^ permalink raw reply
* Re: [PATCH v10 net-next 2/2] net: filter: split filter.h and expose eBPF to user space
From: Alexei Starovoitov @ 2014-09-06 16:04 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: David S. Miller, Ingo Molnar, Linus Torvalds, Andy Lutomirski,
Steven Rostedt, Daniel Borkmann, Hannes Frederic Sowa,
Chema Gonzalez, Eric Dumazet, Peter Zijlstra, H. Peter Anvin,
Andrew Morton, Kees Cook, Linux API, Network Development, LKML
In-Reply-To: <20140906141051.GA4245@salvia>
On Sat, Sep 6, 2014 at 7:10 AM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Thu, Sep 04, 2014 at 10:17:18PM -0700, Alexei Starovoitov wrote:
>> allow user space to generate eBPF programs
>>
>> uapi/linux/bpf.h: eBPF instruction set definition
>>
>> linux/filter.h: the rest
>>
>> This patch only moves macro definitions, but practically it freezes existing
>> eBPF instruction set, though new instructions can still be added in the future.
>>
>> These eBPF definitions cannot go into uapi/linux/filter.h, since the names
>> may conflict with existing applications.
>>
>> Full eBPF ISA description is in Documentation/networking/filter.txt
>
> I think you need to have at least one single interface using this
> before you can expose it to userspace. So this should come in the
> small batch that introduces the first interface of your ebpf code in
> userspace. AFAIK, this has been the policy so far.
That's what I've been doing over the last year.
My first eBPF patch was in Sep of 2013!
since then I've been only tweaking and massaging it.
Nothing fundamentally changed.
Last few month I've been posting these series with not only
first user, but with multiple. Many examples, test cases and so on.
The series became big and Dave asked to split them.
Please see the cover letter. These two patches is stage I.
More examples and use cases in stage II, stage III, stage IV, etc
All these patches are ready. I'm only submitting them one at
a time to make review easier.
Please see them in my tree if you interested.
Thanks
^ permalink raw reply
* Re: Performance regression on kernels 3.10 and newer
From: Eric Dumazet @ 2014-09-06 15:46 UTC (permalink / raw)
To: David Miller; +Cc: therbert, alexander.h.duyck, netdev
In-Reply-To: <1410017237.11872.31.camel@edumazet-glaptop2.roam.corp.google.com>
On Sat, 2014-09-06 at 08:27 -0700, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> Alexander Duyck reported high false sharing on dst refcount in tcp stack
> when prequeue is used. prequeue is the mechanism used when a thread is
> blocked in recvmsg()/read() on the TCP socket.
>
> We already try to use RCU in input path as much as possible, but we were
> forced to take a refcount on the dst when skb escaped RCU protected
> region. When/if the user thread runs on different cpu, dst_release()
> will then touch dst refcount again.
>
> Commit 093162553c33 (tcp: force a dst refcount when prequeue packet)
> was an example of a race fix.
>
> It turns out the only remaining usage of skb->dst for a packet stored
> in a TCP socket prequeue is IP early demux.
>
> We can add a logic to detect when IP early demux is probably going
> to use skb->dst.
>
> In IPv4 stack, we also can drop dst when skb has to be queued into
> socket backlog. IPv6 needs extra care before doing the same.
>
> Many thanks to Alexander for providing a nice bug report, git bisection,
> and reproducer.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: Alexander Duyck <alexander.h.duyck@intel.com>
Works fine on IPv4, not on IPv6, I will submit a v2 (with a proper title
btw)
^ permalink raw reply
* Re: [PATCH net-next] tcp: remove obsolete comment about TCP_SKB_CB(skb)->when in tcp_fragment()
From: Eric Dumazet @ 2014-09-06 15:30 UTC (permalink / raw)
To: Neal Cardwell; +Cc: David Miller, edumazet, ycheng, netdev
In-Reply-To: <1410016700-9145-1-git-send-email-ncardwell@google.com>
On Sat, 2014-09-06 at 11:18 -0400, Neal Cardwell wrote:
> The TCP_SKB_CB(skb)->when field no longer exists as of recent change
> 7faee5c0d514 ("tcp: remove TCP_SKB_CB(skb)->when"). And in any case,
> tcp_fragment() is called on already-transmitted packets from the
> __tcp_retransmit_skb() call site, so copying timestamps of any kind
> in this spot is quite sensible.
>
> Signed-off-by: Neal Cardwell <ncardwell@google.com>
> Reported-by: Yuchung Cheng <ycheng@google.com>
> ---
> net/ipv4/tcp_output.c | 3 ---
Acked-by: Eric Dumazet <edumazet@google.com>
^ permalink raw reply
* Re: Performance regression on kernels 3.10 and newer
From: Eric Dumazet @ 2014-09-06 15:27 UTC (permalink / raw)
To: David Miller; +Cc: therbert, alexander.h.duyck, netdev
In-Reply-To: <1410014704.11872.16.camel@edumazet-glaptop2.roam.corp.google.com>
From: Eric Dumazet <edumazet@google.com>
Alexander Duyck reported high false sharing on dst refcount in tcp stack
when prequeue is used. prequeue is the mechanism used when a thread is
blocked in recvmsg()/read() on the TCP socket.
We already try to use RCU in input path as much as possible, but we were
forced to take a refcount on the dst when skb escaped RCU protected
region. When/if the user thread runs on different cpu, dst_release()
will then touch dst refcount again.
Commit 093162553c33 (tcp: force a dst refcount when prequeue packet)
was an example of a race fix.
It turns out the only remaining usage of skb->dst for a packet stored
in a TCP socket prequeue is IP early demux.
We can add a logic to detect when IP early demux is probably going
to use skb->dst.
In IPv4 stack, we also can drop dst when skb has to be queued into
socket backlog. IPv6 needs extra care before doing the same.
Many thanks to Alexander for providing a nice bug report, git bisection,
and reproducer.
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Alexander Duyck <alexander.h.duyck@intel.com>
---
net/ipv4/tcp_ipv4.c | 27 ++++++++++++++++++---------
net/ipv6/tcp_ipv6.c | 15 +++++++++------
2 files changed, 27 insertions(+), 15 deletions(-)
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 3f9bc3f0bba0..5683505553bf 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1559,7 +1559,11 @@ bool tcp_prequeue(struct sock *sk, struct sk_buff *skb)
skb_queue_len(&tp->ucopy.prequeue) == 0)
return false;
- skb_dst_force(skb);
+ if (likely(sk->sk_rx_dst))
+ skb_dst_drop(skb);
+ else
+ skb_dst_force(skb);
+
__skb_queue_tail(&tp->ucopy.prequeue, skb);
tp->ucopy.memory += skb->truesize;
if (tp->ucopy.memory > sk->sk_rcvbuf) {
@@ -1682,11 +1686,14 @@ process:
if (!tcp_prequeue(sk, skb))
ret = tcp_v4_do_rcv(sk, skb);
}
- } else if (unlikely(sk_add_backlog(sk, skb,
- sk->sk_rcvbuf + sk->sk_sndbuf))) {
- bh_unlock_sock(sk);
- NET_INC_STATS_BH(net, LINUX_MIB_TCPBACKLOGDROP);
- goto discard_and_relse;
+ } else {
+ skb_dst_drop(skb);
+ if (unlikely(sk_add_backlog(sk, skb,
+ sk->sk_rcvbuf + sk->sk_sndbuf))) {
+ bh_unlock_sock(sk);
+ NET_INC_STATS_BH(net, LINUX_MIB_TCPBACKLOGDROP);
+ goto discard_and_relse;
+ }
}
bh_unlock_sock(sk);
@@ -1765,9 +1772,11 @@ void inet_sk_rx_dst_set(struct sock *sk, const struct sk_buff *skb)
{
struct dst_entry *dst = skb_dst(skb);
- dst_hold(dst);
- sk->sk_rx_dst = dst;
- inet_sk(sk)->rx_dst_ifindex = skb->skb_iif;
+ if (dst) {
+ dst_hold(dst);
+ sk->sk_rx_dst = dst;
+ inet_sk(sk)->rx_dst_ifindex = skb->skb_iif;
+ }
}
EXPORT_SYMBOL(inet_sk_rx_dst_set);
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 5b3c70ff7a72..1835480336ac 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -93,13 +93,16 @@ static struct tcp_md5sig_key *tcp_v6_md5_do_lookup(struct sock *sk,
static void inet6_sk_rx_dst_set(struct sock *sk, const struct sk_buff *skb)
{
struct dst_entry *dst = skb_dst(skb);
- const struct rt6_info *rt = (const struct rt6_info *)dst;
- dst_hold(dst);
- sk->sk_rx_dst = dst;
- inet_sk(sk)->rx_dst_ifindex = skb->skb_iif;
- if (rt->rt6i_node)
- inet6_sk(sk)->rx_dst_cookie = rt->rt6i_node->fn_sernum;
+ if (dst) {
+ const struct rt6_info *rt = (const struct rt6_info *)dst;
+
+ dst_hold(dst);
+ sk->sk_rx_dst = dst;
+ inet_sk(sk)->rx_dst_ifindex = skb->skb_iif;
+ if (rt->rt6i_node)
+ inet6_sk(sk)->rx_dst_cookie = rt->rt6i_node->fn_sernum;
+ }
}
static void tcp_v6_hash(struct sock *sk)
^ permalink raw reply related
* [PATCH net-next] tcp: remove obsolete comment about TCP_SKB_CB(skb)->when in tcp_fragment()
From: Neal Cardwell @ 2014-09-06 15:18 UTC (permalink / raw)
To: David Miller; +Cc: edumazet, ycheng, netdev, Neal Cardwell
The TCP_SKB_CB(skb)->when field no longer exists as of recent change
7faee5c0d514 ("tcp: remove TCP_SKB_CB(skb)->when"). And in any case,
tcp_fragment() is called on already-transmitted packets from the
__tcp_retransmit_skb() call site, so copying timestamps of any kind
in this spot is quite sensible.
Signed-off-by: Neal Cardwell <ncardwell@google.com>
Reported-by: Yuchung Cheng <ycheng@google.com>
---
net/ipv4/tcp_output.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 3b22dcb..7f1280d 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -1146,9 +1146,6 @@ int tcp_fragment(struct sock *sk, struct sk_buff *skb, u32 len,
buff->ip_summed = skb->ip_summed;
- /* Looks stupid, but our code really uses when of
- * skbs, which it never sent before. --ANK
- */
buff->tstamp = skb->tstamp;
tcp_fragment_tstamp(skb, buff);
--
2.1.0.rc2.206.gedb03e5
^ permalink raw reply related
* Re: Performance regression on kernels 3.10 and newer
From: Eric Dumazet @ 2014-09-06 14:45 UTC (permalink / raw)
To: David Miller; +Cc: therbert, alexander.h.duyck, netdev
In-Reply-To: <20140821.162406.2076033247433267586.davem@davemloft.net>
On Thu, 2014-08-21 at 16:24 -0700, David Miller wrote:
> From: Tom Herbert <therbert@google.com>
> Date: Thu, 14 Aug 2014 16:25:53 -0700
>
> > I don't know if it's the same problem, but I did post a patch back in
> > January that would resolve false sharing of dst->__refcnt and rt_genid
> > in the same cacheline. We could revisit that if it helps.
>
> I think that regardless of what happens in the discussion here, you
> should repost that patch.
I believe this wont be necessary, the real meat is elsewhere.
There are very few cases where we really need to keep the dst attached
to skb.
Its mostly because of IP early demux, and its possible to take care of
the fast path (where sk->sk_rx_dst is set), and simply drop dst before
prequeue in this case.
Same stuff for the backlog, used when socket is owned by user.
I'll send a patch.
^ permalink raw reply
* Re: [PATCH 3/9] net: Move main gso loop out of dev_hard_start_xmit() into helper.
From: Shmulik Ladkani @ 2014-09-06 14:38 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20140901.152450.815155429225102019.davem@davemloft.net>
Hi,
On Mon, 01 Sep 2014 15:24:50 -0700 (PDT) David Miller <davem@davemloft.net> wrote:
> @@ -2681,25 +2709,7 @@ int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
> }
>
> gso:
> - do {
> - struct sk_buff *nskb = skb->next;
> -
> - skb->next = nskb->next;
> - nskb->next = NULL;
> -
> - rc = xmit_one(nskb, dev, txq);
> - if (unlikely(rc != NETDEV_TX_OK)) {
> - if (rc & ~NETDEV_TX_MASK)
> - goto out_kfree_gso_skb;
> - nskb->next = skb->next;
> - skb->next = nskb;
> - return rc;
> - }
> - if (unlikely(netif_xmit_stopped(txq) && skb->next))
> - return NETDEV_TX_BUSY;
> - } while (skb->next);
> -
> -out_kfree_gso_skb:
> + skb->next = xmit_list(skb->next, dev, txq, &rc);
> if (likely(skb->next == NULL)) {
> skb->destructor = DEV_GSO_CB(skb)->destructor;
> consume_skb(skb);
> }
> out_kfree_skb:
> kfree_skb(skb);
Formely, if 'netif_xmit_stopped(txq) && skb->next', NETDEV_TX_BUSY was
returned, but the head 'skb' wasn't freed;
Currently, if 'xmit_list' returns non-NULL due to same reason, head skb
gets freed in out_kfree_skb.
I'm not certain, but it looks like an oversight that got fixed later in
the patchset:
In net-next ce93718fb "net: Don't keep around original SKB when we
software segment GSO frames." the 'kfree_skb(skb)' in
'dev_hard_start_xmit' got relocated into validate_xmit_skb's gso
segmentation part, making the issue disappear.
Am I right? Didn't we miss anything else here?
^ permalink raw reply
* Re: [PATCH v10 net-next 2/2] net: filter: split filter.h and expose eBPF to user space
From: Pablo Neira Ayuso @ 2014-09-06 14:10 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: David S. Miller, Ingo Molnar, Linus Torvalds, Andy Lutomirski,
Steven Rostedt, Daniel Borkmann, Hannes Frederic Sowa,
Chema Gonzalez, Eric Dumazet, Peter Zijlstra, H. Peter Anvin,
Andrew Morton, Kees Cook, linux-api-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1409894238-9055-3-git-send-email-ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org>
On Thu, Sep 04, 2014 at 10:17:18PM -0700, Alexei Starovoitov wrote:
> allow user space to generate eBPF programs
>
> uapi/linux/bpf.h: eBPF instruction set definition
>
> linux/filter.h: the rest
>
> This patch only moves macro definitions, but practically it freezes existing
> eBPF instruction set, though new instructions can still be added in the future.
>
> These eBPF definitions cannot go into uapi/linux/filter.h, since the names
> may conflict with existing applications.
>
> Full eBPF ISA description is in Documentation/networking/filter.txt
I think you need to have at least one single interface using this
before you can expose it to userspace. So this should come in the
small batch that introduces the first interface of your ebpf code in
userspace. AFAIK, this has been the policy so far.
^ permalink raw reply
* [PATCH net-next 6/6] bonding: remove last users of bond->lock and bond->lock itself
From: Nikolay Aleksandrov @ 2014-09-06 13:59 UTC (permalink / raw)
To: netdev; +Cc: vfalico, j.vosburgh, andy, davem, Nikolay Aleksandrov
In-Reply-To: <1410011971-24922-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 5/6] bonding: options: remove bond->lock usage
From: Nikolay Aleksandrov @ 2014-09-06 13:59 UTC (permalink / raw)
To: netdev; +Cc: vfalico, j.vosburgh, andy, davem, Nikolay Aleksandrov
In-Reply-To: <1410011971-24922-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
* [PATCH net-next 4/6] bonding: procfs: clean bond->lock usage and use RCU
From: Nikolay Aleksandrov @ 2014-09-06 13:59 UTC (permalink / raw)
To: netdev; +Cc: vfalico, j.vosburgh, andy, davem, Nikolay Aleksandrov
In-Reply-To: <1410011971-24922-1-git-send-email-nikolay@redhat.com>
Use RCU to protect against slave release, the proc show function will sync
with the bond destruction by the proc locks and the fact that the bond is
released after NETDEV_UNREGISTER which causes the bonding to remove the
proc entry.
Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
---
drivers/net/bonding/bond_procfs.c | 16 +++-------------
1 file changed, 3 insertions(+), 13 deletions(-)
diff --git a/drivers/net/bonding/bond_procfs.c b/drivers/net/bonding/bond_procfs.c
index 1a9fe1ba4c60..bb09d0442aa8 100644
--- a/drivers/net/bonding/bond_procfs.c
+++ b/drivers/net/bonding/bond_procfs.c
@@ -7,21 +7,18 @@
static void *bond_info_seq_start(struct seq_file *seq, loff_t *pos)
__acquires(RCU)
- __acquires(&bond->lock)
{
struct bonding *bond = seq->private;
struct list_head *iter;
struct slave *slave;
loff_t off = 0;
- /* make sure the bond won't be taken away */
rcu_read_lock();
- read_lock(&bond->lock);
if (*pos == 0)
return SEQ_START_TOKEN;
- bond_for_each_slave(bond, slave, iter)
+ bond_for_each_slave_rcu(bond, slave, iter)
if (++off == *pos)
return slave;
@@ -37,12 +34,9 @@ static void *bond_info_seq_next(struct seq_file *seq, void *v, loff_t *pos)
++*pos;
if (v == SEQ_START_TOKEN)
- return bond_first_slave(bond);
+ return bond_first_slave_rcu(bond);
- if (bond_is_last_slave(bond, v))
- return NULL;
-
- bond_for_each_slave(bond, slave, iter) {
+ bond_for_each_slave_rcu(bond, slave, iter) {
if (found)
return slave;
if (slave == v)
@@ -53,12 +47,8 @@ static void *bond_info_seq_next(struct seq_file *seq, void *v, loff_t *pos)
}
static void bond_info_seq_stop(struct seq_file *seq, void *v)
- __releases(&bond->lock)
__releases(RCU)
{
- struct bonding *bond = seq->private;
-
- read_unlock(&bond->lock);
rcu_read_unlock();
}
--
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