Message ID | 20090227041136.GA21468@gondor.apana.org.au |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
From: Herbert Xu <herbert@gondor.apana.org.au> Date: Fri, 27 Feb 2009 12:11:36 +0800 > /* VLAN rx hw acceleration helper. This acts like netif_{rx,receive_skb}(). */ > int __vlan_hwaccel_rx(struct sk_buff *skb, struct vlan_group *grp, > u16 vlan_tci, int polling) > { > + if (netpoll_receive_skb(skb)) > + return NET_RX_DROP; > + > if (skb_bond_should_drop(skb)) > goto drop; > This case should use netpoll_rx(). netif_rx_skb() --> netpoll_rx() netif_receive_skb() --> netpoll_receive_skb() -- 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
On Fri, Feb 27, 2009 at 12:11:36PM +0800, Herbert Xu wrote: > On Thu, Feb 26, 2009 at 09:06:31PM +0800, Herbert Xu wrote: > > > > OK after much head scratching and staring, it turns out that > > this is caused by the VLAN path not doing the netpoll check > > which would normally drop the packets when a printk is active. > > Note that this patch doesn't apply to net-next-2.6 as GRO has > changed there. But it should be easy to manually slot it in. > Let me know if there are any problems. > > netpoll: Add drop checks to all entry points > > The netpoll entry checks are required to ensure that we don't > receive normal packets when invoked via netpoll. Unfortunately > it only ever worked for the netif_receive_skb/netif_rx entry > points. The VLAN (and subsequently GRO) entry point didn't > have the check and therefore can trigger all sorts of weird > problems. > > This patch adds the netpoll check to all entry points. Probably I miss something, but I'm not sure it's really necessary in all (non-VLAN) entry points. Of course it's an optimization to drop these things early, but there is a lot off mess with replicating various parts of netif_receive_skb() in so many places. As a matter of fact, I wonder why it can't be done in one place, e.g. netif_nit_deliver(), which was created partly for similar problems. Jarek P. PS: it would be nice to prepare a 2.6.27 version for testing yet; it looks like needed for -stable. > > I'm still uneasy with receiving at all under netpoll (which > apparently is only used by the out-of-tree kdump code). The > reason is it is perfectly legal to receive all data including > headers into highmem if netpoll is off, but if you try to do > that with netpoll on and someone gets a printk in an IRQ handler > you're going to get a nice BUG_ON. > > Fortunately I don't think anyone is receiving everything into > highmem as it stands. > > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> > > diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c > index e9db889..a37782d 100644 > --- a/net/8021q/vlan_core.c > +++ b/net/8021q/vlan_core.c > @@ -1,12 +1,16 @@ > #include <linux/skbuff.h> > #include <linux/netdevice.h> > #include <linux/if_vlan.h> > +#include <linux/netpoll.h> > #include "vlan.h" > > /* VLAN rx hw acceleration helper. This acts like netif_{rx,receive_skb}(). */ > int __vlan_hwaccel_rx(struct sk_buff *skb, struct vlan_group *grp, > u16 vlan_tci, int polling) > { > + if (netpoll_receive_skb(skb)) > + return NET_RX_DROP; > + > if (skb_bond_should_drop(skb)) > goto drop; > > @@ -100,6 +104,9 @@ int vlan_gro_receive(struct napi_struct *napi, struct vlan_group *grp, > { > int err = NET_RX_SUCCESS; > > + if (netpoll_receive_skb(skb)) > + return NET_RX_DROP; > + > switch (vlan_gro_common(napi, grp, vlan_tci, skb)) { > case -1: > return netif_receive_skb(skb); > @@ -126,6 +133,9 @@ int vlan_gro_frags(struct napi_struct *napi, struct vlan_group *grp, > if (!skb) > goto out; > > + if (netpoll_receive_skb(skb)) > + goto out; > + > err = NET_RX_SUCCESS; > > switch (vlan_gro_common(napi, grp, vlan_tci, skb)) { > diff --git a/net/core/dev.c b/net/core/dev.c > index a17e006..72b0d26 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -2488,6 +2488,9 @@ static int __napi_gro_receive(struct napi_struct *napi, struct sk_buff *skb) > > int napi_gro_receive(struct napi_struct *napi, struct sk_buff *skb) > { > + if (netpoll_receive_skb(skb)) > + return NET_RX_DROP; > + > switch (__napi_gro_receive(napi, skb)) { > case -1: > return netif_receive_skb(skb); > @@ -2558,6 +2561,9 @@ int napi_gro_frags(struct napi_struct *napi, struct napi_gro_fraginfo *info) > if (!skb) > goto out; > > + if (netpoll_receive_skb(skb)) > + goto out; > + > err = NET_RX_SUCCESS; > > switch (__napi_gro_receive(napi, skb)) { > > Cheers, > -- > Visit Openswan at http://www.openswan.org/ > Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au> > Home Page: http://gondor.apana.org.au/~herbert/ > PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- 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
From: Jarek Poplawski <jarkao2@gmail.com> Date: Fri, 27 Feb 2009 08:41:10 +0000 > Probably I miss something, but I'm not sure it's really necessary in > all (non-VLAN) entry points. Of course it's an optimization to drop > these things early, but there is a lot off mess with replicating > various parts of netif_receive_skb() in so many places. > > As a matter of fact, I wonder why it can't be done in one place, e.g. > netif_nit_deliver(), which was created partly for similar problems. I think we do need to hit all possible entry points. How would you be able to handle it in netif_nit_deliver()? Functions like netif_receive_skb() open-code the delivery to network taps, they don't actually call netif_receive_skb(). -- 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
On Fri, Feb 27, 2009 at 12:59:07AM -0800, David Miller wrote: > From: Jarek Poplawski <jarkao2@gmail.com> > Date: Fri, 27 Feb 2009 08:41:10 +0000 > > > Probably I miss something, but I'm not sure it's really necessary in > > all (non-VLAN) entry points. Of course it's an optimization to drop > > these things early, but there is a lot off mess with replicating > > various parts of netif_receive_skb() in so many places. > > > > As a matter of fact, I wonder why it can't be done in one place, e.g. > > netif_nit_deliver(), which was created partly for similar problems. > > I think we do need to hit all possible entry points. > > How would you be able to handle it in netif_nit_deliver()? > Functions like netif_receive_skb() open-code the delivery to > network taps, they don't actually call netif_receive_skb(). netif_nit_deliver() is a place called by vlan with orig skb->dev, so it could be reused to check for netpoll btw. Of course, return value should be added etc. and maybe name changed too. It could be something like this: netif_receive_skb() if (skb->vlan_tci) { ret = vlan_hwaccel_do_receive(skb); if (ret) return ret; } ... Jarek P. -- 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
From: Jarek Poplawski <jarkao2@gmail.com> Date: Fri, 27 Feb 2009 09:12:16 +0000 > On Fri, Feb 27, 2009 at 12:59:07AM -0800, David Miller wrote: > > From: Jarek Poplawski <jarkao2@gmail.com> > > Date: Fri, 27 Feb 2009 08:41:10 +0000 > > > > > Probably I miss something, but I'm not sure it's really necessary in > > > all (non-VLAN) entry points. Of course it's an optimization to drop > > > these things early, but there is a lot off mess with replicating > > > various parts of netif_receive_skb() in so many places. > > > > > > As a matter of fact, I wonder why it can't be done in one place, e.g. > > > netif_nit_deliver(), which was created partly for similar problems. > > > > I think we do need to hit all possible entry points. > > > > How would you be able to handle it in netif_nit_deliver()? > > Functions like netif_receive_skb() open-code the delivery to > > network taps, they don't actually call netif_receive_skb(). > > netif_nit_deliver() is a place called by vlan with orig skb->dev, so > it could be reused to check for netpoll btw. Of course, return value > should be added etc. and maybe name changed too. It could be > something like this: Note there is already a function that could do this and which needs to hit all the same RX entrypoints just like this check would. And that is skb_bond_should_drop(). We could rename that to skb_rx_should_drop() and put the netpoll checks there. There is some weird conditinalization of skb_bond_should_drop()'s call in netif_receive_skb() but that should be easy to change to suit our needs. Perhaps by putting the calculation of the netdevice bonding pointers into that function. -- 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
On Fri, Feb 27, 2009 at 01:16:15AM -0800, David Miller wrote: > From: Jarek Poplawski <jarkao2@gmail.com> > Date: Fri, 27 Feb 2009 09:12:16 +0000 ... > > netif_nit_deliver() is a place called by vlan with orig skb->dev, so > > it could be reused to check for netpoll btw. Of course, return value > > should be added etc. and maybe name changed too. It could be > > something like this: > > Note there is already a function that could do this and which needs to > hit all the same RX entrypoints just like this check would. > > And that is skb_bond_should_drop(). > > We could rename that to skb_rx_should_drop() and put the netpoll > checks there. > > There is some weird conditinalization of skb_bond_should_drop()'s call > in netif_receive_skb() but that should be easy to change to suit our > needs. Perhaps by putting the calculation of the netdevice bonding > pointers into that function. Yes, it would be nice to have it in this one place, but I guess currently for vlans we depend on vlan_hwaccel_do_receive(), and there are probably some reasons it's so far from the bond check. Jarek P. -- 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
On Fri, Feb 27, 2009 at 12:03:50AM -0800, David Miller wrote: > > This case should use netpoll_rx(). Good point. > netif_rx_skb() --> netpoll_rx() > netif_receive_skb() --> netpoll_receive_skb() Actually I think we could just use netpoll_rx as netpoll_receive_skb is just an optimisation that skips the test if we've already done it. I'll do another version with combining the bonding check as you suggested. Thanks,
From: Herbert Xu <herbert@gondor.apana.org.au> Date: Fri, 27 Feb 2009 19:45:50 +0800 > On Fri, Feb 27, 2009 at 12:03:50AM -0800, David Miller wrote: > > > > This case should use netpoll_rx(). > > Good point. > > > netif_rx_skb() --> netpoll_rx() > > netif_receive_skb() --> netpoll_receive_skb() > > Actually I think we could just use netpoll_rx as netpoll_receive_skb > is just an optimisation that skips the test if we've already done it. > > I'll do another version with combining the bonding check as you > suggested. Thanks a lot Herbert. -- 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
diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c index e9db889..a37782d 100644 --- a/net/8021q/vlan_core.c +++ b/net/8021q/vlan_core.c @@ -1,12 +1,16 @@ #include <linux/skbuff.h> #include <linux/netdevice.h> #include <linux/if_vlan.h> +#include <linux/netpoll.h> #include "vlan.h" /* VLAN rx hw acceleration helper. This acts like netif_{rx,receive_skb}(). */ int __vlan_hwaccel_rx(struct sk_buff *skb, struct vlan_group *grp, u16 vlan_tci, int polling) { + if (netpoll_receive_skb(skb)) + return NET_RX_DROP; + if (skb_bond_should_drop(skb)) goto drop; @@ -100,6 +104,9 @@ int vlan_gro_receive(struct napi_struct *napi, struct vlan_group *grp, { int err = NET_RX_SUCCESS; + if (netpoll_receive_skb(skb)) + return NET_RX_DROP; + switch (vlan_gro_common(napi, grp, vlan_tci, skb)) { case -1: return netif_receive_skb(skb); @@ -126,6 +133,9 @@ int vlan_gro_frags(struct napi_struct *napi, struct vlan_group *grp, if (!skb) goto out; + if (netpoll_receive_skb(skb)) + goto out; + err = NET_RX_SUCCESS; switch (vlan_gro_common(napi, grp, vlan_tci, skb)) { diff --git a/net/core/dev.c b/net/core/dev.c index a17e006..72b0d26 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -2488,6 +2488,9 @@ static int __napi_gro_receive(struct napi_struct *napi, struct sk_buff *skb) int napi_gro_receive(struct napi_struct *napi, struct sk_buff *skb) { + if (netpoll_receive_skb(skb)) + return NET_RX_DROP; + switch (__napi_gro_receive(napi, skb)) { case -1: return netif_receive_skb(skb); @@ -2558,6 +2561,9 @@ int napi_gro_frags(struct napi_struct *napi, struct napi_gro_fraginfo *info) if (!skb) goto out; + if (netpoll_receive_skb(skb)) + goto out; + err = NET_RX_SUCCESS; switch (__napi_gro_receive(napi, skb)) {