| Message ID | 1292855730-19265-1-git-send-email-xiaosuo@gmail.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
Le lundi 20 décembre 2010 à 22:35 +0800, Changli Gao a écrit : > Pawel reported a panic related to handling shared skbs in ixgbe > incorrectly. So we need to revert my previous patch to work around > this bug. Instead of reverting the patch completely, I just revert > the essential lines, so we can add the previous optimization > back more easily in future. > > commit 3511c9132f8b1e1b5634e41a3331c44b0c13be70 > Author: Changli Gao <xiaosuo@gmail.com> > Date: Sat Oct 16 13:04:08 2010 +0000 > > net_sched: remove the unused parameter of qdisc_create_dflt() > > Reported-by: Pawel Staszewski <pstaszewski@itcare.pl> > Signed-off-by: Changli Gao <xiaosuo@gmail.com> Acked-by: Eric Dumazet <eric.dumazet@gmail.com> -- 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: Eric Dumazet <eric.dumazet@gmail.com> Date: Mon, 20 Dec 2010 15:37:56 +0100 > Le lundi 20 décembre 2010 à 22:35 +0800, Changli Gao a écrit : >> Pawel reported a panic related to handling shared skbs in ixgbe >> incorrectly. So we need to revert my previous patch to work around >> this bug. Instead of reverting the patch completely, I just revert >> the essential lines, so we can add the previous optimization >> back more easily in future. >> >> commit 3511c9132f8b1e1b5634e41a3331c44b0c13be70 >> Author: Changli Gao <xiaosuo@gmail.com> >> Date: Sat Oct 16 13:04:08 2010 +0000 >> >> net_sched: remove the unused parameter of qdisc_create_dflt() >> >> Reported-by: Pawel Staszewski <pstaszewski@itcare.pl> >> Signed-off-by: Changli Gao <xiaosuo@gmail.com> > > Acked-by: Eric Dumazet <eric.dumazet@gmail.com> Applied, thanks everyone. -- 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 Mon, Dec 20, 2010 at 10:35:30PM +0800, Changli Gao wrote: > Pawel reported a panic related to handling shared skbs in ixgbe > incorrectly. So we need to revert my previous patch to work around > this bug. Instead of reverting the patch completely, I just revert > the essential lines, so we can add the previous optimization > back more easily in future. > > commit 3511c9132f8b1e1b5634e41a3331c44b0c13be70 > Author: Changli Gao <xiaosuo@gmail.com> > Date: Sat Oct 16 13:04:08 2010 +0000 > > net_sched: remove the unused parameter of qdisc_create_dflt() > Isn't there some mistake in the commit number? IMHO this changelog is mostly wrong. The bug happens outside of ixgbe, probably in dev_hard_start_xmit() -> __skb_linearize(), and even if not, it can, without any driver's fault. The only question is why it didn't triger with 2.6.36. 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
Le mardi 21 décembre 2010 à 00:20 +0100, Jarek Poplawski a écrit : > On Mon, Dec 20, 2010 at 10:35:30PM +0800, Changli Gao wrote: > > Pawel reported a panic related to handling shared skbs in ixgbe > > incorrectly. So we need to revert my previous patch to work around > > this bug. Instead of reverting the patch completely, I just revert > > the essential lines, so we can add the previous optimization > > back more easily in future. > > > > commit 3511c9132f8b1e1b5634e41a3331c44b0c13be70 > > Author: Changli Gao <xiaosuo@gmail.com> > > Date: Sat Oct 16 13:04:08 2010 +0000 > > > > net_sched: remove the unused parameter of qdisc_create_dflt() > > > > Isn't there some mistake in the commit number? IMHO this changelog is > mostly wrong. The bug happens outside of ixgbe, probably in > dev_hard_start_xmit() -> __skb_linearize(), and even if not, it can, > without any driver's fault. The only question is why it didn't triger > with 2.6.36. > Indeed, commit number is wrong. It was : commit 210d6de78c5d7c785fc532556cea340e517955e1 Author: Changli Gao <xiaosuo@gmail.com> Date: Thu Jun 24 16:25:12 2010 +0000 act_mirred: don't clone skb when skb isn't shared It triggers now because GRO might be default to on now. commit 6a08d194ee40806e0ccd5f36ed768e64cbfc979f e1000: use GRO for receive (or other various GRO patches) -- 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 Tue, Dec 21, 2010 at 12:28:09AM +0100, Eric Dumazet wrote: > Le mardi 21 décembre 2010 ?? 00:20 +0100, Jarek Poplawski a écrit : > > On Mon, Dec 20, 2010 at 10:35:30PM +0800, Changli Gao wrote: > > > Pawel reported a panic related to handling shared skbs in ixgbe > > > incorrectly. So we need to revert my previous patch to work around > > > this bug. Instead of reverting the patch completely, I just revert > > > the essential lines, so we can add the previous optimization > > > back more easily in future. > > > > > > commit 3511c9132f8b1e1b5634e41a3331c44b0c13be70 > > > Author: Changli Gao <xiaosuo@gmail.com> > > > Date: Sat Oct 16 13:04:08 2010 +0000 > > > > > > net_sched: remove the unused parameter of qdisc_create_dflt() > > > > > > > Isn't there some mistake in the commit number? IMHO this changelog is > > mostly wrong. The bug happens outside of ixgbe, probably in > > dev_hard_start_xmit() -> __skb_linearize(), and even if not, it can, > > without any driver's fault. The only question is why it didn't triger > > with 2.6.36. > > > > Indeed, commit number is wrong. It was : > > commit 210d6de78c5d7c785fc532556cea340e517955e1 > Author: Changli Gao <xiaosuo@gmail.com> > Date: Thu Jun 24 16:25:12 2010 +0000 > > act_mirred: don't clone skb when skb isn't shared > > It triggers now because GRO might be default to on now. > > commit 6a08d194ee40806e0ccd5f36ed768e64cbfc979f > e1000: use GRO for receive > > (or other various GRO patches) Well, still some doubt after re-reading Pawel's 1st. message: host1 (kernel 2.6.36.2) netperf client -> eth3 (82598EB 10-Gigabit AT CX4) - directly connected to eth2 of host2 ethtool -k eth3 Offload parameters for eth3: rx-checksumming: on tx-checksumming: on scatter-gather: on tcp-segmentation-offload: on udp-fragmentation-offload: off generic-segmentation-offload: on generic-receive-offload: on ... Of course, I know this was another box. 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 Tue, Dec 21, 2010 at 7:28 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > > Indeed, commit number is wrong. It was : > > commit 210d6de78c5d7c785fc532556cea340e517955e1 > Author: Changli Gao <xiaosuo@gmail.com> > Date: Thu Jun 24 16:25:12 2010 +0000 > > act_mirred: don't clone skb when skb isn't shared > > It triggers now because GRO might be default to on now. > > commit 6a08d194ee40806e0ccd5f36ed768e64cbfc979f > e1000: use GRO for receive > > (or other various GRO patches) > > Oops. It is all my fault. Sorry. What can I do to fix this after David has applied it and pushed it out?
From: Changli Gao <xiaosuo@gmail.com> Date: Tue, 21 Dec 2010 08:21:41 +0800 > Oops. It is all my fault. Sorry. What can I do to fix this after David > has applied it and pushed it out? Don't worry about it, mistakes happen. -- 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 Tue, 2010-12-21 at 00:52 +0100, Jarek Poplawski wrote: > > host1 (kernel 2.6.36.2) > netperf client -> eth3 (82598EB 10-Gigabit AT CX4) - directly connected to eth2 of host2 > ethtool -k eth3 > Offload parameters for eth3: > rx-checksumming: on > tx-checksumming: on > scatter-gather: on > tcp-segmentation-offload: on > udp-fragmentation-offload: off > generic-segmentation-offload: on > generic-receive-offload: on Use to be we couldnt get the ifb+mirred combo to work with TSO even without this change - i will have to dig old emails to remember details. So we are making progress;-> I did write a set of rules in: Documentation/networking/tc-actions-env-rules.txt Changli optimized rule #1. When i looked at his patch it seems to not harm that case. Sometimes dumb is a good principle ;-> cheers, jamal -- 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 Tue, Dec 21, 2010 at 9:52 PM, jamal <hadi@cyberus.ca> wrote: > > Use to be we couldnt get the ifb+mirred combo to work > with TSO even without this change - i will have to dig old emails > to remember details. So we are making progress;-> I did write a set > of rules in: Documentation/networking/tc-actions-env-rules.txt > Changli optimized rule #1. When i looked at his patch it seems > to not harm that case. Sometimes dumb is a good principle ;-> > In order to make my trick work. We need to assure dev_queue_xmit() and dev_hard_start_xmit() accept shared skbs. As Eric pointed, pktgen also need dev->netdev_ops->ndo_start_xmit() accept shared skbs. We need to fix every ndo_start_xmit() one by one, then dev_hard_start_xmit(), and when dev_queue_xmit() is also fixed, my trick can be added back. :)
On Tue, Dec 21, 2010 at 10:17:35PM +0800, Changli Gao wrote: > On Tue, Dec 21, 2010 at 9:52 PM, jamal <hadi@cyberus.ca> wrote: > > > > Use to be we couldnt get the ifb+mirred combo to work > > with TSO even without this change - i will have to dig old emails > > to remember details. So we are making progress;-> I did write a set > > of rules in: Documentation/networking/tc-actions-env-rules.txt > > Changli optimized rule #1. When i looked at his patch it seems > > to not harm that case. Sometimes dumb is a good principle ;-> Actually, when dumb isn't a good principle? ;-> Speaking about wrong things like optimizing this 1st commandment: it seems, if we're stealing not shared skb it's not unreasonable to tell others not to touch it anymore, and skip kfree_skb() in handle_ing(), unless I miss something? > In order to make my trick work. We need to assure dev_queue_xmit() and > dev_hard_start_xmit() accept shared skbs. As Eric pointed, pktgen also > need dev->netdev_ops->ndo_start_xmit() accept shared skbs. We need to > fix every ndo_start_xmit() one by one, then dev_hard_start_xmit(), and > when dev_queue_xmit() is also fixed, my trick can be added back. > :) I'm not sure pktgen is right - even if it's safe in its case, it seems to break some older rules, and drivers should really own the things. So it needs reviewing first. Cheers, 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 Tue, 2010-12-21 at 23:37 +0100, Jarek Poplawski wrote: > Actually, when dumb isn't a good principle? ;-> > > Speaking about wrong things like optimizing this 1st commandment: it > seems, if we're stealing not shared skb it's not unreasonable to tell > others not to touch it anymore, and skip kfree_skb() in handle_ing(), > unless I miss something? I dont know how much cycles it will save you vs the code you add but it is not unreasonable. Of course it would mean we are moving beyond KISSes[1] now Jarek;-> > > In order to make my trick work. We need to assure dev_queue_xmit() and > > dev_hard_start_xmit() accept shared skbs. As Eric pointed, pktgen also > > need dev->netdev_ops->ndo_start_xmit() accept shared skbs. We need to > > fix every ndo_start_xmit() one by one, then dev_hard_start_xmit(), and > > when dev_queue_xmit() is also fixed, my trick can be added back. > > :) > > I'm not sure pktgen is right - even if it's safe in its case, it seems > to break some older rules, and drivers should really own the things. > So it needs reviewing first. The idea we always try to keep is that the caller is responsible for the fate of the skb it sent. The callee will do things like make a copy or clone if it needed to make changes or keep the skb. Same should be expected of a driver. cheers, jamal [1] Jarek - so you dont misunderstand this as an overture - it means Keep It Simple Silly (or the principle of dumb we talked about) ;-> -- 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 Thu, Dec 23, 2010 at 08:35:33AM -0500, jamal wrote: > On Tue, 2010-12-21 at 23:37 +0100, Jarek Poplawski wrote: > > > Actually, when dumb isn't a good principle? ;-> > > > > Speaking about wrong things like optimizing this 1st commandment: it > > seems, if we're stealing not shared skb it's not unreasonable to tell > > others not to touch it anymore, and skip kfree_skb() in handle_ing(), > > unless I miss something? > > I dont know how much cycles it will save you vs the code you add > but it is not unreasonable. Of course it would mean we are moving > beyond KISSes[1] now Jarek;-> Hmm... seeing all those multiqued multiques I thought everybody was beyond, but no problem, let's go back to KISSes[2] ;-) > > > In order to make my trick work. We need to assure dev_queue_xmit() and > > > dev_hard_start_xmit() accept shared skbs. As Eric pointed, pktgen also > > > need dev->netdev_ops->ndo_start_xmit() accept shared skbs. We need to > > > fix every ndo_start_xmit() one by one, then dev_hard_start_xmit(), and > > > when dev_queue_xmit() is also fixed, my trick can be added back. > > > :) > > > > I'm not sure pktgen is right - even if it's safe in its case, it seems > > to break some older rules, and drivers should really own the things. > > So it needs reviewing first. > > The idea we always try to keep is that the caller is responsible for the > fate of the skb it sent. The callee will do things like make a copy or > clone if it needed to make changes or keep the skb. Same should be > expected of a driver. Well, I'm not sure we talk about the same thing. I mean e.g. the comment above dev_queue_xmit(); the caller passes all control to the callee here. Cheers, Jarek P. > [1] Jarek - so you dont misunderstand this as an overture - it means > Keep It Simple Silly (or the principle of dumb we talked about) ;-> OK, I can remember the seventies too ;-) -- 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/include/net/sch_generic.h b/include/net/sch_generic.h index 786cc39..0af57eb 100644 --- a/include/net/sch_generic.h +++ b/include/net/sch_generic.h @@ -611,11 +611,7 @@ static inline struct sk_buff *skb_act_clone(struct sk_buff *skb, gfp_t gfp_mask, { struct sk_buff *n; - if ((action == TC_ACT_STOLEN || action == TC_ACT_QUEUED) && - !skb_shared(skb)) - n = skb_get(skb); - else - n = skb_clone(skb, gfp_mask); + n = skb_clone(skb, gfp_mask); if (n) { n->tc_verd = SET_TC_VERD(n->tc_verd, 0);
Pawel reported a panic related to handling shared skbs in ixgbe incorrectly. So we need to revert my previous patch to work around this bug. Instead of reverting the patch completely, I just revert the essential lines, so we can add the previous optimization back more easily in future. commit 3511c9132f8b1e1b5634e41a3331c44b0c13be70 Author: Changli Gao <xiaosuo@gmail.com> Date: Sat Oct 16 13:04:08 2010 +0000 net_sched: remove the unused parameter of qdisc_create_dflt() Reported-by: Pawel Staszewski <pstaszewski@itcare.pl> Signed-off-by: Changli Gao <xiaosuo@gmail.com> --- include/net/sch_generic.h | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) -- 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