* Re: [PATCH] netfilter: nf_conntrack: fix RCU race in nf_conntrack_find_get
From: Eric Dumazet @ 2014-01-08 13:42 UTC (permalink / raw)
To: Florian Westphal
Cc: Andrey Vagin, netfilter-devel, netfilter, coreteam, netdev,
linux-kernel, vvs, Pablo Neira Ayuso, Patrick McHardy,
Jozsef Kadlecsik, David S. Miller, Cyrill Gorcunov
In-Reply-To: <20140107152520.GF9894@breakpoint.cc>
On Tue, 2014-01-07 at 16:25 +0100, Florian Westphal wrote:
> Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > > diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
> > > index 43549eb..7a34bb2 100644
> > > --- a/net/netfilter/nf_conntrack_core.c
> > > +++ b/net/netfilter/nf_conntrack_core.c
> > > @@ -387,8 +387,12 @@ begin:
> > > !atomic_inc_not_zero(&ct->ct_general.use)))
> > > h = NULL;
> > > else {
> > > + /* A conntrack can be recreated with the equal tuple,
> > > + * so we need to check that the conntrack is initialized
> > > + */
> > > if (unlikely(!nf_ct_tuple_equal(tuple, &h->tuple) ||
> > > - nf_ct_zone(ct) != zone)) {
> > > + nf_ct_zone(ct) != zone) ||
> > > + !nf_ct_is_confirmed(ct)) {
> > > nf_ct_put(ct);
> > > goto begin;
> > > }
> >
> > I do not think this is the right way to fix this problem (if said
> > problem is confirmed)
> >
> > Remember the rule about SLAB_DESTROY_BY_RCU :
> >
> > When a struct is freed, then reused, its important to set the its refcnt
> > (from 0 to 1) only when the structure is fully ready for use.
> >
> > If a lookup finds a structure which is not yet setup, the
> > atomic_inc_not_zero() will fail.
>
> Indeed. But, the structure itself might be ready (or rather,
> can be ready since the allocation side will set the refcount to one
> after doing the initial work, such as zapping old ->status flags and
> setting tuple information).
>
> The problem is with nat extension area stored in the ct->ext area.
> This extension area is preallocated but the snat/dnat action
> information is only set up after the ct (or rather, the skb that grabbed
> a reference to the nf_conn entry) traverses nat pre/postrouting.
>
> This will also set up a null-binding when no matching SNAT/DNAT/MASQERUADE
> rule existed.
>
> The manipulations of the skb->nfct->ext nat area are performed without
> a lock. Concurrent access is supposedly impossible as the conntrack
> should not (yet) be in the hash table.
>
> The confirmed bit is set right before we insert the conntrack into
> the hash table (after we traversed rules, ct is ready to be
> 'published').
>
> i.e. when the confirmed bit is NOT set we should not be 'seeing' the nf_conn
> struct when we perform the lookup, as it should still be sitting on the
> 'unconfirmed' list, being invisible to readers.
>
> Does that explanation make sense to you?
>
> Thanks for looking into this.
Still, this patch adds a loop. And maybe an infinite one if confirmed
bit is set from an context that was interrupted by this one.
If you need to test the confirmed bit, then you also need to test it
before taking the refcount.
^ permalink raw reply
* Re: IPv6: Bug in net-next
From: François-Xavier Le Bail @ 2014-01-08 13:35 UTC (permalink / raw)
To: netdev; +Cc: davem
In-Reply-To: <1389187126.20074.YahooMailBasic@web125504.mail.ne1.yahoo.com>
On Wed, 1/8/14, François-Xavier Le Bail <fx.lebail@yahoo.com> wrote:
> > I think there is a bug in actual net-next.
> > When I execute this code and press Crtl-C, all the IPv6
> > Link-Layer addresses are deleted.
> > This happened between
> > c1ddf295f5183a5189196a8035546842caa2055a and HEAD.
> > [...]
> Sorry, it is not linked to this code.
> I'm looking for the reason ...
It seems that the valid lifetime and preferred lft are set to 0 sec for autoconfigured LLA.
BR,
Francois-Xavier
^ permalink raw reply
* Re: IPv6: Bug in net-next
From: François-Xavier Le Bail @ 2014-01-08 13:18 UTC (permalink / raw)
To: netdev; +Cc: davem
In-Reply-To: <1389186135.88856.YahooMailBasic@web125506.mail.ne1.yahoo.com>
On Wed, 1/8/14, François-Xavier Le Bail <fx.lebail@yahoo.com> wrote:
> Hello,
> I think there is a bug in actual net-next.
> When I execute this code and press Crtl-C, all the IPv6
> Link-Layer addresses are deleted.
> This happened between
> c1ddf295f5183a5189196a8035546842caa2055a and HEAD.
> #include <sys/types.h>
> #include <sys/socket.h>
> #include <netinet/in.h>
> #include <net/if.h>
>
> int main (int argc, char **argv) {
> struct ipv6_mreq mreq = { 0 };
> int sockfd1 = socket (AF_INET6, SOCK_DGRAM, 0);
>
> inet_pton (AF_INET6, "2a01:999::1", &mreq.ipv6mr_multiaddr);
> mreq.ipv6mr_interface = if_nametoindex ("dummy0");
> setsockopt (sockfd1, IPPROTO_IPV6, IPV6_JOIN_ANYCAST, &mreq, sizeof (mreq));
>
> pause ();
> }
Sorry, it is not linked to this code.
I'm looking for the reason ...
BR,
Francois-Xavier
^ permalink raw reply
* [PATCH] netfilter: nf_conntrack: fix RCU race in nf_conntrack_find_get (v2)
From: Andrey Vagin @ 2014-01-08 13:17 UTC (permalink / raw)
To: netfilter-devel
Cc: netfilter, coreteam, netdev, linux-kernel, vvs, Andrey Vagin,
Florian Westphal, Pablo Neira Ayuso, Patrick McHardy,
Jozsef Kadlecsik, David S. Miller, Cyrill Gorcunov
In-Reply-To: <1389090711-15843-1-git-send-email-avagin@openvz.org>
Lets look at destroy_conntrack:
hlist_nulls_del_rcu(&ct->tuplehash[IP_CT_DIR_ORIGINAL].hnnode);
...
nf_conntrack_free(ct)
kmem_cache_free(net->ct.nf_conntrack_cachep, ct);
net->ct.nf_conntrack_cachep is created with SLAB_DESTROY_BY_RCU.
The hash is protected by rcu, so readers look up conntracks without
locks.
A conntrack is removed from the hash, but in this moment a few readers
still can use the conntrack. Then this conntrack is released and another
thread creates conntrack with the same address and the equal tuple.
After this a reader starts to validate the conntrack:
* It's not dying, because a new conntrack was created
* nf_ct_tuple_equal() returns true.
But this conntrack is not initialized yet, so it can not be used by two
threads concurrently. In this case BUG_ON may be triggered from
nf_nat_setup_info().
Florian Westphal suggested to check the confirm bit too. I think it's
right.
task 1 task 2 task 3
nf_conntrack_find_get
____nf_conntrack_find
destroy_conntrack
hlist_nulls_del_rcu
nf_conntrack_free
kmem_cache_free
__nf_conntrack_alloc
kmem_cache_alloc
memset(&ct->tuplehash[IP_CT_DIR_MAX],
if (nf_ct_is_dying(ct))
if (!nf_ct_tuple_equal()
I'm not sure, that I have ever seen this race condition in a real life.
Currently we are investigating a bug, which is reproduced on a few node.
In our case one conntrack is initialized from a few tasks concurrently,
we don't have any other explanation for this.
<2>[46267.083061] kernel BUG at net/ipv4/netfilter/nf_nat_core.c:322!
...
<4>[46267.083951] RIP: 0010:[<ffffffffa01e00a4>] [<ffffffffa01e00a4>] nf_nat_setup_info+0x564/0x590 [nf_nat]
...
<4>[46267.085549] Call Trace:
<4>[46267.085622] [<ffffffffa023421b>] alloc_null_binding+0x5b/0xa0 [iptable_nat]
<4>[46267.085697] [<ffffffffa02342bc>] nf_nat_rule_find+0x5c/0x80 [iptable_nat]
<4>[46267.085770] [<ffffffffa0234521>] nf_nat_fn+0x111/0x260 [iptable_nat]
<4>[46267.085843] [<ffffffffa0234798>] nf_nat_out+0x48/0xd0 [iptable_nat]
<4>[46267.085919] [<ffffffff814841b9>] nf_iterate+0x69/0xb0
<4>[46267.085991] [<ffffffff81494e70>] ? ip_finish_output+0x0/0x2f0
<4>[46267.086063] [<ffffffff81484374>] nf_hook_slow+0x74/0x110
<4>[46267.086133] [<ffffffff81494e70>] ? ip_finish_output+0x0/0x2f0
<4>[46267.086207] [<ffffffff814b5890>] ? dst_output+0x0/0x20
<4>[46267.086277] [<ffffffff81495204>] ip_output+0xa4/0xc0
<4>[46267.086346] [<ffffffff814b65a4>] raw_sendmsg+0x8b4/0x910
<4>[46267.086419] [<ffffffff814c10fa>] inet_sendmsg+0x4a/0xb0
<4>[46267.086491] [<ffffffff814459aa>] ? sock_update_classid+0x3a/0x50
<4>[46267.086562] [<ffffffff81444d67>] sock_sendmsg+0x117/0x140
<4>[46267.086638] [<ffffffff8151997b>] ? _spin_unlock_bh+0x1b/0x20
<4>[46267.086712] [<ffffffff8109d370>] ? autoremove_wake_function+0x0/0x40
<4>[46267.086785] [<ffffffff81495e80>] ? do_ip_setsockopt+0x90/0xd80
<4>[46267.086858] [<ffffffff8100be0e>] ? call_function_interrupt+0xe/0x20
<4>[46267.086936] [<ffffffff8118cb10>] ? ub_slab_ptr+0x20/0x90
<4>[46267.087006] [<ffffffff8118cb10>] ? ub_slab_ptr+0x20/0x90
<4>[46267.087081] [<ffffffff8118f2e8>] ? kmem_cache_alloc+0xd8/0x1e0
<4>[46267.087151] [<ffffffff81445599>] sys_sendto+0x139/0x190
<4>[46267.087229] [<ffffffff81448c0d>] ? sock_setsockopt+0x16d/0x6f0
<4>[46267.087303] [<ffffffff810efa47>] ? audit_syscall_entry+0x1d7/0x200
<4>[46267.087378] [<ffffffff810ef795>] ? __audit_syscall_exit+0x265/0x290
<4>[46267.087454] [<ffffffff81474885>] ? compat_sys_setsockopt+0x75/0x210
<4>[46267.087531] [<ffffffff81474b5f>] compat_sys_socketcall+0x13f/0x210
<4>[46267.087607] [<ffffffff8104dea3>] ia32_sysret+0x0/0x5
<4>[46267.087676] Code: 91 20 e2 01 75 29 48 89 de 4c 89 f7 e8 56 fa ff ff 85 c0 0f 84 68 fc ff ff 0f b6 4d c6 41 8b 45 00 e9 4d fb ff ff e8 7c 19 e9 e0 <0f> 0b eb fe f6 05 17 91 20 e2 80 74 ce 80 3d 5f 2e 00 00 00 74
<1>[46267.088023] RIP [<ffffffffa01e00a4>] nf_nat_setup_info+0x564/0x590
v2: move nf_ct_is_confirmed into the unlikely() annotation
Cc: Florian Westphal <fw@strlen.de>
Cc: Pablo Neira Ayuso <pablo@netfilter.org>
Cc: Patrick McHardy <kaber@trash.net>
Cc: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Cyrill Gorcunov <gorcunov@openvz.org>
Signed-off-by: Andrey Vagin <avagin@openvz.org>
---
net/netfilter/nf_conntrack_core.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 43549eb..403f634 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -387,8 +387,12 @@ begin:
!atomic_inc_not_zero(&ct->ct_general.use)))
h = NULL;
else {
+ /* A conntrack can be recreated with the equal tuple,
+ * so we need to check that the conntrack is initialized
+ */
if (unlikely(!nf_ct_tuple_equal(tuple, &h->tuple) ||
- nf_ct_zone(ct) != zone)) {
+ nf_ct_zone(ct) != zone ||
+ !nf_ct_is_confirmed(ct))) {
nf_ct_put(ct);
goto begin;
}
--
1.8.4.2
^ permalink raw reply related
* IPv6: Bug in net-next
From: François-Xavier Le Bail @ 2014-01-08 13:02 UTC (permalink / raw)
To: netdev; +Cc: davem
Hello,
I think there is a bug in actual net-next.
When I execute this code and press Crtl-C, all the IPv6 Link-Layer addresses are deleted.
This happened between c1ddf295f5183a5189196a8035546842caa2055a and HEAD.
#include <sys/types.h>
#include <sys/socket.h>
#include <netinet/in.h>
#include <net/if.h>
int main (int argc, char **argv)
{
struct ipv6_mreq mreq = { 0 };
int sockfd1 = socket (AF_INET6, SOCK_DGRAM, 0);
inet_pton (AF_INET6, "2a01:999::1", &mreq.ipv6mr_multiaddr);
mreq.ipv6mr_interface = if_nametoindex ("dummy0");
setsockopt (sockfd1, IPPROTO_IPV6, IPV6_JOIN_ANYCAST, &mreq, sizeof (mreq));
pause ();
}
BR,
Francois-Xavier
^ permalink raw reply
* Re: [PATCH net 1/2] macvlan: forbid L2 fowarding offload for macvtap
From: Michael S. Tsirkin @ 2014-01-08 12:55 UTC (permalink / raw)
To: Jason Wang
Cc: John Fastabend, John Fastabend, Neil Horman, davem, netdev,
linux-kernel, Vlad Yasevich
In-Reply-To: <52CBC22D.3050002@redhat.com>
On Tue, Jan 07, 2014 at 05:00:29PM +0800, Jason Wang wrote:
> On 01/07/2014 03:26 PM, John Fastabend wrote:
> > [...]
> >
> >>>> Unfortunately not. This commit has a side effect that it in fact
> >>>> disables the multiqueue macvtap transmission. Since all macvtap queues
> >>>> will contend on a single qdisc lock.
> >>>>
> >>>
> >>> They will only contend on a single qdisc lock if the lower device has
> >>> 1 queue.
> >>
> >> I think we are talking about 6acf54f1cf0a6747bac9fea26f34cfc5a9029523.
> >
> > Yes.
> >
> >>
> >> The qdisc or txq lock were macvlan device itself since dev_queue_xmit()
> >> was called for macvlan device itself. So even if lower device has
> >> multiple txqs, if you just create a one queue macvlan device, you will
> >> get lock contention on macvlan device. And even if you explicitly
> >> specifying the txq numbers ( though I don't believe most management
> >> software will do this) when creating the macvlan/macvtap device, you
> >> must also configure the XPS for macvlan to make sure it has the
> >> possibility of using multiple transmit queues.
> >>
> >
> > OK I think I'm finally putting all the pieces together thanks.
> >
> > Do you know why macvtap is setting dev->tx_queue_len by default? If you
> > zero this then the noqueue_qdisc is used and the q->enqueue check in
> > dev_queue_xmit will fail.
>
> It was introduced in commit 8a35747a5d13b99e076b0222729e0caa48cb69b6
> ("macvtap: Limit packet queue length") to limit the length of socket
> receive queue of macvtap. But I'm not sure whether the qdisc is a
> byproduct of this commit, maybe we can switch to use another name
> instead of just reuse dev->tx_queue_length.
You mean tx_queue_len really, right?
Problem is tx_queue_len can be accessed using netlink sysfs or ioctl,
so if someone uses these to control or check the # of packets that
can be queued by device, this will break.
How about adding ndo_set_tx_queue_len then?
At some point we wanted to decouple queue length from tx_queue_length
for tun as well, so that would be benefitial there as well.
> >
> > Also if XPS is not configured then skb_tx_hash is used so multiple
> > transmit queues will still be used.
> >
>
> True.
> >>> Perhaps defaulting the L2 forwarding devices to 1queue was a
> >>> mistake. But the same issue arises when running macvtap over a
> >>> non-multiqueue nic. Or even if you have a multiqueue device and create
> >>> many more macvtap queues than the lower device has queues.
> >>>
> >>> Shouldn't the macvtap configuration take into account the lowest level
> >>> devices queues?
> >>
> >> See commit 8ffab51b3dfc54876f145f15b351c41f3f703195 ("macvlan: lockless
> >> tx path"). It allows the management to create a device without worrying
> >> the underlying device.
> >
> > OK.
> >
> >>> How does using the L2 forwarding device change the
> >>> contention issues? Without the L2 forwarding LLTX is enabled but the
> >>> qdisc lock, etc is still acquired on the device below the macvlan.
> >>>
> >>
> >> That's the point. We need make sure the txq selection and qdisc lock
> >> were done for the lower device not for the macvlan device itself. Then
> >> macvlan can automatically benefit from the multi-queue capable lower
> >> devices. But L2 forwarding needs to contend on the txq lock on macvlan
> >> device itself, which is unnecessary and can complex the management.
> >
> > If I make the l2 forwarding defaults a bit better then using the L2
> > forwarding case should not be any more complex. And because the queues
> > are dedicated to the macvtap device any contention from qdisc lock, etc
> > comes from the upper device only.
>
> At very least the txq of lower device should be held in order to be
> synchronized with management path. Consider txq lock were often held by
> netif_tx_disable() before trying to down the card. Current cold does not
> hold txq lock, so it loses the synchronization which may cause issues.
> And the code also does not check whether the txq has been stopped before
> trying to start the transmission.
>
>
> > Also if I get the bandwidth controls
> > in we can set the max/min bandwidth per macvtap device this way. That
> > is future work though.
> >
>
> That will be a nice feature.
> >>> The ixgbe driver as it is currently written can be configured for up to
> >>> 4 queues by setting numtxqueues when the device is created. I assume
> >>> when creating macvtap queues the user needs to account for the number
> >>> of queues supported by the lower device.
> >>>
> >>
> >> We'd better not complicate the task of management, lockless tx path work
> >> very well so we can just keep it. Btw, there's no way for the user to
> >> know the maximum number of queues that L2 forwarding supports.
> >
> > Good point I'll add an attribute to query it.
> >
> >>>> For L2 forwarding offload itself, more issues need to be addressed for
> >>>> multiqueue macvtap:
> >>>>
> >>>> - ndo_dfwd_add_station() can only create queues per device at
> >>>> ndo_open,
> >>>> but multiqueue macvtap allows user to create and destroy queues at
> >>>> their
> >>>> will and at any time.
> >>>
> >>> same argument as above, isn't this the same when running macvtap
> >>> without
> >>> the l2 offloads over a real device? I expect you hit the same
> >>> contention
> >>> points when running over a real device.
> >>
> >> Not true and not only for contention.
> >>
> >> Macvtap allows user to create or destroy a queue by simply open or close
> >> to character device /dev/tapX. But currently, we do nothing when a new
> >> queue was created or destroyed for L2 forwarding offload.
> >>
> >> For contention, lockless tx path make the contention only happens for
> >> the txq or qdisc for the lower device, but L2 forwarding offload make
> >> contention also happen for the macvlan device itself.
> >
> > Right, but there will be less contention there because those queues
> > are a dedicated resource for the upper device.
>
> Yes and this is also true if we only do synchronization on the lower
> device since only dedicated queues could be selected.
> >
> > At this point I think I need to put together a real testbed and
> > benchmark some of this with netperf and perf running to get real
> > numbers. When I originally did the l2 forwarding I did not do any
> > testing with multiple macvtap queues and only very limited work with
> > macvtap.
> >
>
> As I said above, holding the txq lock of lower device seems a must and
> we should not get regression if NETIF_F_LLTX is kept. But I agree we
> need some test.
> >>
> >>>
> >>>
> >>>> - it looks that ixgbe has a upper limit of 4 queues per station, but
> >>>> macvtap currently allows up to 16 queues per device.
> >>>>
> >>>
> >>> The 4 limit was to simplify the code because the queue mapping in the
> >>> driver gets complicated if it is greater than 4. We can probably
> >>> increase this latter. But sorry reiterating how is this different than
> >>> a macvtap on a real device that supports a max of 4 queues?
> >>
> >> Well, it maybe easy. I just point out possible issues we may meet
> >> currently.
> >
> > Right.
> >
> >>>
> >>>> So more works need to be done and unless those above 3 issues were
> >>>> addressed, this patch is really needed to make sure macvtap works.
> >>>>
> >>>
> >>> Agreed there is a lot more work here to improve things I'm just not
> >>> sure we need to disable this now. Also note its the l2 forwarding
> >>> should be disabled by default so a user would have to enable the
> >>> feature flag.
> >>
> >> Even if it was disabled by default. We should not surprise the user who
> >> want to enable it for macvtap.
> >
> > So the question is what to do in net while we improve net-next. Either
> > we fix the crash from the null txq and note that with l2 forwarding
> > some non default configuration is needed for optimal performance OR
> > for now disable it as your patch does. I would prefer to fix the crash
> > and note the configuration but I see your point about surprising users
> > so could go either way.
> >
>
> It's much safer to disable l2 forwarding offload for macvtap temporarily
> consider it has several issues.We can re-enable it when everything is
> ready in net-next. We we really need to hold the txq lock of lower
> device, only add more check of NULL pointer is not sufficient. So
> explicitly select a txq is still needed. And I don't see any conflicts
> between this and future enhancement.
>
> Also I don't see any drawback of using NETIF_F_LLTX for l2 forwarding.
> So we'd better keep it.
> > Neil any thoughts?
> >
> > To fix the null txq in the gso case adding a check for a non-null
> > txq before calling txq_trans_update() makes sense to me. We already
> > have the check in the non-gso case so making it symmetric fixes it.
> >
> >>>
> >>> Thanks,
> >>> John
> >>>
> >>
> >> Thanks
> >>
> >
> > --
> > 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-next v2] xen-netback: stop vif thread spinning if frontend is unresponsive
From: Paul Durrant @ 2014-01-08 12:41 UTC (permalink / raw)
To: netdev, xen-devel; +Cc: Paul Durrant, Wei Liu, Ian Campbell, David Vrabel
The recent patch to improve guest receive side flow control (ca2f09f2) had a
slight flaw in the wait condition for the vif thread in that any remaining
skbs in the guest receive side netback internal queue would prevent the
thread from sleeping. An unresponsive frontend can lead to a permanently
non-empty internal queue and thus the thread will spin. In this case the
thread should really sleep until the frontend becomes responsive again.
This patch adds an extra flag to the vif which is set if the shared ring
is full and cleared when skbs are drained into the shared ring. Thus,
if the thread runs, finds the shared ring full and can make no progress the
flag remains set. If the flag remains set then the thread will sleep,
regardless of a non-empty queue, until the next event from the frontend.
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: David Vrabel <david.vrabel@citrix.com>
---
v2:
- Use bool for ring_full
- Convert need_to_notify to bool for consistency
drivers/net/xen-netback/common.h | 1 +
drivers/net/xen-netback/netback.c | 14 +++++++++-----
2 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h
index c955fc3..4c76bcb 100644
--- a/drivers/net/xen-netback/common.h
+++ b/drivers/net/xen-netback/common.h
@@ -143,6 +143,7 @@ struct xenvif {
char rx_irq_name[IFNAMSIZ+4]; /* DEVNAME-rx */
struct xen_netif_rx_back_ring rx;
struct sk_buff_head rx_queue;
+ bool rx_queue_stopped;
/* Set when the RX interrupt is triggered by the frontend.
* The worker thread may need to wake the queue.
*/
diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
index 4f81ac0..2738563 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -476,7 +476,8 @@ static void xenvif_rx_action(struct xenvif *vif)
int ret;
unsigned long offset;
struct skb_cb_overlay *sco;
- int need_to_notify = 0;
+ bool need_to_notify = false;
+ bool ring_full = false;
struct netrx_pending_operations npo = {
.copy = vif->grant_copy_op,
@@ -508,7 +509,8 @@ static void xenvif_rx_action(struct xenvif *vif)
/* If the skb may not fit then bail out now */
if (!xenvif_rx_ring_slots_available(vif, max_slots_needed)) {
skb_queue_head(&vif->rx_queue, skb);
- need_to_notify = 1;
+ need_to_notify = true;
+ ring_full = true;
break;
}
@@ -521,6 +523,8 @@ static void xenvif_rx_action(struct xenvif *vif)
BUG_ON(npo.meta_prod > ARRAY_SIZE(vif->meta));
+ vif->rx_queue_stopped = !npo.copy_prod && ring_full;
+
if (!npo.copy_prod)
goto done;
@@ -592,8 +596,7 @@ static void xenvif_rx_action(struct xenvif *vif)
RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(&vif->rx, ret);
- if (ret)
- need_to_notify = 1;
+ need_to_notify |= !!ret;
npo.meta_cons += sco->meta_slots_used;
dev_kfree_skb(skb);
@@ -1724,7 +1727,8 @@ static struct xen_netif_rx_response *make_rx_response(struct xenvif *vif,
static inline int rx_work_todo(struct xenvif *vif)
{
- return !skb_queue_empty(&vif->rx_queue) || vif->rx_event;
+ return (!skb_queue_empty(&vif->rx_queue) && !vif->rx_queue_stopped) ||
+ vif->rx_event;
}
static inline int tx_work_todo(struct xenvif *vif)
--
1.7.10.4
^ permalink raw reply related
* Re: Possible to add netfilter hooks to IFB driver?
From: Jamal Hadi Salim @ 2014-01-08 12:28 UTC (permalink / raw)
To: Alban Crequy, Andrew Collins
Cc: Stephen Hemminger, Brad Johnson, netdev@vger.kernel.org
In-Reply-To: <20140107111807.38624f74@rainbow.cbg.collabora.co.uk>
On 01/07/14 06:18, Alban Crequy wrote:
> On Mon, 6 Jan 2014 15:51:37 -0700
> Andrew Collins <bsderandrew@gmail.com> wrote:
>
>> On Mon, Jan 6, 2014 at 2:51 PM, Stephen Hemminger
>> <stephen@networkplumber.org> wrote:
>>>
>>> The risk is creating the same races that made IMQ unacceptable.
>>> --
>>
>> I believe openwrt nowadays uses a TC action which runs the packet
>> through prerouting then pulls in the mark off the ct entry into the
>> skb, so ingress+IFB can take action on it.
>
> Thanks for the info. I guess the implementation is this one:
>
> https://dev.openwrt.org/browser/trunk/target/linux/generic/patches-3.12/621-sched_act_connmark.patch
> https://dev.openwrt.org/browser/trunk/package/network/utils/iproute2/patches/210-add-act_connmark.patch
> https://dev.openwrt.org/browser/trunk/package/network/config/qos-scripts/files/usr/lib/qos/generate.sh#L343
>
>> Perhaps a cleaned up version of this would be suitable for upstream?
>
> I don't know but this seems a useful feature to me.
>
I like that approach - discussion was had here on netdev
about a year ago refer to:
http://marc.info/?t=135591832200007&r=1&w=2
since it is a long thread, jump to here:
http://marc.info/?l=linux-netdev&m=135634890120552&w=2
I believe Pablo brought it up at the last netfilter meeting
and there was no disagreement to get it going.
I dont know if kids still use these expressions - but send him
some virtual beer and he may return the love.
cheers,
jamal
^ permalink raw reply
* Re: [PATCH net-next V2 1/3] net: Add GRO support for UDP encapsulating protocols
From: Or Gerlitz @ 2014-01-08 12:15 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Or Gerlitz, netdev
In-Reply-To: <1389182291.26646.79.camel@edumazet-glaptop2.roam.corp.google.com>
On 08/01/2014 13:58, Eric Dumazet wrote:
> On Wed, 2014-01-08 at 10:04 +0200, Or Gerlitz wrote:
>> On 07/01/2014 23:38, Eric Dumazet wrote:
>>> On Tue, 2014-01-07 at 22:37 +0200, Or Gerlitz wrote:
>>>
>>>> So here's the thing, per my understanding we want to GRO only received
>>>> **encapsulated** packets whose checksum status is != CHECKSUM_NONE
>>>> which means the NIC has some support for doing RX checksum of
>>>> encapsulated packets. Per the current convension, in that case the NIC
>>>> RX code has to set skb->encapsulation see 6a674e9c75b17 "net: Add
>>>> support for hardware-offloaded encapsulation" this convension is
>>>> implemented in the current drivers that have HW offloads for
>>>> encapsulated packets (bnx2x, i40e and mlx4)
>>> I do not think its true.
>>>
>>> Some drivers set CHECKSUM_COMPLETE even for regular UDP frames...
>>>
>>> git grep -n CHECKSUM_COMPLETE -- drivers/net
>>>
>>>
>>>
>> Eric, the point I was trying to make is that as long as the driver set a
>> value which is different from CHECKSUM_NONE
>> for an skb who carry encapsulated packet, we want skb->encapsulation to
>> be set, per the architecture dictated by the above commit.
> Then this point is obviously wrong. Your interpretation is wrong, I am very sorry.
> skb->encapsulation _might_ be set, only if the NIC is performing header
> analysis. CHECKSUM_COMPLETE support doesn't require header analysis.
>
>
>
>
fair enough && thanks for shedding more light on this - so in that
respect, the gro handler for udp encapsulated packets will not flush skb
who is either marked with CHECKSUM_COMPLETE or has skb->encapsulation set.
Or.
^ permalink raw reply
* [PATCH net-next] cxgb4: Changed FW check version to match FW binary version
From: Hariprasad Shenai @ 2014-01-08 11:24 UTC (permalink / raw)
To: netdev; +Cc: davem, dm, leedom, nirranjan, kumaras, santosh, hariprasad
Signed-off-by: Hariprasad Shenai <hariprasad@chelsio.com>
---
drivers/net/ethernet/chelsio/cxgb4/cxgb4.h | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h b/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h
index b97e35c..16782b2 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h
@@ -50,13 +50,13 @@
#include "cxgb4_uld.h"
#define T4FW_VERSION_MAJOR 0x01
-#define T4FW_VERSION_MINOR 0x06
-#define T4FW_VERSION_MICRO 0x18
+#define T4FW_VERSION_MINOR 0x09
+#define T4FW_VERSION_MICRO 0x17
#define T4FW_VERSION_BUILD 0x00
#define T5FW_VERSION_MAJOR 0x01
-#define T5FW_VERSION_MINOR 0x08
-#define T5FW_VERSION_MICRO 0x1C
+#define T5FW_VERSION_MINOR 0x09
+#define T5FW_VERSION_MICRO 0x17
#define T5FW_VERSION_BUILD 0x00
#define CH_WARN(adap, fmt, ...) dev_warn(adap->pdev_dev, fmt, ## __VA_ARGS__)
--
1.8.0
^ permalink raw reply related
* Re: [PATCH net-next 1/2] cxgb4: Fix namespace collision issue.
From: Hariprasad S @ 2014-01-08 11:19 UTC (permalink / raw)
To: David Miller; +Cc: netdev, dm, leedom, nirranjan, kumaras, santosh
In-Reply-To: <20140107.154813.1483946457674959301.davem@davemloft.net>
On Tue, Jan 07, 2014 at 15:48:13 -0500, David Miller wrote:
> From: Hariprasad Shenai <hariprasad@chelsio.com>
> Date: Tue, 7 Jan 2014 16:58:07 +0530
>
> > Signed-off-by: Hariprasad Shenai <hariprasad@chelsio.com>
> ...
>
> > -int t4_init_tp_params(struct adapter *adap)
>
> There is no reason that an already exported function must be moved
> to solve a namespace issue.
>
> If you want to move this function for another reason, fine, but do
> it in a seperate change with a clear description of that reason in
> your commit message.
>
> I'm not applying this series.
I will drop this patch series. I will send an independent patch for FW version
check.
^ permalink raw reply
* Re: [PATCH 1/2] net/mlx4_core: clean up cq_res_start_move_to()
From: Or Gerlitz @ 2014-01-08 11:18 UTC (permalink / raw)
To: Paul Bolle, Jack Morgenstein, Rony Efraim, Hadar Hen Zion,
David S. Miller
Cc: netdev, linux-kernel
In-Reply-To: <1389099678.15032.19.camel@x41>
On 07/01/2014 15:01, Paul Bolle wrote:
> Building resource_tracker.o triggers a GCC warning:
> drivers/net/ethernet/mellanox/mlx4/resource_tracker.c: In function 'mlx4_HW2SW_CQ_wrapper':
> drivers/net/ethernet/mellanox/mlx4/resource_tracker.c:3019:16: warning: 'cq' may be used uninitialized in this function [-Wmaybe-uninitialized]
> atomic_dec(&cq->mtt->ref_count);
> ^
>
> This is a false positive. But a cleanup of cq_res_start_move_to() can
> help GCC here. The code currently uses a switch statement where a plain
> if/else would do, since only two of the switch's four cases can ever
> occur. Dropping that switch makes the warning go away.
>
> While we're at it, do some coding style cleanups (missing braces), and
> drop a test that always evaluates to true.
>
Hi Paul,
Our maintainer of that area of the code (SRIOV resource tracker) is busy
now, but we will definitely look on these two patches in the coming
days, thanks for posting them!
^ permalink raw reply
* [PATCH 3/3] ss: add unix_seqpacket to the help message and the man page
From: Masatake YAMATO @ 2014-01-08 11:13 UTC (permalink / raw)
To: netdev; +Cc: yamato
In-Reply-To: <1389179628-22147-1-git-send-email-yamato@redhat.com>
Signed-off-by: Masatake YAMATO <yamato@redhat.com>
---
man/man8/ss.8 | 2 +-
misc/ss.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/man/man8/ss.8 b/man/man8/ss.8
index e55dd0c..807d9dc 100644
--- a/man/man8/ss.8
+++ b/man/man8/ss.8
@@ -87,7 +87,7 @@ Currently the following families are supported: unix, inet, inet6, link, netlink
.B \-A QUERY, \-\-query=QUERY, \-\-socket=QUERY
List of socket tables to dump, separated by commas. The following identifiers
are understood: all, inet, tcp, udp, raw, unix, packet, netlink, unix_dgram,
-unix_stream, packet_raw, packet_dgram.
+unix_stream, unix_seqpacket, packet_raw, packet_dgram.
.TP
.B \-D FILE, \-\-diag=FILE
Do not display anything, just dump raw information about TCP sockets to FILE after applying filters. If FILE is - stdout is used.
diff --git a/misc/ss.c b/misc/ss.c
index cea3f2e..675f7c5 100644
--- a/misc/ss.c
+++ b/misc/ss.c
@@ -3072,7 +3072,7 @@ static void _usage(FILE *dest)
" -f, --family=FAMILY display sockets of type FAMILY\n"
"\n"
" -A, --query=QUERY, --socket=QUERY\n"
-" QUERY := {all|inet|tcp|udp|raw|unix|packet|netlink}[,QUERY]\n"
+" QUERY := {all|inet|tcp|udp|raw|unix|unix_dgram|unix_stream|unix_seqpacket|packet|netlink}[,QUERY]\n"
"\n"
" -D, --diag=FILE Dump raw information about TCP sockets to FILE\n"
" -F, --filter=FILE read filter information from FILE\n"
--
1.8.4.2
^ permalink raw reply related
* [PATCH 2/3] ss: enable query by type in unix domain related socket
From: Masatake YAMATO @ 2014-01-08 11:13 UTC (permalink / raw)
To: netdev; +Cc: yamato
In-Reply-To: <1389179628-22147-1-git-send-email-yamato@redhat.com>
This patch enables -A unix_stream, -A unix_dgram and
-A unix_seqpacket option even if ss gets socket information
via netlink.
Signed-off-by: Masatake YAMATO <yamato@redhat.com>
---
misc/ss.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/misc/ss.c b/misc/ss.c
index bac1f9e..cea3f2e 100644
--- a/misc/ss.c
+++ b/misc/ss.c
@@ -2205,6 +2205,13 @@ static int unix_show_sock(struct nlmsghdr *nlh, struct filter *f)
parse_rtattr(tb, UNIX_DIAG_MAX, (struct rtattr*)(r+1),
nlh->nlmsg_len - NLMSG_LENGTH(sizeof(*r)));
+ if (r->udiag_type == SOCK_STREAM && !(f->dbs&(1<<UNIX_ST_DB)))
+ return 0;
+ if (r->udiag_type == SOCK_DGRAM && !(f->dbs&(1<<UNIX_DG_DB)))
+ return 0;
+ if (r->udiag_type == SOCK_SEQPACKET && !(f->dbs&(1<<UNIX_SQ_DB)))
+ return 0;
+
if (netid_width)
printf("%-*s ", netid_width,
unix_netid_name(r->udiag_type));
--
1.8.4.2
^ permalink raw reply related
* [PATCH 1/3] ss: handle seqpacket type of unix domain socket
From: Masatake YAMATO @ 2014-01-08 11:13 UTC (permalink / raw)
To: netdev; +Cc: yamato
ss didn't distignish seqpacket type from dgram type.
With this patch ss can distignish it.
$ misc/ss -x -a | grep seq
u_seq LISTEN 0 128 /run/udev/control 10966 * 0
u_seq ESTAB 0 0 * 115103 * 115104
u_seq ESTAB 0 0 * 115104 * 115103
Signed-off-by: Masatake YAMATO <yamato@redhat.com>
---
misc/ss.c | 31 ++++++++++++++++++++++++++++---
1 file changed, 28 insertions(+), 3 deletions(-)
diff --git a/misc/ss.c b/misc/ss.c
index e59ca5c..bac1f9e 100644
--- a/misc/ss.c
+++ b/misc/ss.c
@@ -71,6 +71,7 @@ enum
RAW_DB,
UNIX_DG_DB,
UNIX_ST_DB,
+ UNIX_SQ_DB,
PACKET_DG_DB,
PACKET_R_DB,
NETLINK_DB,
@@ -78,7 +79,7 @@ enum
};
#define PACKET_DBM ((1<<PACKET_DG_DB)|(1<<PACKET_R_DB))
-#define UNIX_DBM ((1<<UNIX_DG_DB)|(1<<UNIX_ST_DB))
+#define UNIX_DBM ((1<<UNIX_DG_DB)|(1<<UNIX_ST_DB)|(1<<UNIX_SQ_DB))
#define ALL_DB ((1<<MAX_DB)-1)
enum {
@@ -2114,6 +2115,25 @@ static void unix_list_free(struct unixstat *list)
}
}
+static const char *unix_netid_name(int type)
+{
+ const char *netid;
+
+ switch (type) {
+ case SOCK_STREAM:
+ netid = "u_str";
+ break;
+ case SOCK_SEQPACKET:
+ netid = "u_seq";
+ break;
+ case SOCK_DGRAM:
+ default:
+ netid = "u_dgr";
+ break;
+ }
+ return netid;
+}
+
static void unix_list_print(struct unixstat *list, struct filter *f)
{
struct unixstat *s;
@@ -2126,6 +2146,8 @@ static void unix_list_print(struct unixstat *list, struct filter *f)
continue;
if (s->type == SOCK_DGRAM && !(f->dbs&(1<<UNIX_DG_DB)))
continue;
+ if (s->type == SOCK_SEQPACKET && !(f->dbs&(1<<UNIX_SQ_DB)))
+ continue;
peer = "*";
if (s->peer) {
@@ -2156,7 +2178,7 @@ static void unix_list_print(struct unixstat *list, struct filter *f)
if (netid_width)
printf("%-*s ", netid_width,
- s->type == SOCK_STREAM ? "u_str" : "u_dgr");
+ unix_netid_name(s->type));
if (state_width)
printf("%-*s ", state_width, sstate_name[s->state]);
printf("%-6d %-6d ", s->rq, s->wq);
@@ -2185,7 +2207,7 @@ static int unix_show_sock(struct nlmsghdr *nlh, struct filter *f)
if (netid_width)
printf("%-*s ", netid_width,
- r->udiag_type == SOCK_STREAM ? "u_str" : "u_dgr");
+ unix_netid_name(r->udiag_type));
if (state_width)
printf("%-*s ", state_width, sstate_name[r->udiag_state]);
@@ -3253,6 +3275,9 @@ int main(int argc, char *argv[])
} else if (strcasecmp(p, "unix_dgram") == 0 ||
strcmp(p, "u_dgr") == 0) {
current_filter.dbs |= (1<<UNIX_DG_DB);
+ } else if (strcasecmp(p, "unix_seqpacket") == 0 ||
+ strcmp(p, "u_seq") == 0) {
+ current_filter.dbs |= (1<<UNIX_SQ_DB);
} else if (strcmp(p, "packet") == 0) {
current_filter.dbs |= PACKET_DBM;
} else if (strcmp(p, "packet_raw") == 0 ||
--
1.8.4.2
^ permalink raw reply related
* [PATCH -next] openvswitch: Use kmem_cache_free() instead of kfree()
From: Wei Yongjun @ 2014-01-08 10:13 UTC (permalink / raw)
To: jesse, pshelar, davem; +Cc: yongjun_wei, dev, netdev
From: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
memory allocated by kmem_cache_alloc() should be freed using
kmem_cache_free(), not kfree().
Fixes: e298e5057006 ('openvswitch: Per cpu flow stats.')
Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
---
net/openvswitch/flow_table.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/openvswitch/flow_table.c b/net/openvswitch/flow_table.c
index b430d42..c58a0fe 100644
--- a/net/openvswitch/flow_table.c
+++ b/net/openvswitch/flow_table.c
@@ -104,7 +104,7 @@ struct sw_flow *ovs_flow_alloc(bool percpu_stats)
}
return flow;
err:
- kfree(flow);
+ kmem_cache_free(flow_cache, flow);
return ERR_PTR(-ENOMEM);
}
^ permalink raw reply related
* Re: [PATCH net-next v5] IPv6: add the option to use anycast addresses as source addresses in echo reply
From: François-Xavier Le Bail @ 2014-01-08 9:56 UTC (permalink / raw)
To: Bill Fink, netdev, David S. Miller, Alexey Kuznetsov,
James Morris, Hideaki Yoshifuji, Patrick McHardy,
Hannes Frederic Sowa
In-Reply-To: <20140107231737.GM30393@order.stressinduktion.org>
On Tue, 1/7/14, Hannes Frederic Sowa <hannes@stressinduktion.org> wrote:
> I forgot to mention, rest of kernel followes the old RFC
> advise to never
> use anycast addresses as source addresses.
> I guess this will be weakend with upcoming patches so in
> future this
> depends on socket settings and other tweaks. If that's the
> case default will
> be to also respond with anycast source if original
> destination was anycast.
The next patch will address the datagrams case.
BR
Francois-Xavier
^ permalink raw reply
* RE: [PATCH net-next] xen-netback: stop vif thread spinning if frontend is unresponsive
From: David Laight @ 2014-01-08 9:55 UTC (permalink / raw)
To: 'David Miller', paul.durrant@citrix.com
Cc: netdev@vger.kernel.org, xen-devel@lists.xen.org,
wei.liu2@citrix.com, ian.campbell@citrix.com,
david.vrabel@citrix.com
In-Reply-To: <20140107.162956.1062166230525232035.davem@davemloft.net>
> From: David Miller
...
> > - if (!npo.copy_prod)
> > + if (!npo.copy_prod) {
> > + if (ring_full)
> > + vif->rx_queue_stopped = true;
> > goto done;
> > + }
> > +
> > + vif->rx_queue_stopped = false;
>
> And then you can code this as:
>
> vif->rx_queue_stopped = (!npo.copy_prod && ring_full);
> if (!npo.copy_prod)
> goto done;
Which isn't quite the same...
1) It always writes vif->rx_queue_stopped, the old code could
leave it unchanged.
2) If 'npo' is global then the compiler can't assume that 'vif'
doesn't alias it so may have to re-read it following the
write to 'vif->rx_queue_stopped'.
David
^ permalink raw reply
* RE: [PATCH net-next] xen-netback: stop vif thread spinning if frontend is unresponsive
From: Paul Durrant @ 2014-01-08 9:49 UTC (permalink / raw)
To: David Miller
Cc: netdev@vger.kernel.org, xen-devel@lists.xen.org, Wei Liu,
Ian Campbell, David Vrabel
In-Reply-To: <20140107.162956.1062166230525232035.davem@davemloft.net>
> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-
> owner@vger.kernel.org] On Behalf Of David Miller
> Sent: 07 January 2014 21:30
> To: Paul Durrant
> Cc: netdev@vger.kernel.org; xen-devel@lists.xen.org; Wei Liu; Ian Campbell;
> David Vrabel
> Subject: Re: [PATCH net-next] xen-netback: stop vif thread spinning if
> frontend is unresponsive
>
> From: Paul Durrant <paul.durrant@citrix.com>
> Date: Tue, 7 Jan 2014 16:25:29 +0000
>
> > @@ -477,6 +477,7 @@ static void xenvif_rx_action(struct xenvif *vif)
> > unsigned long offset;
> > struct skb_cb_overlay *sco;
> > int need_to_notify = 0;
> > + int ring_full = 0;
>
> Please use bool, false, and true.
>
> >
> > - if (!npo.copy_prod)
> > + if (!npo.copy_prod) {
> > + if (ring_full)
> > + vif->rx_queue_stopped = true;
> > goto done;
> > + }
> > +
> > + vif->rx_queue_stopped = false;
>
> And then you can code this as:
>
> vif->rx_queue_stopped = (!npo.copy_prod && ring_full);
> if (!npo.copy_prod)
> goto done;
Sure. I was just following style (of need_to_notify). If you prefer bool then I'll use that and also convert need_to_notify.
Paul
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH net-next V2 3/3] net: Add GRO support for vxlan traffic
From: Or Gerlitz @ 2014-01-08 9:45 UTC (permalink / raw)
To: Tom Herbert
Cc: Eric Dumazet, Jerry Chu, Eric Dumazet, Herbert Xu,
Linux Netdev List, Yan Burman, Shlomo Pongratz
In-Reply-To: <CA+mtBx_Zctape7mAA=d-bvrFJbzn8f6tW5jBnPUcL5Cfhfai-g@mail.gmail.com>
On 07/01/2014 23:09, Tom Herbert wrote:
> On Tue, Jan 7, 2014 at 12:12 PM, Or Gerlitz <or.gerlitz@gmail.com> wrote:
>> On Tue, Jan 7, 2014 at 10:02 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>>> On Tue, 2014-01-07 at 21:43 +0200, Or Gerlitz wrote:
>>>> On Tue, Jan 7, 2014 at 8:08 PM, Tom Herbert <therbert@google.com> wrote:
>>>>> Why ^ instead of != ?
>>>> The XOR approach is very popular in the GRO stack, e.g see the IPv4 chain
>>>> of inet_gro_receive() && tcp_gro_receive(), I guess this might relates
>>>> to more efficient assembly code for ^ vs. != and/or the fast/elegant
>>>> transitive nature of that operator
>>> This trick is only needed/used when many compares are folded into a
>>> single conditional :
>>>
>>> if (a->f1 != b->f1 || a->f2 != b->f2)
>>>
>>> ->
>>>
>>> if (((a->f1 ^ b->f1) | (a->f2 ^ b->f2)) != 0)
>>>
>>> Please do not use XOR for a single compare.
>> OK, but just out of curiosity -- what's the reasoning? clarity or
>> efficiency or both?
> Both. Compiling a simple program and comparing alternatives: gcc
> produced the identical code for the single conditional (^ vs !=)
> using the cmp instruction. Testing the two conditional case like Eric
> provided; the second method (using ^) resulted in 4 more instructions,
> but only one branch as opposed to two in the first method (!=). Method
> #1 has the advantage of short circuiting when the first condition is
> true, so organizing the conditionals to maximize the probability of
> short circuit could be beneficial.
OK, I will follow that.
Or.
^ permalink raw reply
* Re: [PATCH -next] net/mlx4_en: fix error return code in mlx4_en_get_qp()
From: Or Gerlitz @ 2014-01-08 9:40 UTC (permalink / raw)
To: weiyj.lk; +Cc: David Miller, amirv, yongjun_wei, netdev
In-Reply-To: <20140107.154328.116165277036485786.davem@davemloft.net>
On 07/01/2014 22:43, David Miller wrote:
> From: Wei Yongjun <weiyj.lk@gmail.com>
> Date: Tue, 7 Jan 2014 16:56:07 +0800
>
>> From: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
>>
>> Fix to return a negative error code from the error handling
>> case instead of 0.
>>
>> Fixes: 837052d0ccc5 ('net/mlx4_en: Add netdev support for TCP/IP offloads of vxlan tunneling')
>> Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
> Looks good, applied, thanks.
Indeed, thanks for spotting and fixing.
Or.
^ permalink raw reply
* [PATCH net-next] ipv6: move IPV6_TCLASS_SHIFT into ipv6.h
From: roy.qing.li @ 2014-01-08 9:37 UTC (permalink / raw)
To: netdev
From: Li RongQing <roy.qing.li@gmail.com>
Two places defined IPV6_TCLASS_SHIFT, so we should move it into ipv6.h,
and use this macro as possible.
Signed-off-by: Li RongQing <roy.qing.li@gmail.com>
---
include/net/ipv6.h | 1 +
net/ipv6/fib6_rules.c | 4 +++-
net/ipv6/ip6_gre.c | 2 --
net/ipv6/ip6_tunnel.c | 2 --
net/ipv6/ipv6_sockglue.c | 4 +++-
5 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index 12079c6..d819f7a 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -239,6 +239,7 @@ struct ip6_flowlabel {
#define IPV6_FLOWINFO_MASK cpu_to_be32(0x0FFFFFFF)
#define IPV6_FLOWLABEL_MASK cpu_to_be32(0x000FFFFF)
#define IPV6_TCLASS_MASK (IPV6_FLOWINFO_MASK & ~IPV6_FLOWLABEL_MASK)
+#define IPV6_TCLASS_SHIFT 20
struct ipv6_fl_socklist {
struct ipv6_fl_socklist __rcu *next;
diff --git a/net/ipv6/fib6_rules.c b/net/ipv6/fib6_rules.c
index 3fd0a57..d50c5ca 100644
--- a/net/ipv6/fib6_rules.c
+++ b/net/ipv6/fib6_rules.c
@@ -150,6 +150,8 @@ static int fib6_rule_match(struct fib_rule *rule, struct flowi *fl, int flags)
{
struct fib6_rule *r = (struct fib6_rule *) rule;
struct flowi6 *fl6 = &fl->u.ip6;
+ u8 tclass = (ntohl(fl6->flowlabel) & IPV6_TCLASS_MASK) >>
+ IPV6_TCLASS_SHIFT;
if (r->dst.plen &&
!ipv6_prefix_equal(&fl6->daddr, &r->dst.addr, r->dst.plen))
@@ -169,7 +171,7 @@ static int fib6_rule_match(struct fib_rule *rule, struct flowi *fl, int flags)
return 0;
}
- if (r->tclass && r->tclass != ((ntohl(fl6->flowlabel) >> 20) & 0xff))
+ if (r->tclass && r->tclass != tclass)
return 0;
return 1;
diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
index e7a440d..f3ffb43 100644
--- a/net/ipv6/ip6_gre.c
+++ b/net/ipv6/ip6_gre.c
@@ -61,8 +61,6 @@ static bool log_ecn_error = true;
module_param(log_ecn_error, bool, 0644);
MODULE_PARM_DESC(log_ecn_error, "Log packets received with corrupted ECN");
-#define IPV6_TCLASS_SHIFT 20
-
#define HASH_SIZE_SHIFT 5
#define HASH_SIZE (1 << HASH_SIZE_SHIFT)
diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index 1e5e240..5db8d31 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -69,8 +69,6 @@ MODULE_ALIAS_NETDEV("ip6tnl0");
#define IP6_TNL_TRACE(x...) do {;} while(0)
#endif
-#define IPV6_TCLASS_SHIFT 20
-
#define HASH_SIZE_SHIFT 5
#define HASH_SIZE (1 << HASH_SIZE_SHIFT)
diff --git a/net/ipv6/ipv6_sockglue.c b/net/ipv6/ipv6_sockglue.c
index af0ecb9..1c9aa35 100644
--- a/net/ipv6/ipv6_sockglue.c
+++ b/net/ipv6/ipv6_sockglue.c
@@ -1019,7 +1019,9 @@ static int do_ipv6_getsockopt(struct sock *sk, int level, int optname,
put_cmsg(&msg, SOL_IPV6, IPV6_HOPLIMIT, sizeof(hlim), &hlim);
}
if (np->rxopt.bits.rxtclass) {
- int tclass = ntohl(np->rcv_flowinfo & IPV6_TCLASS_MASK) >> 20;
+ int tclass = ntohl(np->rcv_flowinfo &
+ IPV6_TCLASS_MASK) >>
+ IPV6_TCLASS_SHIFT;
put_cmsg(&msg, SOL_IPV6, IPV6_TCLASS, sizeof(tclass), &tclass);
}
if (np->rxopt.bits.rxoinfo) {
--
1.7.10.4
^ permalink raw reply related
* Re: [PATCH 2/2] utils: add test for nfq_get_uid and nfq_get_gid
From: Valentina Giusti @ 2014-01-08 9:36 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: netfilter-devel, netdev, fw, daniel.wagner
In-Reply-To: <20140107234319.GB17986@localhost>
On 01/08/2014 12:43 AM, Pablo Neira Ayuso wrote:
> Applied, but mangled the error message to avoid confusing users in
> case they use an old kernel.
>
Thanks Pablo!
BR,
- Val
^ permalink raw reply
* RE: [PATCH net 2/2] be2net: Need a delay before processing CQE after 2nd mbox register write
From: David Laight @ 2014-01-08 9:32 UTC (permalink / raw)
To: 'Somnath Kotur', netdev@vger.kernel.org
Cc: davem@davemloft.net, Kalesh AP
In-Reply-To: <4b4fd4cb-9d23-4900-a20c-f2c2bef70849@CMEXHTCAS2.ad.emulex.com>
> From: Somnath Kotur
> Due to Host platform synchronization issues between the mbox RDY bit polled
> status and the completion of the DMA for the CQE, it is preferable that the
> Host always wait for the RDY bit to transition to 1 after the 2nd mbox register
> write and always follow that with a short wait for the valid bit in the CQE,
> before processing the CQE.
While I don't doubt that a delay(1) fixes the problem it doesn't
seem an ideal solution.
I've not looked at what the code is doing (or how often it does it)
but either delay(1) is far, far longer than is necessary or it might
not be long enough and some kind of retry loop is required.
It might even be that the driver is just missing a memory barrier.
David
> Signed-off-by: Kalesh AP <kalesh.purayil@emulex.com>
> Signed-off-by: Somnath Kotur <somnath.kotur@emulex.com>
> ---
> drivers/net/ethernet/emulex/benet/be_cmds.c | 3 +++
> 1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/ethernet/emulex/benet/be_cmds.c b/drivers/net/ethernet/emulex/benet/be_cmds.c
> index 94c35c8..78560f2 100644
> --- a/drivers/net/ethernet/emulex/benet/be_cmds.c
> +++ b/drivers/net/ethernet/emulex/benet/be_cmds.c
> @@ -502,6 +502,9 @@ static int be_mbox_notify_wait(struct be_adapter *adapter)
> if (status != 0)
> return status;
>
> + /* Need a delay before processing CQE after 2nd mbox register write */
> + udelay(1);
> +
> /* A cq entry has been made now */
> if (be_mcc_compl_is_new(compl)) {
> status = be_mcc_compl_process(adapter, &mbox->compl);
> --
> 1.6.0.2
^ permalink raw reply
* Re: [PATCH net 1/2] bonding: ensure that the TSO being set on bond master
From: Ding Tianhong @ 2014-01-08 9:25 UTC (permalink / raw)
To: Veaceslav Falico; +Cc: Jay Vosburgh, Eric Dumazet, David S. Miller, Netdev
In-Reply-To: <20140108083303.GA28509@redhat.com>
On 2014/1/8 16:33, Veaceslav Falico wrote:
> On Wed, Jan 08, 2014 at 03:28:24PM +0800, Ding Tianhong wrote:
>> The commit b0ce3508(bonding: allow TSO being set on bonding master)
>> has make the TSO being set for bond dev, but in some situation, if
>> the slave did not have the NETIF_F_SG features, the bond master will
>> miss the TSO features in netdev_fix_features because the TSO is
>> depended on SG. So I have to add SG and TSO features on bond master
>> together.
>
> Do you know why TSO depends on SG? And what will happen if bonding supports
> SG, but one of its slaves doesn't?
Yes, I miss it, something terrible will happen, thanks.
I think if the slave's hw_features support the SG and TSO, but not set, the bond
maybe could set them and then performance will be better, otherwise, the bond should
not do anything for it.
Regards
Ding
>
>>
>> The netdev_add_tso_features() was only used for bonding, so I think no
>> need to export it to netdevice.h.
>>
>> Cc: Eric Dumazet <edumazet@google.com>
>> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>> ---
>> drivers/net/bonding/bond_main.c | 16 +++++++++++++++-
>> 1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index e06c445..8ce67ed 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -1045,6 +1045,20 @@ static void bond_netpoll_cleanup(struct net_device *bond_dev)
>>
>> /*---------------------------------- IOCTL ----------------------------------*/
>>
>> +/* Allow TSO being used on stacked device:
>> + * Performing the GSO segmentation before last device
>> + * is a performance improvement.
>> + * The TSO is depended on SG, so add SG and TSO together,
>> + * otherwise the netdev_fix_features() may clean the TSO.
>> + */
>> +static netdev_features_t bond_add_tso_features(netdev_features_t features,
>> + netdev_features_t mask)
>> +{
>> + return netdev_increment_features(features,
>> + NETIF_F_ALL_TSO | NETIF_F_SG,
>> + mask);
>> +}
>> +
>> static netdev_features_t bond_fix_features(struct net_device *dev,
>> netdev_features_t features)
>> {
>> @@ -1068,7 +1082,7 @@ static netdev_features_t bond_fix_features(struct net_device *dev,
>> slave->dev->features,
>> mask);
>> }
>> - features = netdev_add_tso_features(features, mask);
>> + features = bond_add_tso_features(features, mask);
>>
>> return features;
>> }
>> --
>> 1.8.0
>>
>>
>>
>
> .
>
^ 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