diff mbox

pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods

Message ID 20110720151827.GD12349@hmsreliant.think-freely.org
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Neil Horman July 20, 2011, 3:18 p.m. UTC 
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



--
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

Comments

Eric Dumazet July 20, 2011, 3:30 p.m. UTC | #1 
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
=?ISO-8859-2?Q?Micha=B3_Miros=B3aw?= July 20, 2011, 3:35 p.m. UTC | #2 
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
Neil Horman July 20, 2011, 3:39 p.m. UTC | #3 
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
Neil Horman July 20, 2011, 3:40 p.m. UTC | #4 
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
Eric Dumazet July 20, 2011, 3:44 p.m. UTC | #5 
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
=?ISO-8859-2?Q?Micha=B3_Miros=B3aw?= July 20, 2011, 4:08 p.m. UTC | #6 
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
Neil Horman July 20, 2011, 4:18 p.m. UTC | #7 
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
Neil Horman July 20, 2011, 4:19 p.m. UTC | #8 
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
Ben Greear July 20, 2011, 4:33 p.m. UTC | #9 
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
Neil Horman July 20, 2011, 4:36 p.m. UTC | #10 
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
Ben Greear July 20, 2011, 4:37 p.m. UTC | #11 
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
David Miller July 21, 2011, 10:01 p.m. UTC | #12 
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
Ben Greear July 21, 2011, 10:14 p.m. UTC | #13 
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
David Miller July 21, 2011, 10:19 p.m. UTC | #14 
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
Ben Greear July 21, 2011, 10:26 p.m. UTC | #15 
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
Neil Horman July 21, 2011, 11:50 p.m. UTC | #16 
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
David Miller July 22, 2011, 12:08 a.m. UTC | #17 
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
Neil Horman July 22, 2011, 1:37 a.m. UTC | #18 
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 mbox

Patch

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;