* Re: pull request: wireless 2014-09-05
From: David Miller @ 2014-09-07 23:11 UTC (permalink / raw)
To: linville; +Cc: linux-wireless, netdev, linux-kernel
In-Reply-To: <20140905150453.GB3454@tuxdriver.com>
From: "John W. Linville" <linville@tuxdriver.com>
Date: Fri, 5 Sep 2014 11:04:53 -0400
> Please pull this batch of fixes intended for the 3.17 stream...
>
> For the mac80211 bits, Johannes says:
>
> "Here are a few fixes for mac80211. One has been discussed for a while
> and adds a terminating NUL-byte to the alpha2 sent to userspace, which
> shouldn't be necessary but since many places treat it as a string we
> couldn't move to just sending two bytes.
>
> In addition to that, we have two VLAN fixes from Felix, a mesh fix, a
> fix for the recently introduced RX aggregation offload, a revert for
> a broken patch (that luckily didn't really cause any harm) and a small
> fix for alignment in debugfs."
>
> For the iwlwifi bits, Emmanuel says:
>
> "I revert a patch that disabled CTS to self in dvm because users
> reported issues. The revert is CCed to stable since the offending
> patch was sent to stable too. I also bump the firmware API versions
> since a new firmware is coming up. On top of that, Marcel fixes a
> bug I introduced while fixing a bug in our Kconfig file."
>
> Please let me know if there are problems!
Pulled, thanks John.
^ permalink raw reply
* Re: [Patch net] ipv6: restore the behavior of ipv6_sock_ac_drop()
From: David Miller @ 2014-09-07 23:10 UTC (permalink / raw)
To: xiyou.wangcong; +Cc: netdev, sd
In-Reply-To: <1409952780-24471-1-git-send-email-xiyou.wangcong@gmail.com>
From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Fri, 5 Sep 2014 14:33:00 -0700
> It is possible that the interface is already gone after joining
> the list of anycast on this interface as we don't hold a refcount
> for the device, in this case we are safe to ignore the error.
>
> What's more important, for API compatibility we should not
> change this behavior for applications even if it were correct.
>
> Fixes: commit a9ed4a2986e13011 ("ipv6: fix rtnl locking in setsockopt for anycast and multicast")
> Cc: Sabrina Dubroca <sd@queasysnail.net>
> Cc: David S. Miller <davem@davemloft.net>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH] irda: vlsi_ir: use %*ph specifier
From: David Miller @ 2014-09-07 23:09 UTC (permalink / raw)
To: sergei.shtylyov; +Cc: andriy.shevchenko, samuel, netdev
In-Reply-To: <5409F104.5040705@cogentembedded.com>
From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Date: Fri, 05 Sep 2014 21:21:08 +0400
> On 09/05/2014 06:37 PM, Andy Shevchenko wrote:
>
>> + seq_printf(seq, " data: %*ph\n", j > 20 ? 20 : j,
>
> Why not min(j, 20)?
Agreed.
^ permalink raw reply
* Re: [PATCH v2] rose: use %*ph specifier
From: David Miller @ 2014-09-07 23:07 UTC (permalink / raw)
To: andriy.shevchenko; +Cc: linux-hams, netdev, eric.dumazet
In-Reply-To: <1409931138-25447-1-git-send-email-andriy.shevchenko@linux.intel.com>
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Date: Fri, 5 Sep 2014 18:32:18 +0300
> Instead of dereference each byte let's use %*ph specifier in the printk()
> calls.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Applied.
^ permalink raw reply
* Re: [patch net-next 0/5] bridge: implement rtnl_link options for getting and setting bridge options
From: David Miller @ 2014-09-07 23:06 UTC (permalink / raw)
To: jiri; +Cc: netdev, stephen, sfeldma, arvid.brodin, sucheta.chakraborty
In-Reply-To: <1409925092-17808-1-git-send-email-jiri@resnulli.us>
From: Jiri Pirko <jiri@resnulli.us>
Date: Fri, 5 Sep 2014 15:51:27 +0200
> So far, only sysfs is complete interface for getting and setting bridge
> options. This patchset follows-up on the similar bonding code and
> allows userspace to get/set bridge master/port options using Netlink
> IFLA_INFO_DATA/IFLA_INFO_SLAVE_DATA attr.
Stephen, please review this series, thanks.
^ permalink raw reply
* Re: [PATCH net] ipv6: refresh rt6i_genid in ip6_pol_route()
From: David Miller @ 2014-09-07 22:54 UTC (permalink / raw)
To: eric.dumazet; +Cc: nicolas.dichtel, therbert, alexander.h.duyck, netdev
In-Reply-To: <1410116703.11872.55.camel@edumazet-glaptop2.roam.corp.google.com>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Sun, 07 Sep 2014 12:05:03 -0700
> From: Eric Dumazet <edumazet@google.com>
>
> While tracking IPv6 poor performance, I found that IPv6 early demux
> was broken in recent kernels. perf profiles show inet6_sk_rx_dst_set()
> being called for every incoming TCP segment :
>
> 20.95% netserver [kernel.kallsyms] [k] dst_release
> 19.33% netserver [kernel.kallsyms] [k] ip6_pol_route
> 11.75% netserver [kernel.kallsyms] [k] inet6_sk_rx_dst_set
> 3.72% netserver [kernel.kallsyms] [k] ip6_input_finish
>
> Regression came in linux-3.6 with commit 6f3118b571b8 ("ipv6: use
> net->rt_genid to check dst validity")
>
> When a route found in ip6_pol_route() is cloned (either using
> rt6_alloc_cow() or rt6_alloc_clone()), copy gets an updated rt6i_genid.
>
> But when original route is selected, we need to refresh its rt6i_genid
> that could be obsolete. If we do not refresh rt6i_genid, ip6_dst_check()
> will fail.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Fixes: 6f3118b571b8 ("ipv6: use net->rt_genid to check dst validity")
This might be broken.
We are dealing here with persistent entries in the ipv6 routine trie.
If you just bump the genid on the next person to look it up, other
sockets and cached entities might not have validated the route yet,
and now will falsely see the route as valid. We have to ensure that
they too drop this route and perform a relookup.
^ permalink raw reply
* Re: [PATCH net-next v4 2/3] net-timestamp: Make the clone operation stand-alone from phy timestamping
From: Richard Cochran @ 2014-09-07 21:54 UTC (permalink / raw)
To: Alexander Duyck; +Cc: netdev, davem, willemb
In-Reply-To: <20140904173116.7702.30877.stgit@ahduyck-bv4.jf.intel.com>
On Thu, Sep 04, 2014 at 01:31:35PM -0400, Alexander Duyck wrote:
> +struct sk_buff *skb_clone_sk(struct sk_buff *skb)
> +{
> + struct sock *sk = skb->sk;
> + struct sk_buff *clone;
> +
> + if (!sk || !atomic_inc_not_zero(&sk->sk_refcnt))
> + return NULL;
> +
> + clone = skb_clone(skb, GFP_ATOMIC);
> + if (!clone) {
> + sock_put(sk);
> + return NULL;
> + }
> +
> + clone->sk = sk;
> + clone->destructor = sock_efree;
> +
> + return clone;
> +}
> +EXPORT_SYMBOL(skb_clone_sk);
This function could use a little kerneldoc explaining its purpose and
when to use it.
Thanks,
Richard
^ permalink raw reply
* Re: [PATCH net-next v4 2/3] net-timestamp: Make the clone operation stand-alone from phy timestamping
From: Richard Cochran @ 2014-09-07 21:50 UTC (permalink / raw)
To: Alexander Duyck; +Cc: netdev, davem, willemb
In-Reply-To: <20140904173116.7702.30877.stgit@ahduyck-bv4.jf.intel.com>
Just saw this now, was away on vacation, so sorry for the delay...
On Thu, Sep 04, 2014 at 01:31:35PM -0400, Alexander Duyck wrote:
> v2: Renamed function to skb_clone_sk.
> Added destructor to call sock_put instead of doing it ourselves.
> Dropped freeing functionality from skb_complete_tx_timestamp.
...
> diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c
> index d5991ac..87648b3 100644
> --- a/drivers/net/phy/dp83640.c
> +++ b/drivers/net/phy/dp83640.c
> @@ -1148,7 +1148,7 @@ static void dp83640_remove(struct phy_device *phydev)
> kfree_skb(skb);
>
> while ((skb = skb_dequeue(&dp83640->tx_queue)) != NULL)
> - skb_complete_tx_timestamp(skb, NULL);
> + kfree_skb(skb);
The way the code was before, there was a clear usage pattern for
phy_driver.txtstamp() and skb_complete_tx_timestamp() which was also
documented in the comment to the latter.
Now, we have drivers freeing buffers allocated by the stack. I
thought it was cleaner to have the same layer allocate and free the
clone. Even if you say that this new way is just fine, still you
should correct the comment to reflect the new pattern.
Thanks,
Richard
^ permalink raw reply
* Re: [PATCH v3] 3c59x: avoid panic in boomerang_start_xmit when finding page address:
From: Neil Horman @ 2014-09-07 20:43 UTC (permalink / raw)
To: David Miller; +Cc: netdev, klassert
In-Reply-To: <20140905.172426.1277528296784403545.davem@davemloft.net>
On Fri, Sep 05, 2014 at 05:24:26PM -0700, David Miller wrote:
> From: Neil Horman <nhorman@tuxdriver.com>
> Date: Thu, 4 Sep 2014 06:13:38 -0400
>
> > This bug was reported on a very old kernel (RHEL6, 2.6.32-491.el6):
> ...
> > But the problem seems to still exist upstream. It seems on 32 bit kernels
> > page_address() can reutrn a NULL value in some circumstances, and the
> > pci_map_single api isn't prepared to handle that (on this system it results in a
> > bogus pointer deference in nommu_map_page.
> >
> > The fix is pretty easy, if we convert the 3c59x driver to use the more
> > convieient skb_frag_dma_map api we don't need to find the virtual address of the
> > page at all, and page gets mapped to the hardware properly. Verified to fix the
> > problem as described by the reporter.
> >
> > Applies to the net tree
> >
> > Change Notes:
> >
> > v2) Converted PCI_DMA_TODEVICE to DMA_TO_DEVICE. Thanks Dave!
> >
> > v3) Actually Run git commit after making changes to v2 :)
> >
> > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
>
> Applied, thanks Neil.
>
Thanks Dave!
Neil
^ 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-07 20:13 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: <20140907180737.GA5057@salvia>
On Sun, Sep 7, 2014 at 11:07 AM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>
> If the patches that provide the very first user interface don't get in
> time to this merge window round for whatever reason, we'll have the
> layout of this exposed to userspace in the next kernel version with no
> clients at all, that doesn't make sense to me.
eBPF cannot have the first user without verifier and tracing
fully reviewed, so first user cannot be in the first
patch no matter what.
In particular this patch only exposed eBPF as an _instruction set_
to user space. llvm and gcc are only two users.
llvm patches _were_ submitted to the list.
Compilers are not some fictitious users.
eBPF ISA is solid. Two backends is a proof.
For eBPF to be loaded, verifier, syscall and other pieces need to
come in gradually. So I logically split them in series:
stage I - expose instruction set
stage II - bpf syscall for maps and manpage
stage III - programs, verifier and user space testsuite
stage IV - ebpf+tracing (the first user of ebpf isa and syscall)
stage V - ebpf+sockets
stage VI - ebpf+ovs
all of the patches _were_ submitted in the past.
Split is done to make review and integration easier.
After stage III user space will be able to load eBPF programs,
but they will still be useless, because they cannot be attached
to anything. Realistically only stage IV makes first real use of
them in tracing.
You know this, yet, you're saying the first user must be
in the first series. Really, what this 'feedback' is about?
> I don't think the speed up of the llvm submission is a good argument,
> this sounds to me similar to the "please apply this patch that
> reserves this new netlink family in include/linux/netlink.h, I promise
> this new subsystem will be submitted soon though. Meanwhile this will
> speed up submission of my userspace software to distributions for
> packaging" argument.
You're not correct here. I'm not saying 'I promise it will be submitted'.
There _were_ already submitted. I split them in chunks to make
review easier and to follow standard linux philosophy of making
small decisions that can be reverted.
We still have a month until merge window, so if stages II and III
don't make it in time, Dave can revert these small patches just
as easily. You're advocating first_user_must_be_in_first_patch
approach, which is against the linux methodology.
> I think you have to find the way to send a small batch with the very
> essencial stuff that, if merged mainstream, will provide just one new
> feature while leaving the repository in consistent state. Then, send
> follow up patches that enhance your thing and that add new clients of
> it.
As I said above the _minimum_ useful program needs verifier
which is more than Dave's cutoff of 10 patches. Therefore I
split all very_essential_stuff into these stages.
Note I'm not sending radix-tree type of eBPF maps as part
of these stages, neither I send array type of eBPF maps,
though they're needed for my last stage (ebpf+ovs).
I'm not sending pointer leak detector for verifier either,
it will be needed before syscall can be exposed to unprivileged
users, etc. There is a lot of stuff that I need for ebpf+ovs that
is _not_ part of these stages. What you see is really
the minimum to make ebpf+tracing useful.
btw, all of ebpf+ovs was submitted to the list back in Sep 2013.
So there is no practical way to do first set of < 10 patches
that will be usable from user space on its own.
Even if I remove verifier and maps altogether, bpf programs
need to be attached to something like tracing and
that is again >10 patches.
These stages with small patches is only sensible approach.
^ permalink raw reply
* Re: [PATCH net-next v2 2/2] Re-check for a VIO_DESC_READY data descriptor after short udelay()
From: Raghuram Kothakota @ 2014-09-07 19:36 UTC (permalink / raw)
To: Sowmini Varadhan; +Cc: David Miller, david.stevens, netdev
In-Reply-To: <20140907181510.GA23753@oracle.com>
Sorry for joining this thread late, please see my response below:
On Sep 7, 2014, at 11:15 AM, Sowmini Varadhan <sowmini.varadhan@oracle.com> wrote:
>
>
> On (09/06/14 17:02), Sowmini Varadhan wrote:
>> Date: Sat, 6 Sep 2014 17:02:53 -0400
>> From: Sowmini Varadhan <sowmini.varadhan@oracle.com>
>> To: David Miller <davem@davemloft.net>
>> Cc: david.stevens@oracle.com, netdev@vger.kernel.org
>> Subject: Re: [PATCH net-next v2 2/2] Re-check for a VIO_DESC_READY data
>> descriptor after short udelay()
>> User-Agent: Mutt/1.5.21 (2010-09-15)
>>
>> 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;
>>
Yeah, we need the producer memory barrier before setting
the descriptor state to ready. This ensures both data in the buffers
and other fields in the descriptor are flushed before the state
is visible to the consumer. I do not believe the HV trap caused
by the trigger performs any sync of the store buffers.
>> 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())
Note, LDC message infrastructure is not a high performance infrastructure,
relying on an ldc message for each packet can greatly impact performance.
We need to reduce as many ldc messages as possible and rely on the
ring more to pick up as many packets as possible without ldc messages.
The current implementation of vnet_start_xmit() performs all operations
such as grabbing a descriptor, copying the data into the data buffers
and updating the descriptor with in the same lock. This limits the parallelism
on the transmit path especially because the data copy operation is a bigger
operation and blocks other transmitters more than it is needed. It is
possible to split these operations where multiple transmitters can grab
descriptors in sequence but perform the copy operation in parallel. When
this accomplished, there will be more possibility of racing with descriptors
getting ready quickly in a burst traffic situations. I believe the udelay()
introduced in this patch will help even more when the transmitters are highly parallel.
-Raghuram
>> So what (if any) is the outstanding question about wmb() at this
>> point?
>>
>> --Sowmini
>>
>>
>>
>>
>>
>>
>>
>>
>>
>> --
>> 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
* [PATCH net] ipv6: refresh rt6i_genid in ip6_pol_route()
From: Eric Dumazet @ 2014-09-07 19:05 UTC (permalink / raw)
To: David Miller; +Cc: Nicolas Dichtel, therbert, alexander.h.duyck, netdev
In-Reply-To: <1410027712.11872.40.camel@edumazet-glaptop2.roam.corp.google.com>
From: Eric Dumazet <edumazet@google.com>
While tracking IPv6 poor performance, I found that IPv6 early demux
was broken in recent kernels. perf profiles show inet6_sk_rx_dst_set()
being called for every incoming TCP segment :
20.95% netserver [kernel.kallsyms] [k] dst_release
19.33% netserver [kernel.kallsyms] [k] ip6_pol_route
11.75% netserver [kernel.kallsyms] [k] inet6_sk_rx_dst_set
3.72% netserver [kernel.kallsyms] [k] ip6_input_finish
Regression came in linux-3.6 with commit 6f3118b571b8 ("ipv6: use
net->rt_genid to check dst validity")
When a route found in ip6_pol_route() is cloned (either using
rt6_alloc_cow() or rt6_alloc_clone()), copy gets an updated rt6i_genid.
But when original route is selected, we need to refresh its rt6i_genid
that could be obsolete. If we do not refresh rt6i_genid, ip6_dst_check()
will fail.
Signed-off-by: Eric Dumazet <edumazet@google.com>
Fixes: 6f3118b571b8 ("ipv6: use net->rt_genid to check dst validity")
Cc: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
net/ipv6/route.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index f23fbd28a501..1e76c3c5b87b 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -944,13 +944,20 @@ restart:
dst_hold(&rt->dst);
read_unlock_bh(&table->tb6_lock);
- if (!(rt->rt6i_flags & (RTF_NONEXTHOP | RTF_GATEWAY)))
+ if (!(rt->rt6i_flags & (RTF_NONEXTHOP | RTF_GATEWAY))) {
nrt = rt6_alloc_cow(rt, &fl6->daddr, &fl6->saddr);
- else if (!(rt->dst.flags & DST_HOST))
+ } else if (!(rt->dst.flags & DST_HOST)) {
nrt = rt6_alloc_clone(rt, &fl6->daddr);
- else
- goto out2;
+ } else {
+ u32 genid = rt_genid_ipv6(net);
+ /* We must refresh rt6i_genid, but only if needed
+ * to avoid false sharing.
+ */
+ if (rt->rt6i_genid != genid)
+ rt->rt6i_genid = genid;
+ goto out2;
+ }
ip6_rt_put(rt);
rt = nrt ? : net->ipv6.ip6_null_entry;
^ permalink raw reply related
* Re: [PATCH net-next] amd-xgbe-phy: Fix build break for missing declaration
From: David Miller @ 2014-09-07 18:21 UTC (permalink / raw)
To: thomas.lendacky; +Cc: netdev
In-Reply-To: <20140907145441.7257.26465.stgit@tlendack-t1.amdoffice.net>
From: Tom Lendacky <thomas.lendacky@amd.com>
Date: Sun, 7 Sep 2014 09:54:41 -0500
> A previous patch inadvertently deleted a declaration in the
> amd_xgbe_an_tx_training function causing the build to fail.
>
> Add the declaration for 'priv' back to the function.
>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
Applied.
^ 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-07 18:07 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, Network Development, LKML
In-Reply-To: <CAMEtUuxPynQ4wdWDgCU+9_3_GRYn8oA4f-emcF+mqiQdLsqEWg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Sat, Sep 06, 2014 at 09:04:23AM -0700, Alexei Starovoitov wrote:
> On Sat, Sep 6, 2014 at 7:10 AM, Pablo Neira Ayuso <pablo-Cap9r6Oaw4JrovVCs/uTlw@public.gmane.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
If the patches that provide the very first user interface don't get in
time to this merge window round for whatever reason, we'll have the
layout of this exposed to userspace in the next kernel version with no
clients at all, that doesn't make sense to me.
I don't think the speed up of the llvm submission is a good argument,
this sounds to me similar to the "please apply this patch that
reserves this new netlink family in include/linux/netlink.h, I promise
this new subsystem will be submitted soon though. Meanwhile this will
speed up submission of my userspace software to distributions for
packaging" argument.
I think you have to find the way to send a small batch with the very
essencial stuff that, if merged mainstream, will provide just one new
feature while leaving the repository in consistent state. Then, send
follow up patches that enhance your thing and that add new clients of
it.
^ permalink raw reply
* Financing
From: info23 @ 2014-09-08 9:29 UTC (permalink / raw)
To: Recipients
We give out loan to any individual and company at 3% interest rate yearly. Our Investors also fund projects . For more information, Contact Us via Email: asp.omfinan@contractor.net
^ permalink raw reply
* Re: [PATCH net-next] netfilter: create audit records for ebtables replaces
From: Pablo Neira Ayuso @ 2014-09-07 16:07 UTC (permalink / raw)
To: Nicolas Dichtel
Cc: kaber, kadlec, stephen, davem, netfilter-devel, bridge, netdev,
linux-audit
In-Reply-To: <1409907015-6145-1-git-send-email-nicolas.dichtel@6wind.com>
On Fri, Sep 05, 2014 at 10:50:15AM +0200, Nicolas Dichtel wrote:
> This is already done for x_tables (family AF_INET and AF_INET6), let's do it
> for AF_BRIDGE also.
>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> ---
> net/bridge/netfilter/ebtables.c | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
> index 6d69631b9f4d..4ba0c5c78778 100644
> --- a/net/bridge/netfilter/ebtables.c
> +++ b/net/bridge/netfilter/ebtables.c
> @@ -26,6 +26,7 @@
> #include <asm/uaccess.h>
> #include <linux/smp.h>
> #include <linux/cpumask.h>
> +#include <linux/audit.h>
> #include <net/sock.h>
> /* needed for logical [in,out]-dev filtering */
> #include "../br_private.h"
> @@ -1126,6 +1127,20 @@ static int do_replace(struct net *net, const void __user *user,
> }
>
> ret = do_replace_finish(net, &tmp, newinfo);
> +#ifdef CONFIG_AUDIT
> + if (audit_enabled) {
> + struct audit_buffer *ab;
> +
> + ab = audit_log_start(current->audit_context, GFP_KERNEL,
> + AUDIT_NETFILTER_CFG);
> + if (ab) {
> + audit_log_format(ab, "table=%s family=%u entries=%u",
> + tmp.name, AF_BRIDGE,
> + tmp.nentries);
> + audit_log_end(ab);
> + }
> + }
> +#endif
This chunks belongs to do_replace_finish(). We still need the audit
code for compat_do_replace() and iff there are no errors when
replacing the table.
> if (ret == 0)
> return ret;
> free_entries:
> --
> 1.9.0
>
^ permalink raw reply
* [PATCH net-next v2] amd-xgbe-phy: Fix build break for missing declaration
From: Tom Lendacky @ 2014-09-07 15:33 UTC (permalink / raw)
To: netdev; +Cc: davem
A previous patch inadvertently deleted a declaration in the
amd_xgbe_an_tx_training function causing the build to fail.
Add the declaration for 'priv' back to the function.
Reported-by: kbuild test robot <fengguang.wu@intel.com>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
Changes in v2:
- Add "Reported-by" tag to the changelog
drivers/net/phy/amd-xgbe-phy.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/phy/amd-xgbe-phy.c b/drivers/net/phy/amd-xgbe-phy.c
index 4e1f7f7..c456559 100644
--- a/drivers/net/phy/amd-xgbe-phy.c
+++ b/drivers/net/phy/amd-xgbe-phy.c
@@ -638,6 +638,7 @@ static int amd_xgbe_phy_set_mode(struct phy_device *phydev,
static enum amd_xgbe_phy_an amd_xgbe_an_tx_training(struct phy_device *phydev,
enum amd_xgbe_phy_rx *state)
{
+ struct amd_xgbe_phy_priv *priv = phydev->priv;
int ad_reg, lp_reg, ret;
*state = AMD_XGBE_RX_COMPLETE;
^ permalink raw reply related
* [PATCH net-next] amd-xgbe-phy: Fix build break for missing declaration
From: Tom Lendacky @ 2014-09-07 14:54 UTC (permalink / raw)
To: netdev; +Cc: davem
A previous patch inadvertently deleted a declaration in the
amd_xgbe_an_tx_training function causing the build to fail.
Add the declaration for 'priv' back to the function.
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
drivers/net/phy/amd-xgbe-phy.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/phy/amd-xgbe-phy.c b/drivers/net/phy/amd-xgbe-phy.c
index 4e1f7f7..c456559 100644
--- a/drivers/net/phy/amd-xgbe-phy.c
+++ b/drivers/net/phy/amd-xgbe-phy.c
@@ -638,6 +638,7 @@ static int amd_xgbe_phy_set_mode(struct phy_device *phydev,
static enum amd_xgbe_phy_an amd_xgbe_an_tx_training(struct phy_device *phydev,
enum amd_xgbe_phy_rx *state)
{
+ struct amd_xgbe_phy_priv *priv = phydev->priv;
int ad_reg, lp_reg, ret;
*state = AMD_XGBE_RX_COMPLETE;
^ permalink raw reply related
* Re: [PATCH net-next v1 2/2] bonding: Simplify the xmit function for modes that use xmit_hash
From: Nikolay Aleksandrov @ 2014-09-07 10:36 UTC (permalink / raw)
To: Mahesh Bandewar
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 09/07/2014 07:33 AM, Mahesh Bandewar 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.
>>>
<<<<<snip>>>>>
>>> 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!
>
I don't see any additional protection you'd get with RCU here, and for a
writer it's definitely useless.
>>> + 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.
>
Ah, okay now it makes sense, but then you should probably add a comment
about that peculiarity and also lockdep_rtnl_is_held().
>>> + 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?
>
No, the xmit code was converted to RCU some time ago and runs in parallel
with these operations. I've actually hit this bug with bond->slave_cnt
before. You should probably edit the xmit code that uses ->count and make
sure to fetch it only once.
>
>>> + 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: [PATCH][net-next] openvswitch: change the data type of error status to atomic_long_t
From: Li RongQing @ 2014-09-07 9:24 UTC (permalink / raw)
To: Pravin Shelar; +Cc: netdev
In-Reply-To: <CALnjE+o121kh+hEtEsSLJqBw68dy=epEsXGVDN5wv+4g1xph+g@mail.gmail.com>
On Sun, Sep 7, 2014 at 11:44 AM, Pravin Shelar <pshelar@nicira.com> wrote:
>> The operation of atomic maybe faster than spin lock.
>
> What is reason for this change?
1. The operation of atomic maybe faster than spin lock
2. I did not find that tx_dropped/tx_error/.. is protected by spin
lock under net dir,
sometime tx_dropped is atomic_long_t; sometime it is percpu variable;
sometime it is
u64,but does not need to protect.
-Roy
^ permalink raw reply
* Re: [PATCH 1/3] virtio_net: pass well-formed sgs to virtqueue_add_*()
From: Michael S. Tsirkin @ 2014-09-07 7:20 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: netdev, Andy Lutomirski, virtualization
In-Reply-To: <54099332.7060909@redhat.com>
On Fri, Sep 05, 2014 at 12:40:50PM +0200, Paolo Bonzini wrote:
> Il 03/09/2014 06:29, Rusty Russell ha scritto:
> > + sg_init_table(rq->sg, MAX_SKB_FRAGS + 2);
>
> I think 2 is enough here. That said...
>
> > sg_set_buf(rq->sg, &hdr->hdr, sizeof hdr->hdr);
> > -
> > skb_to_sgvec(skb, rq->sg + 1, 0, skb->len);
> >
> > err = virtqueue_add_inbuf(rq->vq, rq->sg, 2, skb, gfp);
>
> ... skb_to_sgvec will already make the sg well formed, so the
> sg_init_table is _almost_ redundant; it is only there to remove
> intermediate end marks. The block layer takes care to remove
> them, but skb_to_sgvec doesn't.
>
> If the following patch can be accepted to net/core/skbuff.c, the
> sg_init_table in virtnet_alloc_queues will suffice.
>
> Paolo
You will have to post it to netdev as a new topic and Cc
Dave Miller for it to be considered.
> -------------------- 8< -------------------
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH] net: skb_to_sgvec: do not leave intermediate marks in the sgvec
>
> sg_set_buf/sg_set_page will leave the end mark in place in their
> argument, which may be in the middle of a scatterlist. If we
> remove the mark before calling them, we can avoid calls to
> sg_init_table before skb_to_sgvec.
>
> However, users of skb_to_sgvec_nomark now need to be careful and
> possibly restore the mark.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 163b673f9e62..a3108ef1f1c0 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -3265,6 +3265,7 @@ __skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len)
> if (copy > 0) {
> if (copy > len)
> copy = len;
> + sg_unmark_end(sg);
> sg_set_buf(sg, skb->data + offset, copy);
> elt++;
> if ((len -= copy) == 0)
> @@ -3283,6 +3284,7 @@ __skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len)
>
> if (copy > len)
> copy = len;
> + sg_unmark_end(&sg[elt]);
> sg_set_page(&sg[elt], skb_frag_page(frag), copy,
> frag->page_offset+offset-start);
> elt++;
> @@ -3322,7 +3324,7 @@ __skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len)
> * Scenario to use skb_to_sgvec_nomark:
> * 1. sg_init_table
> * 2. skb_to_sgvec_nomark(payload1)
> - * 3. skb_to_sgvec_nomark(payload2)
> + * 3. skb_to_sgvec(payload2)
> *
> * This is equivalent to:
> * 1. sg_init_table
> diff --git a/net/ipv4/ah4.c b/net/ipv4/ah4.c
> index a2afa89513a0..9ae5756d9e5f 100644
> --- a/net/ipv4/ah4.c
> +++ b/net/ipv4/ah4.c
> @@ -227,6 +227,7 @@ static int ah_output(struct xfrm_state *x, struct sk_buff *skb)
> *seqhi = htonl(XFRM_SKB_CB(skb)->seq.output.hi);
> sg_set_buf(seqhisg, seqhi, seqhi_len);
> }
> + sg_mark_end(&sg[nfrags + sglists]);
> ahash_request_set_crypt(req, sg, icv, skb->len + seqhi_len);
> ahash_request_set_callback(req, 0, ah_output_done, skb);
>
> @@ -395,6 +396,7 @@ static int ah_input(struct xfrm_state *x, struct sk_buff *skb)
> *seqhi = XFRM_SKB_CB(skb)->seq.input.hi;
> sg_set_buf(seqhisg, seqhi, seqhi_len);
> }
> + sg_mark_end(&sg[nfrags + sglists]);
> ahash_request_set_crypt(req, sg, icv, skb->len + seqhi_len);
> ahash_request_set_callback(req, 0, ah_input_done, skb);
>
> diff --git a/net/ipv6/ah6.c b/net/ipv6/ah6.c
> index 72a4930bdc0a..c680d82e43de 100644
> --- a/net/ipv6/ah6.c
> +++ b/net/ipv6/ah6.c
> @@ -430,6 +430,8 @@ static int ah6_output(struct xfrm_state *x, struct sk_buff *skb)
> *seqhi = htonl(XFRM_SKB_CB(skb)->seq.output.hi);
> sg_set_buf(seqhisg, seqhi, seqhi_len);
> }
> + sg_mark_end(&sg[nfrags + sglists]);
> +
> ahash_request_set_crypt(req, sg, icv, skb->len + seqhi_len);
> ahash_request_set_callback(req, 0, ah6_output_done, skb);
>
> @@ -608,6 +610,7 @@ static int ah6_input(struct xfrm_state *x, struct sk_buff *skb)
> *seqhi = XFRM_SKB_CB(skb)->seq.input.hi;
> sg_set_buf(seqhisg, seqhi, seqhi_len);
> }
> + sg_mark_end(&sg[nfrags + sglists]);
>
> ahash_request_set_crypt(req, sg, icv, skb->len + seqhi_len);
> ahash_request_set_callback(req, 0, ah6_input_done, skb);
^ 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-07 5:33 UTC (permalink / raw)
To: Nikolay Aleksandrov
Cc: Jay Vosburgh, Veaceslav Falico, Andy Gospodarek, David Miller,
netdev, Eric Dumazet, Maciej Zenczykowski
In-Reply-To: <540AE9CC.10206@redhat.com>
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.
>> 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
* WINNING NOTIFICATION
From: Kathleen Aubin @ 2014-09-07 3:13 UTC (permalink / raw)
You have been awarded a 750,000.00USD by BMW Automobile Company in Malaysia. Contact Mr. Abdul Karan, with your Fullname: Country: Address: Telephone no: via Email: abdulkaran_bmwclaims1@shqiptar.eu
^ permalink raw reply
* Re: [PATCH][net-next] openvswitch: change the data type of error status to atomic_long_t
From: Pravin Shelar @ 2014-09-07 3:44 UTC (permalink / raw)
To: roy.qing.li; +Cc: netdev
In-Reply-To: <1410001571-13338-1-git-send-email-roy.qing.li@gmail.com>
On Sat, Sep 6, 2014 at 4:06 AM, <roy.qing.li@gmail.com> wrote:
> From: Li RongQing <roy.qing.li@gmail.com>
>
> Change the date type of error status from u64 to atomic_long_t, and use atomic
> operation, then remove the lock which is used to protect the error status.
>
> The operation of atomic maybe faster than spin lock.
What is reason for this change?
>
> Cc: Pravin Shelar <pshelar@nicira.com>
> Signed-off-by: Li RongQing <roy.qing.li@gmail.com>
> ---
> net/openvswitch/vport.c | 25 ++++++++-----------------
> net/openvswitch/vport.h | 10 ++++------
> 2 files changed, 12 insertions(+), 23 deletions(-)
>
> diff --git a/net/openvswitch/vport.c b/net/openvswitch/vport.c
> index 6d8f2ec..f7e63f9 100644
> --- a/net/openvswitch/vport.c
> +++ b/net/openvswitch/vport.c
> @@ -148,8 +148,6 @@ struct vport *ovs_vport_alloc(int priv_size, const struct vport_ops *ops,
> return ERR_PTR(-ENOMEM);
> }
>
> - spin_lock_init(&vport->stats_lock);
> -
> return vport;
> }
>
> @@ -268,14 +266,10 @@ void ovs_vport_get_stats(struct vport *vport, struct ovs_vport_stats *stats)
> * netdev-stats can be directly read over netlink-ioctl.
> */
>
> - spin_lock_bh(&vport->stats_lock);
> -
> - stats->rx_errors = vport->err_stats.rx_errors;
> - stats->tx_errors = vport->err_stats.tx_errors;
> - stats->tx_dropped = vport->err_stats.tx_dropped;
> - stats->rx_dropped = vport->err_stats.rx_dropped;
> -
> - spin_unlock_bh(&vport->stats_lock);
> + stats->rx_errors = atomic_long_read(&vport->err_stats.rx_errors);
> + stats->tx_errors = atomic_long_read(&vport->err_stats.tx_errors);
> + stats->tx_dropped = atomic_long_read(&vport->err_stats.tx_dropped);
> + stats->rx_dropped = atomic_long_read(&vport->err_stats.rx_dropped);
>
> for_each_possible_cpu(i) {
> const struct pcpu_sw_netstats *percpu_stats;
> @@ -495,27 +489,24 @@ int ovs_vport_send(struct vport *vport, struct sk_buff *skb)
> static void ovs_vport_record_error(struct vport *vport,
> enum vport_err_type err_type)
> {
> - spin_lock(&vport->stats_lock);
> -
> switch (err_type) {
> case VPORT_E_RX_DROPPED:
> - vport->err_stats.rx_dropped++;
> + atomic_long_inc(&vport->err_stats.rx_dropped);
> break;
>
> case VPORT_E_RX_ERROR:
> - vport->err_stats.rx_errors++;
> + atomic_long_inc(&vport->err_stats.rx_errors);
> break;
>
> case VPORT_E_TX_DROPPED:
> - vport->err_stats.tx_dropped++;
> + atomic_long_inc(&vport->err_stats.tx_dropped);
> break;
>
> case VPORT_E_TX_ERROR:
> - vport->err_stats.tx_errors++;
> + atomic_long_inc(&vport->err_stats.tx_errors);
> break;
> }
>
> - spin_unlock(&vport->stats_lock);
> }
>
> static void free_vport_rcu(struct rcu_head *rcu)
> diff --git a/net/openvswitch/vport.h b/net/openvswitch/vport.h
> index 35f89d8..0d95b9f 100644
> --- a/net/openvswitch/vport.h
> +++ b/net/openvswitch/vport.h
> @@ -62,10 +62,10 @@ int ovs_vport_send(struct vport *, struct sk_buff *);
> /* The following definitions are for implementers of vport devices: */
>
> struct vport_err_stats {
> - u64 rx_dropped;
> - u64 rx_errors;
> - u64 tx_dropped;
> - u64 tx_errors;
> + atomic_long_t rx_dropped;
> + atomic_long_t rx_errors;
> + atomic_long_t tx_dropped;
> + atomic_long_t tx_errors;
> };
> /**
> * struct vport_portids - array of netlink portids of a vport.
> @@ -93,7 +93,6 @@ struct vport_portids {
> * @dp_hash_node: Element in @datapath->ports hash table in datapath.c.
> * @ops: Class structure.
> * @percpu_stats: Points to per-CPU statistics used and maintained by vport
> - * @stats_lock: Protects @err_stats;
> * @err_stats: Points to error statistics used and maintained by vport
> */
> struct vport {
> @@ -108,7 +107,6 @@ struct vport {
>
> struct pcpu_sw_netstats __percpu *percpu_stats;
>
> - spinlock_t stats_lock;
> struct vport_err_stats err_stats;
> };
>
> --
> 1.7.10.4
>
^ permalink raw reply
* 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
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