* Re: [RFC] gro: Is it ok to share a single napi from several devs ?
@ 2010-08-29 4:07 Stephen Hemminger
0 siblings, 0 replies; 16+ messages in thread
From: Stephen Hemminger @ 2010-08-29 4:07 UTC (permalink / raw)
To: David Miller, shemminger; +Cc: eric.dumazet, jarkao2, herbert, netdev
I am not really worried. about sky2 dual port since the dual port boards are rare. I might have one of the few in use
David Miller <davem@davemloft.net> wrote:
>From: Stephen Hemminger <shemminger@vyatta.com>
>Date: Sat, 28 Aug 2010 15:31:24 -0700
>
>> On Sat, 28 Aug 2010 14:41:30 -0700 (PDT)
>> David Miller <davem@davemloft.net> wrote:
>>
>>> Since netpoll does similar things, this means both NAPI and netpoll
>>> cannot function properly with SKY2's second port. It will only work
>>> right on the first port.
>>
>> I knew netpoll was broken on second port.
>
>Well, now we know that GRO is too :-)
>
>If we wish to keep the one-to-one mapping of NAPI contexts to
>interrupt sources, what we can do is provide some kind of "dev list".
>
>The other option is to register two NAPI contexts and schedule them
>both on an interrupt.
>
>But in both cases, detecting the end of polling, and thus when to turn
>the interrupts back on, is non-trivial.
>
>I really don't like either solution, therefore, so I'll try to brain
>storm on this a bit more.
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH] net: Fix vlan_gro_frags vs netpoll and bonding paths
@ 2010-08-27 20:50 Jarek Poplawski
2010-08-28 0:13 ` Herbert Xu
0 siblings, 1 reply; 16+ messages in thread
From: Jarek Poplawski @ 2010-08-27 20:50 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Herbert Xu, Patrick McHardy
After positive netpoll_rx_on() check in vlan_gro_receive() there is
skipped part of the "common" GRO_NORMAL path, especially "pull:" in
dev_gro_receive(), where at least eth header should be copied for
entirely paged skbs. So, eth_type_trans() can read zeroed header only.
Alas moving the netpoll_rx_on() check isn't enough here because a bit
later, in vlan_gro_common(), skb_bond_should_drop() is called, which
depends on skb->protocol and skb->pkt_type data, and can also change
eth_hdr h_dest address (which is overwritten later, btw).
To fix both problems this patch completes copying and verifying of eth
header from the fragment in vlan_gro_receive(). For that purpose the
"pull:" part of dev_gro_receive() was separated into an inline as
skb_gro_pull_in(), and there was added a "lighter" version of
napi_frags_finish(). No gro path except vlan_gro_frags() should be
affected by these changes.
Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
---
include/linux/netdevice.h | 33 ++++++++++++++++++++++++++++++
net/8021q/vlan_core.c | 16 ++++++++++----
net/core/dev.c | 48 +++++++++++++++++++++++++-------------------
3 files changed, 71 insertions(+), 26 deletions(-)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 46c36ff..ad59f76 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1339,6 +1339,36 @@ static inline void skb_gro_pull(struct sk_buff *skb, unsigned int len)
NAPI_GRO_CB(skb)->data_offset += len;
}
+/*
+ * Complete pulling of the header(s) from the fragment
+ */
+static inline void skb_gro_pull_in(struct sk_buff *skb)
+{
+ int grow;
+
+ if (skb_headlen(skb) >= skb_gro_offset(skb))
+ return;
+
+ grow = skb_gro_offset(skb) - skb_headlen(skb);
+
+ BUG_ON(skb->end - skb->tail < grow);
+
+ memcpy(skb_tail_pointer(skb), NAPI_GRO_CB(skb)->frag0, grow);
+
+ skb->tail += grow;
+ skb->data_len -= grow;
+
+ skb_shinfo(skb)->frags[0].page_offset += grow;
+ skb_shinfo(skb)->frags[0].size -= grow;
+
+ if (unlikely(!skb_shinfo(skb)->frags[0].size)) {
+ put_page(skb_shinfo(skb)->frags[0].page);
+ memmove(skb_shinfo(skb)->frags,
+ skb_shinfo(skb)->frags + 1,
+ --skb_shinfo(skb)->nr_frags * sizeof(skb_frag_t));
+ }
+}
+
static inline void *skb_gro_header_fast(struct sk_buff *skb,
unsigned int offset)
{
@@ -1701,6 +1731,9 @@ extern struct sk_buff * napi_get_frags(struct napi_struct *napi);
extern gro_result_t napi_frags_finish(struct napi_struct *napi,
struct sk_buff *skb,
gro_result_t ret);
+extern gro_result_t __napi_frags_finish(struct napi_struct *napi,
+ struct sk_buff *skb,
+ gro_result_t ret);
extern struct sk_buff * napi_frags_skb(struct napi_struct *napi);
extern gro_result_t napi_gro_frags(struct napi_struct *napi);
diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
index 01ddb04..58289fe 100644
--- a/net/8021q/vlan_core.c
+++ b/net/8021q/vlan_core.c
@@ -139,13 +139,19 @@ gro_result_t vlan_gro_frags(struct napi_struct *napi, struct vlan_group *grp,
if (!skb)
return GRO_DROP;
- if (netpoll_rx_on(skb)) {
- skb->protocol = eth_type_trans(skb, skb->dev);
+ /*
+ * Complete the eth header here, mainly for skb_bond_should_drop(),
+ * and for netpoll_rx_on() btw.
+ */
+ skb_gro_pull_in(skb);
+ skb->protocol = eth_type_trans(skb, skb->dev);
+ skb_gro_pull(skb, -ETH_HLEN);
+
+ if (netpoll_rx_on(skb))
return vlan_hwaccel_receive_skb(skb, grp, vlan_tci)
? GRO_DROP : GRO_NORMAL;
- }
- return napi_frags_finish(napi, skb,
- vlan_gro_common(napi, grp, vlan_tci, skb));
+ return __napi_frags_finish(napi, skb,
+ vlan_gro_common(napi, grp, vlan_tci, skb));
}
EXPORT_SYMBOL(vlan_gro_frags);
diff --git a/net/core/dev.c b/net/core/dev.c
index 3721fbb..cdc0be5 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3126,27 +3126,7 @@ enum gro_result dev_gro_receive(struct napi_struct *napi, struct sk_buff *skb)
ret = GRO_HELD;
pull:
- if (skb_headlen(skb) < skb_gro_offset(skb)) {
- int grow = skb_gro_offset(skb) - skb_headlen(skb);
-
- BUG_ON(skb->end - skb->tail < grow);
-
- memcpy(skb_tail_pointer(skb), NAPI_GRO_CB(skb)->frag0, grow);
-
- skb->tail += grow;
- skb->data_len -= grow;
-
- skb_shinfo(skb)->frags[0].page_offset += grow;
- skb_shinfo(skb)->frags[0].size -= grow;
-
- if (unlikely(!skb_shinfo(skb)->frags[0].size)) {
- put_page(skb_shinfo(skb)->frags[0].page);
- memmove(skb_shinfo(skb)->frags,
- skb_shinfo(skb)->frags + 1,
- --skb_shinfo(skb)->nr_frags * sizeof(skb_frag_t));
- }
- }
-
+ skb_gro_pull_in(skb);
ok:
return ret;
@@ -3267,6 +3247,32 @@ gro_result_t napi_frags_finish(struct napi_struct *napi, struct sk_buff *skb,
}
EXPORT_SYMBOL(napi_frags_finish);
+/*
+ * The lighter version of napi_frags_finish() without eth_type_trans() etc.
+ */
+gro_result_t __napi_frags_finish(struct napi_struct *napi, struct sk_buff *skb,
+ gro_result_t ret)
+{
+ switch (ret) {
+ case GRO_NORMAL:
+ if (netif_receive_skb(skb))
+ ret = GRO_DROP;
+ break;
+
+ case GRO_DROP:
+ case GRO_MERGED_FREE:
+ napi_reuse_skb(napi, skb);
+ break;
+
+ case GRO_HELD:
+ case GRO_MERGED:
+ break;
+ }
+
+ return ret;
+}
+EXPORT_SYMBOL(__napi_frags_finish);
+
struct sk_buff *napi_frags_skb(struct napi_struct *napi)
{
struct sk_buff *skb = napi->skb;
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] net: Fix vlan_gro_frags vs netpoll and bonding paths
2010-08-27 20:50 [PATCH] net: Fix vlan_gro_frags vs netpoll and bonding paths Jarek Poplawski
@ 2010-08-28 0:13 ` Herbert Xu
2010-08-28 9:44 ` Jarek Poplawski
0 siblings, 1 reply; 16+ messages in thread
From: Herbert Xu @ 2010-08-28 0:13 UTC (permalink / raw)
To: Jarek Poplawski; +Cc: David Miller, netdev, Patrick McHardy
On Fri, Aug 27, 2010 at 10:50:42PM +0200, Jarek Poplawski wrote:
> After positive netpoll_rx_on() check in vlan_gro_receive() there is
> skipped part of the "common" GRO_NORMAL path, especially "pull:" in
> dev_gro_receive(), where at least eth header should be copied for
> entirely paged skbs. So, eth_type_trans() can read zeroed header only.
Right, thanks for catching this.
> diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
> index 01ddb04..58289fe 100644
> --- a/net/8021q/vlan_core.c
> +++ b/net/8021q/vlan_core.c
> @@ -139,13 +139,19 @@ gro_result_t vlan_gro_frags(struct napi_struct *napi, struct vlan_group *grp,
> if (!skb)
> return GRO_DROP;
>
> - if (netpoll_rx_on(skb)) {
> - skb->protocol = eth_type_trans(skb, skb->dev);
> + /*
> + * Complete the eth header here, mainly for skb_bond_should_drop(),
> + * and for netpoll_rx_on() btw.
> + */
> + skb_gro_pull_in(skb);
> + skb->protocol = eth_type_trans(skb, skb->dev);
> + skb_gro_pull(skb, -ETH_HLEN);
But this code should go into the netpoll (i.e., slow-path) case
only so as not to impede performance.
Also, we need to fix this for the non-VLAN case as well.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] net: Fix vlan_gro_frags vs netpoll and bonding paths
2010-08-28 0:13 ` Herbert Xu
@ 2010-08-28 9:44 ` Jarek Poplawski
2010-08-28 10:54 ` [RFC] gro: Is it ok to share a single napi from several devs ? Eric Dumazet
0 siblings, 1 reply; 16+ messages in thread
From: Jarek Poplawski @ 2010-08-28 9:44 UTC (permalink / raw)
To: Herbert Xu; +Cc: David Miller, netdev, Patrick McHardy
On Sat, Aug 28, 2010 at 08:13:37AM +0800, Herbert Xu wrote:
> On Fri, Aug 27, 2010 at 10:50:42PM +0200, Jarek Poplawski wrote:
> > After positive netpoll_rx_on() check in vlan_gro_receive() there is
> > skipped part of the "common" GRO_NORMAL path, especially "pull:" in
> > dev_gro_receive(), where at least eth header should be copied for
> > entirely paged skbs. So, eth_type_trans() can read zeroed header only.
>
> Right, thanks for catching this.
>
> > diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
> > index 01ddb04..58289fe 100644
> > --- a/net/8021q/vlan_core.c
> > +++ b/net/8021q/vlan_core.c
> > @@ -139,13 +139,19 @@ gro_result_t vlan_gro_frags(struct napi_struct *napi, struct vlan_group *grp,
> > if (!skb)
> > return GRO_DROP;
> >
> > - if (netpoll_rx_on(skb)) {
> > - skb->protocol = eth_type_trans(skb, skb->dev);
> > + /*
> > + * Complete the eth header here, mainly for skb_bond_should_drop(),
> > + * and for netpoll_rx_on() btw.
> > + */
> > + skb_gro_pull_in(skb);
> > + skb->protocol = eth_type_trans(skb, skb->dev);
> > + skb_gro_pull(skb, -ETH_HLEN);
>
> But this code should go into the netpoll (i.e., slow-path) case
> only so as not to impede performance.
I'm not sure I can understand. What about skb_bond_should_drop()
mentioned in the comments? IMHO we have to impede performance a bit
(and treat it similarly to non-fragmented path) just to fix it, and
consider optimization of this bonding call later.
>
> Also, we need to fix this for the non-VLAN case as well.
But the only other non-VLAN case I know was fixed already...
Cheers,
Jarek P.
^ permalink raw reply [flat|nested] 16+ messages in thread
* [RFC] gro: Is it ok to share a single napi from several devs ?
2010-08-28 9:44 ` Jarek Poplawski
@ 2010-08-28 10:54 ` Eric Dumazet
2010-08-28 14:31 ` Jarek Poplawski
0 siblings, 1 reply; 16+ messages in thread
From: Eric Dumazet @ 2010-08-28 10:54 UTC (permalink / raw)
To: Jarek Poplawski, Stephen Hemminger; +Cc: Herbert Xu, David Miller, netdev
In commit f2bde7328633269ee935d9ed96535ade15cc348f
Author: Stephen Hemminger <shemminger@vyatta.com>
net: allow multiple dev per napi with GRO
GRO assumes that there is a one-to-one relationship between NAPI
structure and network device. Some devices like sky2 share multiple
devices on a single interrupt so only have one NAPI handler. Rather than
split GRO from NAPI, just have GRO assume if device changes that
it is a different flow.
It was assumed a napi could be shared by several devs, but I dont really
understand, since we have an unique ->dev pointer inside "napi_struct",
this one is set once, and never change.
This pointer is currently used from napi_get_frags() [but that could be
avoided], and from netpoll_poll_lock().
The netpoll_poll_lock() case is problematic.
static inline void *netpoll_poll_lock(struct napi_struct *napi)
{
struct net_device *dev = napi->_dev;
if (dev && dev->npinfo) {
spin_lock(&napi->poll_lock);
Maybe we should remove 'dev' field from napi_struct and replace it by a
npinfo pointer ?
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC] gro: Is it ok to share a single napi from several devs ?
2010-08-28 10:54 ` [RFC] gro: Is it ok to share a single napi from several devs ? Eric Dumazet
@ 2010-08-28 14:31 ` Jarek Poplawski
2010-08-28 14:48 ` Eric Dumazet
0 siblings, 1 reply; 16+ messages in thread
From: Jarek Poplawski @ 2010-08-28 14:31 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Stephen Hemminger, Herbert Xu, David Miller, netdev
On Sat, Aug 28, 2010 at 12:54:06PM +0200, Eric Dumazet wrote:
> In commit f2bde7328633269ee935d9ed96535ade15cc348f
> Author: Stephen Hemminger <shemminger@vyatta.com>
>
> net: allow multiple dev per napi with GRO
>
> GRO assumes that there is a one-to-one relationship between NAPI
> structure and network device. Some devices like sky2 share multiple
> devices on a single interrupt so only have one NAPI handler. Rather than
> split GRO from NAPI, just have GRO assume if device changes that
> it is a different flow.
>
>
> It was assumed a napi could be shared by several devs, but I dont really
> understand, since we have an unique ->dev pointer inside "napi_struct",
> this one is set once, and never change.
>
> This pointer is currently used from napi_get_frags() [but that could be
> avoided], and from netpoll_poll_lock().
>
> The netpoll_poll_lock() case is problematic.
>
> static inline void *netpoll_poll_lock(struct napi_struct *napi)
> {
> struct net_device *dev = napi->_dev;
>
> if (dev && dev->npinfo) {
> spin_lock(&napi->poll_lock);
>
> Maybe we should remove 'dev' field from napi_struct and replace it by a
> npinfo pointer ?
Sky2 seems to work like a single netdev (with an internal sub-netdev),
so I can't see your concern: what is the main aim of this replacement?
Jarek P.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC] gro: Is it ok to share a single napi from several devs ?
2010-08-28 14:31 ` Jarek Poplawski
@ 2010-08-28 14:48 ` Eric Dumazet
2010-08-28 15:16 ` Jarek Poplawski
2010-08-28 17:14 ` Stephen Hemminger
0 siblings, 2 replies; 16+ messages in thread
From: Eric Dumazet @ 2010-08-28 14:48 UTC (permalink / raw)
To: Jarek Poplawski; +Cc: Stephen Hemminger, Herbert Xu, David Miller, netdev
Le samedi 28 août 2010 à 16:31 +0200, Jarek Poplawski a écrit :
> On Sat, Aug 28, 2010 at 12:54:06PM +0200, Eric Dumazet wrote:
> > In commit f2bde7328633269ee935d9ed96535ade15cc348f
> > Author: Stephen Hemminger <shemminger@vyatta.com>
> >
> > net: allow multiple dev per napi with GRO
> >
> > GRO assumes that there is a one-to-one relationship between NAPI
> > structure and network device. Some devices like sky2 share multiple
> > devices on a single interrupt so only have one NAPI handler. Rather than
> > split GRO from NAPI, just have GRO assume if device changes that
> > it is a different flow.
> >
> >
> > It was assumed a napi could be shared by several devs, but I dont really
> > understand, since we have an unique ->dev pointer inside "napi_struct",
> > this one is set once, and never change.
> >
> > This pointer is currently used from napi_get_frags() [but that could be
> > avoided], and from netpoll_poll_lock().
> >
> > The netpoll_poll_lock() case is problematic.
> >
> > static inline void *netpoll_poll_lock(struct napi_struct *napi)
> > {
> > struct net_device *dev = napi->_dev;
> >
> > if (dev && dev->npinfo) {
> > spin_lock(&napi->poll_lock);
> >
> > Maybe we should remove 'dev' field from napi_struct and replace it by a
> > npinfo pointer ?
>
> Sky2 seems to work like a single netdev (with an internal sub-netdev),
> so I can't see your concern: what is the main aim of this replacement?
I am trying to understand why this commit was needed then.
It adds an extra test in the main loop, testing skb->dev against p->dev,
it must me for something...
I am trying to say that the one to one relationship between NAPI
structure and a device is not only a GRO thing, but also a netpoll one.
So either we completely remove this one to one relationship, either
Stephen commit was not needed.
Some clarification is needed.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC] gro: Is it ok to share a single napi from several devs ?
2010-08-28 14:48 ` Eric Dumazet
@ 2010-08-28 15:16 ` Jarek Poplawski
2010-08-28 17:14 ` Stephen Hemminger
1 sibling, 0 replies; 16+ messages in thread
From: Jarek Poplawski @ 2010-08-28 15:16 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Stephen Hemminger, Herbert Xu, David Miller, netdev
On Sat, Aug 28, 2010 at 04:48:29PM +0200, Eric Dumazet wrote:
> Le samedi 28 ao??t 2010 ?? 16:31 +0200, Jarek Poplawski a écrit :
> > On Sat, Aug 28, 2010 at 12:54:06PM +0200, Eric Dumazet wrote:
> > > In commit f2bde7328633269ee935d9ed96535ade15cc348f
> > > Author: Stephen Hemminger <shemminger@vyatta.com>
> > >
> > > net: allow multiple dev per napi with GRO
> > >
> > > GRO assumes that there is a one-to-one relationship between NAPI
> > > structure and network device. Some devices like sky2 share multiple
> > > devices on a single interrupt so only have one NAPI handler. Rather than
> > > split GRO from NAPI, just have GRO assume if device changes that
> > > it is a different flow.
> > >
> > >
> > > It was assumed a napi could be shared by several devs, but I dont really
> > > understand, since we have an unique ->dev pointer inside "napi_struct",
> > > this one is set once, and never change.
> > >
> > > This pointer is currently used from napi_get_frags() [but that could be
> > > avoided], and from netpoll_poll_lock().
> > >
> > > The netpoll_poll_lock() case is problematic.
> > >
> > > static inline void *netpoll_poll_lock(struct napi_struct *napi)
> > > {
> > > struct net_device *dev = napi->_dev;
> > >
> > > if (dev && dev->npinfo) {
> > > spin_lock(&napi->poll_lock);
> > >
> > > Maybe we should remove 'dev' field from napi_struct and replace it by a
> > > npinfo pointer ?
> >
> > Sky2 seems to work like a single netdev (with an internal sub-netdev),
> > so I can't see your concern: what is the main aim of this replacement?
>
> I am trying to understand why this commit was needed then.
>
> It adds an extra test in the main loop, testing skb->dev against p->dev,
> it must me for something...
>
> I am trying to say that the one to one relationship between NAPI
> structure and a device is not only a GRO thing, but also a netpoll one.
I guess it's not about the full one to one relationship, but kind of
overloading (pseudo vlan?). How much it's needed is another question,
but at least the order of these tests seems doubtful.
Jarek P.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC] gro: Is it ok to share a single napi from several devs ?
2010-08-28 14:48 ` Eric Dumazet
2010-08-28 15:16 ` Jarek Poplawski
@ 2010-08-28 17:14 ` Stephen Hemminger
2010-08-28 21:41 ` David Miller
1 sibling, 1 reply; 16+ messages in thread
From: Stephen Hemminger @ 2010-08-28 17:14 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Jarek Poplawski, Herbert Xu, David Miller, netdev
On Sat, 28 Aug 2010 16:48:29 +0200
Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le samedi 28 août 2010 à 16:31 +0200, Jarek Poplawski a écrit :
> > On Sat, Aug 28, 2010 at 12:54:06PM +0200, Eric Dumazet wrote:
> > > In commit f2bde7328633269ee935d9ed96535ade15cc348f
> > > Author: Stephen Hemminger <shemminger@vyatta.com>
> > >
> > > net: allow multiple dev per napi with GRO
> > >
> > > GRO assumes that there is a one-to-one relationship between NAPI
> > > structure and network device. Some devices like sky2 share multiple
> > > devices on a single interrupt so only have one NAPI handler. Rather than
> > > split GRO from NAPI, just have GRO assume if device changes that
> > > it is a different flow.
> > >
> > >
> > > It was assumed a napi could be shared by several devs, but I dont really
> > > understand, since we have an unique ->dev pointer inside "napi_struct",
> > > this one is set once, and never change.
> > >
> > > This pointer is currently used from napi_get_frags() [but that could be
> > > avoided], and from netpoll_poll_lock().
> > >
> > > The netpoll_poll_lock() case is problematic.
> > >
> > > static inline void *netpoll_poll_lock(struct napi_struct *napi)
> > > {
> > > struct net_device *dev = napi->_dev;
> > >
> > > if (dev && dev->npinfo) {
> > > spin_lock(&napi->poll_lock);
> > >
> > > Maybe we should remove 'dev' field from napi_struct and replace it by a
> > > npinfo pointer ?
> >
> > Sky2 seems to work like a single netdev (with an internal sub-netdev),
> > so I can't see your concern: what is the main aim of this replacement?
>
> I am trying to understand why this commit was needed then.
>
> It adds an extra test in the main loop, testing skb->dev against p->dev,
> it must me for something...
>
> I am trying to say that the one to one relationship between NAPI
> structure and a device is not only a GRO thing, but also a netpoll one.
>
> So either we completely remove this one to one relationship, either
> Stephen commit was not needed.
>
> Some clarification is needed.
>
>
The Marvell Yukon2 hardware supports two interfaces sharing a common interrupt.
Therfore the sky2 driver has up to two net devices and a single NAPI per board.
It is possible in a single invocation of the poll loop to process frames
for both ports. GRO works by combining received packets from identical
flows over one NAPI interval. It is possible on sky2 that one packet
could be processed for the first port, and the next packet processed was for
second port and the two packets were related so that GRO would combine them.
The check for the same dev is required to prevent this. Yes it is an unlikely
corner case, but the purpose of GRO is to do aggregation but preserve the
flow characteristics of the incoming traffic.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC] gro: Is it ok to share a single napi from several devs ?
2010-08-28 17:14 ` Stephen Hemminger
@ 2010-08-28 21:41 ` David Miller
2010-08-28 22:31 ` Stephen Hemminger
2010-08-29 9:59 ` Jarek Poplawski
0 siblings, 2 replies; 16+ messages in thread
From: David Miller @ 2010-08-28 21:41 UTC (permalink / raw)
To: shemminger; +Cc: eric.dumazet, jarkao2, herbert, netdev
From: Stephen Hemminger <shemminger@vyatta.com>
Date: Sat, 28 Aug 2010 10:14:24 -0700
> The Marvell Yukon2 hardware supports two interfaces sharing a common interrupt.
> Therfore the sky2 driver has up to two net devices and a single NAPI per board.
> It is possible in a single invocation of the poll loop to process frames
> for both ports. GRO works by combining received packets from identical
> flows over one NAPI interval. It is possible on sky2 that one packet
> could be processed for the first port, and the next packet processed was for
> second port and the two packets were related so that GRO would combine them.
> The check for the same dev is required to prevent this. Yes it is an unlikely
> corner case, but the purpose of GRO is to do aggregation but preserve the
> flow characteristics of the incoming traffic.
If that is true then GRO is going to refuse to merge every single
frame that arrives on the second port of a SKY2 device. :-)
This is because for the two ports, you allocate and register one NAPI
instance which uses only the first port's netdev pointer.
Therefore, when GRO compares napi->dev to skb->dev it will always not
match for packets coming from the second port since the netdev pointer
in skb->dev will be different.
Since netpoll does similar things, this means both NAPI and netpoll
cannot function properly with SKY2's second port. It will only work
right on the first port.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC] gro: Is it ok to share a single napi from several devs ?
2010-08-28 21:41 ` David Miller
@ 2010-08-28 22:31 ` Stephen Hemminger
2010-08-28 22:33 ` David Miller
2010-08-29 9:59 ` Jarek Poplawski
1 sibling, 1 reply; 16+ messages in thread
From: Stephen Hemminger @ 2010-08-28 22:31 UTC (permalink / raw)
To: David Miller; +Cc: eric.dumazet, jarkao2, herbert, netdev
On Sat, 28 Aug 2010 14:41:30 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:
> Since netpoll does similar things, this means both NAPI and netpoll
> cannot function properly with SKY2's second port. It will only work
> right on the first port.
I knew netpoll was broken on second port.
--
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC] gro: Is it ok to share a single napi from several devs ?
2010-08-28 22:31 ` Stephen Hemminger
@ 2010-08-28 22:33 ` David Miller
0 siblings, 0 replies; 16+ messages in thread
From: David Miller @ 2010-08-28 22:33 UTC (permalink / raw)
To: shemminger; +Cc: eric.dumazet, jarkao2, herbert, netdev
From: Stephen Hemminger <shemminger@vyatta.com>
Date: Sat, 28 Aug 2010 15:31:24 -0700
> On Sat, 28 Aug 2010 14:41:30 -0700 (PDT)
> David Miller <davem@davemloft.net> wrote:
>
>> Since netpoll does similar things, this means both NAPI and netpoll
>> cannot function properly with SKY2's second port. It will only work
>> right on the first port.
>
> I knew netpoll was broken on second port.
Well, now we know that GRO is too :-)
If we wish to keep the one-to-one mapping of NAPI contexts to
interrupt sources, what we can do is provide some kind of "dev list".
The other option is to register two NAPI contexts and schedule them
both on an interrupt.
But in both cases, detecting the end of polling, and thus when to turn
the interrupts back on, is non-trivial.
I really don't like either solution, therefore, so I'll try to brain
storm on this a bit more.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC] gro: Is it ok to share a single napi from several devs ?
2010-08-28 21:41 ` David Miller
2010-08-28 22:31 ` Stephen Hemminger
@ 2010-08-29 9:59 ` Jarek Poplawski
2010-08-29 17:06 ` David Miller
1 sibling, 1 reply; 16+ messages in thread
From: Jarek Poplawski @ 2010-08-29 9:59 UTC (permalink / raw)
To: David Miller; +Cc: shemminger, eric.dumazet, herbert, netdev
On Sat, Aug 28, 2010 at 02:41:30PM -0700, David Miller wrote:
> From: Stephen Hemminger <shemminger@vyatta.com>
> Date: Sat, 28 Aug 2010 10:14:24 -0700
>
> > The Marvell Yukon2 hardware supports two interfaces sharing a common interrupt.
> > Therfore the sky2 driver has up to two net devices and a single NAPI per board.
> > It is possible in a single invocation of the poll loop to process frames
> > for both ports. GRO works by combining received packets from identical
> > flows over one NAPI interval. It is possible on sky2 that one packet
> > could be processed for the first port, and the next packet processed was for
> > second port and the two packets were related so that GRO would combine them.
> > The check for the same dev is required to prevent this. Yes it is an unlikely
> > corner case, but the purpose of GRO is to do aggregation but preserve the
> > flow characteristics of the incoming traffic.
>
> If that is true then GRO is going to refuse to merge every single
> frame that arrives on the second port of a SKY2 device. :-)
>
> This is because for the two ports, you allocate and register one NAPI
> instance which uses only the first port's netdev pointer.
>
> Therefore, when GRO compares napi->dev to skb->dev it will always not
Actually, when GRO compares napi->dev to skb->dev?
Jarek P.
> match for packets coming from the second port since the netdev pointer
> in skb->dev will be different.
>
> Since netpoll does similar things, this means both NAPI and netpoll
> cannot function properly with SKY2's second port. It will only work
> right on the first port.
>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC] gro: Is it ok to share a single napi from several devs ?
2010-08-29 9:59 ` Jarek Poplawski
@ 2010-08-29 17:06 ` David Miller
2010-08-29 18:39 ` Eric Dumazet
0 siblings, 1 reply; 16+ messages in thread
From: David Miller @ 2010-08-29 17:06 UTC (permalink / raw)
To: jarkao2; +Cc: shemminger, eric.dumazet, herbert, netdev
From: Jarek Poplawski <jarkao2@gmail.com>
Date: Sun, 29 Aug 2010 11:59:51 +0200
> Actually, when GRO compares napi->dev to skb->dev?
Hmmm, I thought the code made a skb->dev comparison with the
existing SKBs in the list when checking for same-flow matches.
It doesn't, probably based upon the assumption that a NAPI
instance maps to a unique device, the very topic we're
discussing right now :-/
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC] gro: Is it ok to share a single napi from several devs ?
2010-08-29 17:06 ` David Miller
@ 2010-08-29 18:39 ` Eric Dumazet
2010-08-30 6:42 ` Jarek Poplawski
0 siblings, 1 reply; 16+ messages in thread
From: Eric Dumazet @ 2010-08-29 18:39 UTC (permalink / raw)
To: David Miller; +Cc: jarkao2, shemminger, herbert, netdev
Le dimanche 29 août 2010 à 10:06 -0700, David Miller a écrit :
> From: Jarek Poplawski <jarkao2@gmail.com>
> Date: Sun, 29 Aug 2010 11:59:51 +0200
>
> > Actually, when GRO compares napi->dev to skb->dev?
>
> Hmmm, I thought the code made a skb->dev comparison with the
> existing SKBs in the list when checking for same-flow matches.
>
> It doesn't, probably based upon the assumption that a NAPI
> instance maps to a unique device, the very topic we're
> discussing right now :-/
>
>
It does the check, Stephen added it in the commit I mentioned to start
this thread.
With net-next-2.6 this now reads :
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC] gro: Is it ok to share a single napi from several devs ?
2010-08-29 18:39 ` Eric Dumazet
@ 2010-08-30 6:42 ` Jarek Poplawski
2010-08-30 15:57 ` Stephen Hemminger
0 siblings, 1 reply; 16+ messages in thread
From: Jarek Poplawski @ 2010-08-30 6:42 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, shemminger, herbert, netdev
On 2010-08-29 20:39, Eric Dumazet wrote:
> Le dimanche 29 aoĂťt 2010 Ă 10:06 -0700, David Miller a ĂŠcrit :
>> From: Jarek Poplawski <jarkao2@gmail.com>
>> Date: Sun, 29 Aug 2010 11:59:51 +0200
>>
>>> Actually, when GRO compares napi->dev to skb->dev?
>>
>> Hmmm, I thought the code made a skb->dev comparison with the
>> existing SKBs in the list when checking for same-flow matches.
>>
>> It doesn't, probably based upon the assumption that a NAPI
>> instance maps to a unique device, the very topic we're
>> discussing right now :-/
>>
>>
>
> It does the check, Stephen added it in the commit I mentioned to start
> this thread.
>
> With net-next-2.6 this now reads :
>
Since Stephen didn't seem to miss this too much it seems quite obvious
to me this check should be removed.
Jarek P.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC] gro: Is it ok to share a single napi from several devs ?
2010-08-30 6:42 ` Jarek Poplawski
@ 2010-08-30 15:57 ` Stephen Hemminger
2010-08-30 16:50 ` David Miller
0 siblings, 1 reply; 16+ messages in thread
From: Stephen Hemminger @ 2010-08-30 15:57 UTC (permalink / raw)
To: Jarek Poplawski; +Cc: Eric Dumazet, David Miller, herbert, netdev
On Mon, 30 Aug 2010 06:42:31 +0000
Jarek Poplawski <jarkao2@gmail.com> wrote:
> On 2010-08-29 20:39, Eric Dumazet wrote:
> > Le dimanche 29 aoĂťt 2010 Ă 10:06 -0700, David Miller a ĂŠcrit :
> >> From: Jarek Poplawski <jarkao2@gmail.com>
> >> Date: Sun, 29 Aug 2010 11:59:51 +0200
> >>
> >>> Actually, when GRO compares napi->dev to skb->dev?
> >>
> >> Hmmm, I thought the code made a skb->dev comparison with the
> >> existing SKBs in the list when checking for same-flow matches.
> >>
> >> It doesn't, probably based upon the assumption that a NAPI
> >> instance maps to a unique device, the very topic we're
> >> discussing right now :-/
> >>
> >>
> >
> > It does the check, Stephen added it in the commit I mentioned to start
> > this thread.
> >
> > With net-next-2.6 this now reads :
> >
>
> Since Stephen didn't seem to miss this too much it seems quite obvious
> to me this check should be removed.
No. I just don't use that system much, breaking code for
sake of one comparison is ridiculous. If you need to do some
performance wanking, go figure out why just loading netfilter
drops forwarding performance by 20%.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC] gro: Is it ok to share a single napi from several devs ?
2010-08-30 15:57 ` Stephen Hemminger
@ 2010-08-30 16:50 ` David Miller
2010-08-30 18:36 ` Jarek Poplawski
0 siblings, 1 reply; 16+ messages in thread
From: David Miller @ 2010-08-30 16:50 UTC (permalink / raw)
To: shemminger; +Cc: jarkao2, eric.dumazet, herbert, netdev
From: Stephen Hemminger <shemminger@vyatta.com>
Date: Mon, 30 Aug 2010 08:57:21 -0700
> On Mon, 30 Aug 2010 06:42:31 +0000
> Jarek Poplawski <jarkao2@gmail.com> wrote:
>
>> On 2010-08-29 20:39, Eric Dumazet wrote:
>> > Le dimanche 29 aoĂťt 2010 Ă 10:06 -0700, David Miller a ĂŠcrit :
>> >> From: Jarek Poplawski <jarkao2@gmail.com>
>> >> Date: Sun, 29 Aug 2010 11:59:51 +0200
>> >>
>> >>> Actually, when GRO compares napi->dev to skb->dev?
>> >>
>> >> Hmmm, I thought the code made a skb->dev comparison with the
>> >> existing SKBs in the list when checking for same-flow matches.
>> >>
>> >> It doesn't, probably based upon the assumption that a NAPI
>> >> instance maps to a unique device, the very topic we're
>> >> discussing right now :-/
>> >>
>> >>
>> >
>> > It does the check, Stephen added it in the commit I mentioned to start
>> > this thread.
>> >
>> > With net-next-2.6 this now reads :
>> >
>>
>> Since Stephen didn't seem to miss this too much it seems quite obvious
>> to me this check should be removed.
>
> No. I just don't use that system much, breaking code for
> sake of one comparison is ridiculous.
It's not working to begin with.
I agree with Jarek that the check should be removed. And GRO is one
of those places that, precisely, even one memory reference removal
can improve performance dramatically.
Herbert spent a lot of time doing micro-optimizations to make GRO
better and better, and the smallest things can turn out to make a huge
difference there.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC] gro: Is it ok to share a single napi from several devs ?
2010-08-30 16:50 ` David Miller
@ 2010-08-30 18:36 ` Jarek Poplawski
0 siblings, 0 replies; 16+ messages in thread
From: Jarek Poplawski @ 2010-08-30 18:36 UTC (permalink / raw)
To: David Miller; +Cc: shemminger, eric.dumazet, herbert, netdev
On Mon, Aug 30, 2010 at 09:50:12AM -0700, David Miller wrote:
> From: Stephen Hemminger <shemminger@vyatta.com>
> Date: Mon, 30 Aug 2010 08:57:21 -0700
>
> > On Mon, 30 Aug 2010 06:42:31 +0000
> > Jarek Poplawski <jarkao2@gmail.com> wrote:
> >
> >> On 2010-08-29 20:39, Eric Dumazet wrote:
> >> > Le dimanche 29 aoĂťt 2010 Ă 10:06 -0700, David Miller a ĂŠcrit :
> >> >> From: Jarek Poplawski <jarkao2@gmail.com>
> >> >> Date: Sun, 29 Aug 2010 11:59:51 +0200
> >> >>
> >> >>> Actually, when GRO compares napi->dev to skb->dev?
> >> >>
> >> >> Hmmm, I thought the code made a skb->dev comparison with the
> >> >> existing SKBs in the list when checking for same-flow matches.
> >> >>
> >> >> It doesn't, probably based upon the assumption that a NAPI
> >> >> instance maps to a unique device, the very topic we're
> >> >> discussing right now :-/
> >> >>
> >> >>
> >> >
> >> > It does the check, Stephen added it in the commit I mentioned to start
> >> > this thread.
> >> >
> >> > With net-next-2.6 this now reads :
> >> >
> >>
> >> Since Stephen didn't seem to miss this too much it seems quite obvious
> >> to me this check should be removed.
> >
> > No. I just don't use that system much, breaking code for
> > sake of one comparison is ridiculous.
>
> It's not working to begin with.
IMHO it should work yet. On the other hand, after removing this test
GRO should still work OK for these NICs in most cases, so it should be
treated as an optimization only. And it seems very unusual to keep such
optimizations at this level for such rare cases.
> I agree with Jarek that the check should be removed. And GRO is one
> of those places that, precisely, even one memory reference removal
> can improve performance dramatically.
Hmm... I proposed removal when Stephen didn't defend it. Since he
seems to change his line, I'd prefer to be convinced I'm wrong,
preferably with some use/test case; most preferably from some
desperated user...
> Herbert spent a lot of time doing micro-optimizations to make GRO
> better and better, and the smallest things can turn out to make a huge
> difference there.
Anyway, after the last Eric's optimization, it's almost invisible...
Jarek P.
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2010-08-30 18:38 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-08-29 4:07 [RFC] gro: Is it ok to share a single napi from several devs ? Stephen Hemminger
-- strict thread matches above, loose matches on Subject: below --
2010-08-27 20:50 [PATCH] net: Fix vlan_gro_frags vs netpoll and bonding paths Jarek Poplawski
2010-08-28 0:13 ` Herbert Xu
2010-08-28 9:44 ` Jarek Poplawski
2010-08-28 10:54 ` [RFC] gro: Is it ok to share a single napi from several devs ? Eric Dumazet
2010-08-28 14:31 ` Jarek Poplawski
2010-08-28 14:48 ` Eric Dumazet
2010-08-28 15:16 ` Jarek Poplawski
2010-08-28 17:14 ` Stephen Hemminger
2010-08-28 21:41 ` David Miller
2010-08-28 22:31 ` Stephen Hemminger
2010-08-28 22:33 ` David Miller
2010-08-29 9:59 ` Jarek Poplawski
2010-08-29 17:06 ` David Miller
2010-08-29 18:39 ` Eric Dumazet
2010-08-30 6:42 ` Jarek Poplawski
2010-08-30 15:57 ` Stephen Hemminger
2010-08-30 16:50 ` David Miller
2010-08-30 18:36 ` Jarek Poplawski
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.