Message ID | 20110720151827.GD12349@hmsreliant.think-freely.org |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
Le mercredi 20 juillet 2011 à 11:18 -0400, Neil Horman a écrit : > On Wed, Jul 20, 2011 at 06:24:15AM +0200, Eric Dumazet wrote: > > Le mardi 19 juillet 2011 à 22:07 -0400, Neil Horman a écrit : > > > > > > > I think this is a good idea. It lets pktgen dynamically make the clone/share > > > decision dynamically and only impacts performance for those systems. > > > > > > > Just let pktgen refuse to use clone_skb command for these devices. > > > copy that, This is by no means final, but what do you think of this? If its > agreeable to you, Ben, et al. I can add this to my local tree and start auditing > all the drivers that may need to have the flag set. > > Regards > Neil > > > diff --git a/include/linux/if.h b/include/linux/if.h > index 3bc63e6..ae904fe 100644 > --- a/include/linux/if.h > +++ b/include/linux/if.h > @@ -76,6 +76,7 @@ > #define IFF_BRIDGE_PORT 0x4000 /* device used as bridge port */ > #define IFF_OVS_DATAPATH 0x8000 /* device used as Open vSwitch > * datapath port */ > +#define IFF_CANT_SHARE_SKB 0x10000 /* Device can't share skbs in tx path */ > > #define IF_GET_IFACE 0x0001 /* for querying only */ > #define IF_GET_PROTO 0x0002 > diff --git a/net/core/pktgen.c b/net/core/pktgen.c > index f76079c..bf6d88d 100644 > --- a/net/core/pktgen.c > +++ b/net/core/pktgen.c > @@ -1071,6 +1071,9 @@ static ssize_t pktgen_if_write(struct file *file, > if (len < 0) > return len; > > + if (pkt_dev->priv_flags & IFF_CANT_SHARE_SKB) > + return -EOPNOTSUPP; > + Well, the general idea was to intercept the "clone_skb XXX" command and cap XXX to 0 for said devices. So some admin can still use pktgen without clone_skb stuff. -- 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
2011/7/20 Neil Horman <nhorman@tuxdriver.com>: > On Wed, Jul 20, 2011 at 06:24:15AM +0200, Eric Dumazet wrote: >> Le mardi 19 juillet 2011 à 22:07 -0400, Neil Horman a écrit : >> > > >> > I think this is a good idea. It lets pktgen dynamically make the clone/share >> > decision dynamically and only impacts performance for those systems. >> > >> >> Just let pktgen refuse to use clone_skb command for these devices. >> > copy that, This is by no means final, but what do you think of this? If its > agreeable to you, Ben, et al. I can add this to my local tree and start auditing > all the drivers that may need to have the flag set. > > Regards > Neil > > > diff --git a/include/linux/if.h b/include/linux/if.h > index 3bc63e6..ae904fe 100644 > --- a/include/linux/if.h > +++ b/include/linux/if.h > @@ -76,6 +76,7 @@ > #define IFF_BRIDGE_PORT 0x4000 /* device used as bridge port */ > #define IFF_OVS_DATAPATH 0x8000 /* device used as Open vSwitch > * datapath port */ > +#define IFF_CANT_SHARE_SKB 0x10000 /* Device can't share skbs in tx path */ > > #define IF_GET_IFACE 0x0001 /* for querying only */ > #define IF_GET_PROTO 0x0002 > diff --git a/net/core/pktgen.c b/net/core/pktgen.c > index f76079c..bf6d88d 100644 > --- a/net/core/pktgen.c > +++ b/net/core/pktgen.c > @@ -1071,6 +1071,9 @@ static ssize_t pktgen_if_write(struct file *file, > if (len < 0) > return len; > > + if (pkt_dev->priv_flags & IFF_CANT_SHARE_SKB) > + return -EOPNOTSUPP; > + > i += len; > pkt_dev->clone_skb = value; > I would prefer that the flag be inclusive, i.e. it should mark drivers which can use shared skbs. And it might be better to clone the skb in case the flag is disabled to keep functionality working. Best Regards, Michał Mirosław -- 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 Wed, Jul 20, 2011 at 05:30:17PM +0200, Eric Dumazet wrote: > Le mercredi 20 juillet 2011 à 11:18 -0400, Neil Horman a écrit : > > On Wed, Jul 20, 2011 at 06:24:15AM +0200, Eric Dumazet wrote: > > > Le mardi 19 juillet 2011 à 22:07 -0400, Neil Horman a écrit : > > > > > > > > > I think this is a good idea. It lets pktgen dynamically make the clone/share > > > > decision dynamically and only impacts performance for those systems. > > > > > > > > > > Just let pktgen refuse to use clone_skb command for these devices. > > > > > copy that, This is by no means final, but what do you think of this? If its > > agreeable to you, Ben, et al. I can add this to my local tree and start auditing > > all the drivers that may need to have the flag set. > > > > Regards > > Neil > > > > > > diff --git a/include/linux/if.h b/include/linux/if.h > > index 3bc63e6..ae904fe 100644 > > --- a/include/linux/if.h > > +++ b/include/linux/if.h > > @@ -76,6 +76,7 @@ > > #define IFF_BRIDGE_PORT 0x4000 /* device used as bridge port */ > > #define IFF_OVS_DATAPATH 0x8000 /* device used as Open vSwitch > > * datapath port */ > > +#define IFF_CANT_SHARE_SKB 0x10000 /* Device can't share skbs in tx path */ > > > > #define IF_GET_IFACE 0x0001 /* for querying only */ > > #define IF_GET_PROTO 0x0002 > > diff --git a/net/core/pktgen.c b/net/core/pktgen.c > > index f76079c..bf6d88d 100644 > > --- a/net/core/pktgen.c > > +++ b/net/core/pktgen.c > > @@ -1071,6 +1071,9 @@ static ssize_t pktgen_if_write(struct file *file, > > if (len < 0) > > return len; > > > > + if (pkt_dev->priv_flags & IFF_CANT_SHARE_SKB) > > + return -EOPNOTSUPP; > > + > > Well, the general idea was to intercept the "clone_skb XXX" command and > cap XXX to 0 for said devices. > Isn't that exactly what this does? Disallows a user from issuing the cone_skb command in a pktgen script if the underlying driver can't support the sharing of skbs. > So some admin can still use pktgen without clone_skb stuff. > Yes. this doesn't preclude that unless you see something I dont Neil > > -- 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 Wed, Jul 20, 2011 at 05:35:47PM +0200, Michał Mirosław wrote: > 2011/7/20 Neil Horman <nhorman@tuxdriver.com>: > > On Wed, Jul 20, 2011 at 06:24:15AM +0200, Eric Dumazet wrote: > >> Le mardi 19 juillet 2011 à 22:07 -0400, Neil Horman a écrit : > >> > > > >> > I think this is a good idea. It lets pktgen dynamically make the clone/share > >> > decision dynamically and only impacts performance for those systems. > >> > > >> > >> Just let pktgen refuse to use clone_skb command for these devices. > >> > > copy that, This is by no means final, but what do you think of this? If its > > agreeable to you, Ben, et al. I can add this to my local tree and start auditing > > all the drivers that may need to have the flag set. > > > > Regards > > Neil > > > > > > diff --git a/include/linux/if.h b/include/linux/if.h > > index 3bc63e6..ae904fe 100644 > > --- a/include/linux/if.h > > +++ b/include/linux/if.h > > @@ -76,6 +76,7 @@ > > #define IFF_BRIDGE_PORT 0x4000 /* device used as bridge port */ > > #define IFF_OVS_DATAPATH 0x8000 /* device used as Open vSwitch > > * datapath port */ > > +#define IFF_CANT_SHARE_SKB 0x10000 /* Device can't share skbs in tx path */ > > > > #define IF_GET_IFACE 0x0001 /* for querying only */ > > #define IF_GET_PROTO 0x0002 > > diff --git a/net/core/pktgen.c b/net/core/pktgen.c > > index f76079c..bf6d88d 100644 > > --- a/net/core/pktgen.c > > +++ b/net/core/pktgen.c > > @@ -1071,6 +1071,9 @@ static ssize_t pktgen_if_write(struct file *file, > > if (len < 0) > > return len; > > > > + if (pkt_dev->priv_flags & IFF_CANT_SHARE_SKB) > > + return -EOPNOTSUPP; > > + > > i += len; > > pkt_dev->clone_skb = value; > > > > I would prefer that the flag be inclusive, i.e. it should mark drivers > which can use shared skbs. And it might be better to clone the skb in > case the flag is disabled to keep functionality working. > Ok, I can agree with that. But you're ok with the general approach? Neil > Best Regards, > Michał Mirosław > -- 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 mercredi 20 juillet 2011 à 11:39 -0400, Neil Horman a écrit : > Isn't that exactly what this does? Disallows a user from issuing the cone_skb > command in a pktgen script if the underlying driver can't support the sharing of > skbs. My bad, I misread your patch. Seems fine to me ;) -- 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
W dniu 20 lipca 2011 17:40 użytkownik Neil Horman <nhorman@tuxdriver.com> napisał: > On Wed, Jul 20, 2011 at 05:35:47PM +0200, Michał Mirosław wrote: >> 2011/7/20 Neil Horman <nhorman@tuxdriver.com>: >> > On Wed, Jul 20, 2011 at 06:24:15AM +0200, Eric Dumazet wrote: >> >> Le mardi 19 juillet 2011 à 22:07 -0400, Neil Horman a écrit : >> >> > > >> >> > I think this is a good idea. It lets pktgen dynamically make the clone/share >> >> > decision dynamically and only impacts performance for those systems. >> >> > >> >> >> >> Just let pktgen refuse to use clone_skb command for these devices. >> >> >> > copy that, This is by no means final, but what do you think of this? If its >> > agreeable to you, Ben, et al. I can add this to my local tree and start auditing >> > all the drivers that may need to have the flag set. >> > >> > Regards >> > Neil >> > >> > >> > diff --git a/include/linux/if.h b/include/linux/if.h >> > index 3bc63e6..ae904fe 100644 >> > --- a/include/linux/if.h >> > +++ b/include/linux/if.h >> > @@ -76,6 +76,7 @@ >> > #define IFF_BRIDGE_PORT 0x4000 /* device used as bridge port */ >> > #define IFF_OVS_DATAPATH 0x8000 /* device used as Open vSwitch >> > * datapath port */ >> > +#define IFF_CANT_SHARE_SKB 0x10000 /* Device can't share skbs in tx path */ >> > >> > #define IF_GET_IFACE 0x0001 /* for querying only */ >> > #define IF_GET_PROTO 0x0002 >> > diff --git a/net/core/pktgen.c b/net/core/pktgen.c >> > index f76079c..bf6d88d 100644 >> > --- a/net/core/pktgen.c >> > +++ b/net/core/pktgen.c >> > @@ -1071,6 +1071,9 @@ static ssize_t pktgen_if_write(struct file *file, >> > if (len < 0) >> > return len; >> > >> > + if (pkt_dev->priv_flags & IFF_CANT_SHARE_SKB) >> > + return -EOPNOTSUPP; >> > + >> > i += len; >> > pkt_dev->clone_skb = value; >> > >> >> I would prefer that the flag be inclusive, i.e. it should mark drivers >> which can use shared skbs. And it might be better to clone the skb in >> case the flag is disabled to keep functionality working. > Ok, I can agree with that. But you're ok with the general approach? I assumed you wanted to use netdev->features and I noticed priv_flags just now. If the flag is supposed to be permanent then its fine by me. Actually this makes me wonder if NETIF_F_LLTX and similar should get moved to netdev->priv_flags. Best Regards, Michał Mirosław -- 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 Wed, Jul 20, 2011 at 06:08:39PM +0200, Michał Mirosław wrote: > W dniu 20 lipca 2011 17:40 użytkownik Neil Horman > <nhorman@tuxdriver.com> napisał: > > On Wed, Jul 20, 2011 at 05:35:47PM +0200, Michał Mirosław wrote: > >> 2011/7/20 Neil Horman <nhorman@tuxdriver.com>: > >> > On Wed, Jul 20, 2011 at 06:24:15AM +0200, Eric Dumazet wrote: > >> >> Le mardi 19 juillet 2011 à 22:07 -0400, Neil Horman a écrit : > >> >> > > > >> >> > I think this is a good idea. It lets pktgen dynamically make the clone/share > >> >> > decision dynamically and only impacts performance for those systems. > >> >> > > >> >> > >> >> Just let pktgen refuse to use clone_skb command for these devices. > >> >> > >> > copy that, This is by no means final, but what do you think of this? If its > >> > agreeable to you, Ben, et al. I can add this to my local tree and start auditing > >> > all the drivers that may need to have the flag set. > >> > > >> > Regards > >> > Neil > >> > > >> > > >> > diff --git a/include/linux/if.h b/include/linux/if.h > >> > index 3bc63e6..ae904fe 100644 > >> > --- a/include/linux/if.h > >> > +++ b/include/linux/if.h > >> > @@ -76,6 +76,7 @@ > >> > #define IFF_BRIDGE_PORT 0x4000 /* device used as bridge port */ > >> > #define IFF_OVS_DATAPATH 0x8000 /* device used as Open vSwitch > >> > * datapath port */ > >> > +#define IFF_CANT_SHARE_SKB 0x10000 /* Device can't share skbs in tx path */ > >> > > >> > #define IF_GET_IFACE 0x0001 /* for querying only */ > >> > #define IF_GET_PROTO 0x0002 > >> > diff --git a/net/core/pktgen.c b/net/core/pktgen.c > >> > index f76079c..bf6d88d 100644 > >> > --- a/net/core/pktgen.c > >> > +++ b/net/core/pktgen.c > >> > @@ -1071,6 +1071,9 @@ static ssize_t pktgen_if_write(struct file *file, > >> > if (len < 0) > >> > return len; > >> > > >> > + if (pkt_dev->priv_flags & IFF_CANT_SHARE_SKB) > >> > + return -EOPNOTSUPP; > >> > + > >> > i += len; > >> > pkt_dev->clone_skb = value; > >> > > >> > >> I would prefer that the flag be inclusive, i.e. it should mark drivers > >> which can use shared skbs. And it might be better to clone the skb in > >> case the flag is disabled to keep functionality working. > > Ok, I can agree with that. But you're ok with the general approach? > > I assumed you wanted to use netdev->features and I noticed priv_flags > just now. If the flag is supposed to be permanent then its fine by me. > Actually this makes me wonder if NETIF_F_LLTX and similar should get > moved to netdev->priv_flags. > Yeah, I expect it will be both permanent and invisible to user space. Neil > Best Regards, > Michał Mirosław > -- 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 Wed, Jul 20, 2011 at 05:44:48PM +0200, Eric Dumazet wrote: > Le mercredi 20 juillet 2011 à 11:39 -0400, Neil Horman a écrit : > > > Isn't that exactly what this does? Disallows a user from issuing the cone_skb > > command in a pktgen script if the underlying driver can't support the sharing of > > skbs. > > My bad, I misread your patch. Seems fine to me ;) > Ok, thanks. I'll put this in a local tree and stat auditing drivers to see who needs it set/cleared. I'll post a new series when I have it done. Thanks! Neil -- 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 07/20/2011 08:18 AM, Neil Horman wrote: > On Wed, Jul 20, 2011 at 06:24:15AM +0200, Eric Dumazet wrote: >> Le mardi 19 juillet 2011 à 22:07 -0400, Neil Horman a écrit : >>>> >>> I think this is a good idea. It lets pktgen dynamically make the clone/share >>> decision dynamically and only impacts performance for those systems. >>> >> >> Just let pktgen refuse to use clone_skb command for these devices. >> > copy that, This is by no means final, but what do you think of this? If its > agreeable to you, Ben, et al. I can add this to my local tree and start auditing > all the drivers that may need to have the flag set. > > Regards > Neil > > > diff --git a/include/linux/if.h b/include/linux/if.h > index 3bc63e6..ae904fe 100644 > --- a/include/linux/if.h > +++ b/include/linux/if.h > @@ -76,6 +76,7 @@ > #define IFF_BRIDGE_PORT 0x4000 /* device used as bridge port */ > #define IFF_OVS_DATAPATH 0x8000 /* device used as Open vSwitch > * datapath port */ > +#define IFF_CANT_SHARE_SKB 0x10000 /* Device can't share skbs in tx path */ > > #define IF_GET_IFACE 0x0001 /* for querying only */ > #define IF_GET_PROTO 0x0002 > diff --git a/net/core/pktgen.c b/net/core/pktgen.c > index f76079c..bf6d88d 100644 > --- a/net/core/pktgen.c > +++ b/net/core/pktgen.c > @@ -1071,6 +1071,9 @@ static ssize_t pktgen_if_write(struct file *file, > if (len< 0) > return len; > > + if (pkt_dev->priv_flags& IFF_CANT_SHARE_SKB) > + return -EOPNOTSUPP; > + > i += len; > pkt_dev->clone_skb = value; > Please only return an error if value > 1. Also, if there is any place where user can configure pkt_dev, you would want code there to check for the CANT_SHARE_SKB flag and force clone_skb to zero if user changes to a different device that cannot share skbs. Thanks, Ben
On Wed, Jul 20, 2011 at 09:33:20AM -0700, Ben Greear wrote: > On 07/20/2011 08:18 AM, Neil Horman wrote: > >On Wed, Jul 20, 2011 at 06:24:15AM +0200, Eric Dumazet wrote: > >>Le mardi 19 juillet 2011 à 22:07 -0400, Neil Horman a écrit : > >>>> > >>>I think this is a good idea. It lets pktgen dynamically make the clone/share > >>>decision dynamically and only impacts performance for those systems. > >>> > >> > >>Just let pktgen refuse to use clone_skb command for these devices. > >> > >copy that, This is by no means final, but what do you think of this? If its > >agreeable to you, Ben, et al. I can add this to my local tree and start auditing > >all the drivers that may need to have the flag set. > > > >Regards > >Neil > > > > > >diff --git a/include/linux/if.h b/include/linux/if.h > >index 3bc63e6..ae904fe 100644 > >--- a/include/linux/if.h > >+++ b/include/linux/if.h > >@@ -76,6 +76,7 @@ > > #define IFF_BRIDGE_PORT 0x4000 /* device used as bridge port */ > > #define IFF_OVS_DATAPATH 0x8000 /* device used as Open vSwitch > > * datapath port */ > >+#define IFF_CANT_SHARE_SKB 0x10000 /* Device can't share skbs in tx path */ > > > > #define IF_GET_IFACE 0x0001 /* for querying only */ > > #define IF_GET_PROTO 0x0002 > >diff --git a/net/core/pktgen.c b/net/core/pktgen.c > >index f76079c..bf6d88d 100644 > >--- a/net/core/pktgen.c > >+++ b/net/core/pktgen.c > >@@ -1071,6 +1071,9 @@ static ssize_t pktgen_if_write(struct file *file, > > if (len< 0) > > return len; > > > >+ if (pkt_dev->priv_flags& IFF_CANT_SHARE_SKB) > >+ return -EOPNOTSUPP; > >+ > > i += len; > > pkt_dev->clone_skb = value; > > > > Please only return an error if value > 1. > > Also, if there is any place where user can configure pkt_dev, > you would want code there to check for the CANT_SHARE_SKB flag > and force clone_skb to zero if user changes to a different > device that cannot share skbs. > > Thanks, > Ben > Understood, thank you Ben. Neil > -- > Ben Greear <greearb@candelatech.com> > Candela Technologies Inc http://www.candelatech.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
On 07/20/2011 08:35 AM, Michał Mirosław wrote: > 2011/7/20 Neil Horman<nhorman@tuxdriver.com>: >> On Wed, Jul 20, 2011 at 06:24:15AM +0200, Eric Dumazet wrote: >>> Le mardi 19 juillet 2011 à 22:07 -0400, Neil Horman a écrit : >>>>> >>>> I think this is a good idea. It lets pktgen dynamically make the clone/share >>>> decision dynamically and only impacts performance for those systems. >>>> >>> >>> Just let pktgen refuse to use clone_skb command for these devices. >>> >> copy that, This is by no means final, but what do you think of this? If its >> agreeable to you, Ben, et al. I can add this to my local tree and start auditing >> all the drivers that may need to have the flag set. >> >> Regards >> Neil >> >> >> diff --git a/include/linux/if.h b/include/linux/if.h >> index 3bc63e6..ae904fe 100644 >> --- a/include/linux/if.h >> +++ b/include/linux/if.h >> @@ -76,6 +76,7 @@ >> #define IFF_BRIDGE_PORT 0x4000 /* device used as bridge port */ >> #define IFF_OVS_DATAPATH 0x8000 /* device used as Open vSwitch >> * datapath port */ >> +#define IFF_CANT_SHARE_SKB 0x10000 /* Device can't share skbs in tx path */ >> >> #define IF_GET_IFACE 0x0001 /* for querying only */ >> #define IF_GET_PROTO 0x0002 >> diff --git a/net/core/pktgen.c b/net/core/pktgen.c >> index f76079c..bf6d88d 100644 >> --- a/net/core/pktgen.c >> +++ b/net/core/pktgen.c >> @@ -1071,6 +1071,9 @@ static ssize_t pktgen_if_write(struct file *file, >> if (len< 0) >> return len; >> >> + if (pkt_dev->priv_flags& IFF_CANT_SHARE_SKB) >> + return -EOPNOTSUPP; >> + >> i += len; >> pkt_dev->clone_skb = value; >> > > I would prefer that the flag be inclusive, i.e. it should mark drivers > which can use shared skbs. And it might be better to clone the skb in > case the flag is disabled to keep functionality working. The whole point of clone-skb is for speedup due to not allocating new memory..so if the netdev can't support it, lets just fail instead of giving user false sense of it working... User can just use clone-skb value of 0 in that case... Thanks, Ben > > Best Regards, > Michał Mirosław > -- > 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: Neil Horman <nhorman@tuxdriver.com> Date: Wed, 20 Jul 2011 11:18:27 -0400 > On Wed, Jul 20, 2011 at 06:24:15AM +0200, Eric Dumazet wrote: >> Le mardi 19 juillet 2011 à 22:07 -0400, Neil Horman a écrit : >> > > >> > I think this is a good idea. It lets pktgen dynamically make the clone/share >> > decision dynamically and only impacts performance for those systems. >> > >> >> Just let pktgen refuse to use clone_skb command for these devices. >> > copy that, This is by no means final, but what do you think of this? If its > agreeable to you, Ben, et al. I can add this to my local tree and start auditing > all the drivers that may need to have the flag set. I think there is a much simpler solution. Set a flag in the SKB when pktgen does SKB sharing. In dev_queue_xmit() (or perhaps, dev_hard_start_xmit()), check the flag and if it's set then we copy the SKB. If this works, then we fix the crash and no driver changes are necessary both now and in the future. -- 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 07/21/2011 03:01 PM, David Miller wrote: > From: Neil Horman<nhorman@tuxdriver.com> > Date: Wed, 20 Jul 2011 11:18:27 -0400 > >> On Wed, Jul 20, 2011 at 06:24:15AM +0200, Eric Dumazet wrote: >>> Le mardi 19 juillet 2011 à 22:07 -0400, Neil Horman a écrit : >>>>> >>>> I think this is a good idea. It lets pktgen dynamically make the clone/share >>>> decision dynamically and only impacts performance for those systems. >>>> >>> >>> Just let pktgen refuse to use clone_skb command for these devices. >>> >> copy that, This is by no means final, but what do you think of this? If its >> agreeable to you, Ben, et al. I can add this to my local tree and start auditing >> all the drivers that may need to have the flag set. > > I think there is a much simpler solution. > > Set a flag in the SKB when pktgen does SKB sharing. > > In dev_queue_xmit() (or perhaps, dev_hard_start_xmit()), check the flag > and if it's set then we copy the SKB. > > If this works, then we fix the crash and no driver changes are > necessary both now and in the future. Doesn't that make clone-skb in pktgen much less efficient in all cases? Thanks, Ben
From: Ben Greear <greearb@candelatech.com> Date: Thu, 21 Jul 2011 15:14:32 -0700 > On 07/21/2011 03:01 PM, David Miller wrote: >> From: Neil Horman<nhorman@tuxdriver.com> >> Date: Wed, 20 Jul 2011 11:18:27 -0400 >> >>> On Wed, Jul 20, 2011 at 06:24:15AM +0200, Eric Dumazet wrote: >>>> Le mardi 19 juillet 2011 à 22:07 -0400, Neil Horman a écrit : >>>>>> >>>>> I think this is a good idea. It lets pktgen dynamically make the >>>>> clone/share >>>>> decision dynamically and only impacts performance for those systems. >>>>> >>>> >>>> Just let pktgen refuse to use clone_skb command for these devices. >>>> >>> copy that, This is by no means final, but what do you think of this? >>> If its >>> agreeable to you, Ben, et al. I can add this to my local tree and >>> start auditing >>> all the drivers that may need to have the flag set. >> >> I think there is a much simpler solution. >> >> Set a flag in the SKB when pktgen does SKB sharing. >> >> In dev_queue_xmit() (or perhaps, dev_hard_start_xmit()), check the >> flag >> and if it's set then we copy the SKB. >> >> If this works, then we fix the crash and no driver changes are >> necessary both now and in the future. > > Doesn't that make clone-skb in pktgen much less efficient > in all cases? No, the copy only happens if we enter dev_queue_xmit() which pktgen doesn't do, it calls the driver's ->ndo_start_xmit() method directly. That's the whole idea. Only these encapsulating software devices will trigger the condition. -- 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 07/21/2011 03:19 PM, David Miller wrote: > From: Ben Greear<greearb@candelatech.com> > Date: Thu, 21 Jul 2011 15:14:32 -0700 > >> On 07/21/2011 03:01 PM, David Miller wrote: >>> From: Neil Horman<nhorman@tuxdriver.com> >>> Date: Wed, 20 Jul 2011 11:18:27 -0400 >>> >>>> On Wed, Jul 20, 2011 at 06:24:15AM +0200, Eric Dumazet wrote: >>>>> Le mardi 19 juillet 2011 à 22:07 -0400, Neil Horman a écrit : >>>>>>> >>>>>> I think this is a good idea. It lets pktgen dynamically make the >>>>>> clone/share >>>>>> decision dynamically and only impacts performance for those systems. >>>>>> >>>>> >>>>> Just let pktgen refuse to use clone_skb command for these devices. >>>>> >>>> copy that, This is by no means final, but what do you think of this? >>>> If its >>>> agreeable to you, Ben, et al. I can add this to my local tree and >>>> start auditing >>>> all the drivers that may need to have the flag set. >>> >>> I think there is a much simpler solution. >>> >>> Set a flag in the SKB when pktgen does SKB sharing. >>> >>> In dev_queue_xmit() (or perhaps, dev_hard_start_xmit()), check the >>> flag >>> and if it's set then we copy the SKB. >>> >>> If this works, then we fix the crash and no driver changes are >>> necessary both now and in the future. >> >> Doesn't that make clone-skb in pktgen much less efficient >> in all cases? > > No, the copy only happens if we enter dev_queue_xmit() which pktgen > doesn't do, it calls the driver's ->ndo_start_xmit() method directly. > > That's the whole idea. Only these encapsulating software devices > will trigger the condition. It seems there may be some Ethernet drivers that have similar issues, though I don't know of any personally (many years ago, Chelsio 10G NICs had this issue, but it may be fixed now.) But, it should be no worse than what we have today, and now that I better understand what you suggest, it sounds OK to me. Thanks, Ben
On Thu, Jul 21, 2011 at 03:19:03PM -0700, David Miller wrote: > From: Ben Greear <greearb@candelatech.com> > Date: Thu, 21 Jul 2011 15:14:32 -0700 > > > On 07/21/2011 03:01 PM, David Miller wrote: > >> From: Neil Horman<nhorman@tuxdriver.com> > >> Date: Wed, 20 Jul 2011 11:18:27 -0400 > >> > >>> On Wed, Jul 20, 2011 at 06:24:15AM +0200, Eric Dumazet wrote: > >>>> Le mardi 19 juillet 2011 à 22:07 -0400, Neil Horman a écrit : > >>>>>> > >>>>> I think this is a good idea. It lets pktgen dynamically make the > >>>>> clone/share > >>>>> decision dynamically and only impacts performance for those systems. > >>>>> > >>>> > >>>> Just let pktgen refuse to use clone_skb command for these devices. > >>>> > >>> copy that, This is by no means final, but what do you think of this? > >>> If its > >>> agreeable to you, Ben, et al. I can add this to my local tree and > >>> start auditing > >>> all the drivers that may need to have the flag set. > >> > >> I think there is a much simpler solution. > >> > >> Set a flag in the SKB when pktgen does SKB sharing. > >> > >> In dev_queue_xmit() (or perhaps, dev_hard_start_xmit()), check the > >> flag > >> and if it's set then we copy the SKB. > >> > >> If this works, then we fix the crash and no driver changes are > >> necessary both now and in the future. > > > > Doesn't that make clone-skb in pktgen much less efficient > > in all cases? > > No, the copy only happens if we enter dev_queue_xmit() which pktgen > doesn't do, it calls the driver's ->ndo_start_xmit() method directly. > > That's the whole idea. Only these encapsulating software devices > will trigger the condition. > I'm happy to go down this route Dave, and agree, its a more solid solution, but I think the problem with it (which Ben may have been alluding to previously) is that pktgen doesn't use dev_queue_xmit or dev_hard_start_xmit to send frames. It mimics the locking of dev_hard_start_xmit (but ignores the other checks that function does), and just calls the ndo_start_xmit routine of the driver directly. So theres no common code that an skb traverses from pktgen to a given driver where we can check such a flag Again, I'm happy to change that so that pktgen uses dev_hard_start_xmit, but I wonder if thats going to get the same sort of pushback about performance that my origional patch did. Eric, et al., thoughts? Regards Neil -- 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: Neil Horman <nhorman@tuxdriver.com> Date: Thu, 21 Jul 2011 19:50:49 -0400 > I'm happy to go down this route Dave, and agree, its a more solid solution, but > I think the problem with it (which Ben may have been alluding to previously) is > that pktgen doesn't use dev_queue_xmit or dev_hard_start_xmit to send frames. Neil, that's THE WHOLE POINT, and HOW MY IDEA WORKS. Pktgen bypasses those functions, so it won't trigger the check and therefore won't trigger making a copy of the SKB, since copying isn't needed. Only layering devices will end up eventually calling into those two functions and trigger the "need to copy because this SKB is pktgen shared" check. -- 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, Jul 21, 2011 at 05:08:55PM -0700, David Miller wrote: > From: Neil Horman <nhorman@tuxdriver.com> > Date: Thu, 21 Jul 2011 19:50:49 -0400 > > > I'm happy to go down this route Dave, and agree, its a more solid solution, but > > I think the problem with it (which Ben may have been alluding to previously) is > > that pktgen doesn't use dev_queue_xmit or dev_hard_start_xmit to send frames. > > Neil, that's THE WHOLE POINT, and HOW MY IDEA WORKS. > Dave, RELAX! I GET HOW YOUR IDEA SHOULD WORK! CAPS ARENT NEEDED! :) > Pktgen bypasses those functions, so it won't trigger the check and > therefore won't trigger making a copy of the SKB, since copying isn't > needed. > But thats part of the problem, we can't just have pktgen bypass those calls entirely, assuming that the underlying driver will do so. See the ppp driver as an example, it does an skb_pull to add a header to the skb, so multiple iterations of the multi-skb case will result in stacked header inadvertently. Another example is the virtio_net driver, which uses the skb->cb area to store information that would get corrupted in the process. Neither of those would pass through the dev_hard_start_xmit path and would be left uncovered by this solution. We could fix that by: a) having pktgen call dev_hard_start_xmit for all frames, giving rise to the performance issue I was bringing up in my last note. b) augmenting each driver to check for the flag in the skb as you describe, which in my mind is no better than having a flag in the netdevice informing pktgen of weather or not it can use a shared skb approach. > Only layering devices will end up eventually calling into those two > functions and trigger the "need to copy because this SKB is pktgen > shared" check. > See above, that doesn't solve the entire problem. Regards Neil -- 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/linux/if.h b/include/linux/if.h index 3bc63e6..ae904fe 100644 --- a/include/linux/if.h +++ b/include/linux/if.h @@ -76,6 +76,7 @@ #define IFF_BRIDGE_PORT 0x4000 /* device used as bridge port */ #define IFF_OVS_DATAPATH 0x8000 /* device used as Open vSwitch * datapath port */ +#define IFF_CANT_SHARE_SKB 0x10000 /* Device can't share skbs in tx path */ #define IF_GET_IFACE 0x0001 /* for querying only */ #define IF_GET_PROTO 0x0002 diff --git a/net/core/pktgen.c b/net/core/pktgen.c index f76079c..bf6d88d 100644 --- a/net/core/pktgen.c +++ b/net/core/pktgen.c @@ -1071,6 +1071,9 @@ static ssize_t pktgen_if_write(struct file *file, if (len < 0) return len; + if (pkt_dev->priv_flags & IFF_CANT_SHARE_SKB) + return -EOPNOTSUPP; + i += len; pkt_dev->clone_skb = value;