Re: [PATCH] veth: Optionally pad packets to minimum Ethernet length

[Stephen Hemminger]

On Tue, 12 Dec 2017 11:00:38 -0800
Ed Swierk  wrote:

> On Tue, Dec 12, 2017 at 10:34 AM, Stephen Hemminger
>  wrote:
> > Why not add to netdevsim rather than cluttering up a normal driver
> > with test support.  We just pulled a bunch of test stuff out of dummy
> > for the same reason.  
> 
> My test setup to trigger an openvswitch conntrack issue
> (https://marc.info/?l=linux-netdev=151309548725627) involves a lot
> of moving parts:
> 
> [netns-a: vetha1] - [vetha0] - [ovsbr0] - [vethb0] - [netns-b: vethb1]
> 
> with nc client and server in netns-a and -b, and tweaks like turning
> off tcp_timestamps to make sure the packets in the TCP stream are
> small enough to reproduce the problem. A simpler, less fragile test
> setup would be valuable, especially if it ends up as an automated
> regression test.
> 
> Could netdevsim be useful for that? Are there any existing tests
> producing TCP traffic that might serve as an example?
> 
> --Ed

Maybe add a netem impairment that does padding?

Re: [PATCH] veth: Optionally pad packets to minimum Ethernet length

[Cong Wang]

On Tue, Dec 12, 2017 at 8:13 AM, Ed Swierk  wrote:
> Most physical Ethernet devices pad short packets to the minimum length
> of 64 bytes (including FCS) on transmit. It can be useful to simulate
> this behavior when debugging a problem that results from it (such as
> incorrect L4 checksum calculation).
>
> Padding is unnecessary for most applications so leave it off by
> default. Enable padding only when the otherwise unused IFF_AUTOMEDIA
> flag is set (e.g. by writing 0x5003 to flags in sysfs).

This doesn't make sense, why should veth hot path be punished for
such an unusual flag which it doesn't care? Also, why should we allow
setting this flag via sysfs for veth from the beginning?

Re: [PATCH] veth: Optionally pad packets to minimum Ethernet length

[Ed Swierk]

On Tue, Dec 12, 2017 at 10:34 AM, Stephen Hemminger
 wrote:
> Why not add to netdevsim rather than cluttering up a normal driver
> with test support.  We just pulled a bunch of test stuff out of dummy
> for the same reason.

My test setup to trigger an openvswitch conntrack issue
(https://marc.info/?l=linux-netdev=151309548725627) involves a lot
of moving parts:

[netns-a: vetha1] - [vetha0] - [ovsbr0] - [vethb0] - [netns-b: vethb1]

with nc client and server in netns-a and -b, and tweaks like turning
off tcp_timestamps to make sure the packets in the TCP stream are
small enough to reproduce the problem. A simpler, less fragile test
setup would be valuable, especially if it ends up as an automated
regression test.

Could netdevsim be useful for that? Are there any existing tests
producing TCP traffic that might serve as an example?

--Ed

Re: [PATCH] veth: Optionally pad packets to minimum Ethernet length

[Stephen Hemminger]

On Tue, 12 Dec 2017 16:18:17 -0200
Marcelo Ricardo Leitner  wrote:

> On Tue, Dec 12, 2017 at 11:32:46AM -0600, Dan Williams wrote:
> > On Tue, 2017-12-12 at 08:13 -0800, Ed Swierk wrote:  
> > > Most physical Ethernet devices pad short packets to the minimum
> > > length
> > > of 64 bytes (including FCS) on transmit. It can be useful to simulate
> > > this behavior when debugging a problem that results from it (such as
> > > incorrect L4 checksum calculation).
> > > 
> > > Padding is unnecessary for most applications so leave it off by
> > > default. Enable padding only when the otherwise unused IFF_AUTOMEDIA
> > > flag is set (e.g. by writing 0x5003 to flags in sysfs).  
> > 
> > This seems like a weird overload of AUTOMEDIA, which no other driver
> > uses for this purpose.  Seems like the only other user of AUTOMEDIA is
> > 8390/etherh.c for some 10BaseT/10Base2 stuff.
> > 
> > I'm not sure what the interface should be, but perhaps a sysfs
> > attribute would be better than overloading IFF_AUTOMEDIA?  
> 
> What about using some tc action (i.e. skbmod) for this?
> 
>   Marcelo

Why not add to netdevsim rather than cluttering up a normal driver
with test support.  We just pulled a bunch of test stuff out of dummy
for the same reason.  

Re: [PATCH] veth: Optionally pad packets to minimum Ethernet length

[Marcelo Ricardo Leitner]

On Tue, Dec 12, 2017 at 11:32:46AM -0600, Dan Williams wrote:
> On Tue, 2017-12-12 at 08:13 -0800, Ed Swierk wrote:
> > Most physical Ethernet devices pad short packets to the minimum
> > length
> > of 64 bytes (including FCS) on transmit. It can be useful to simulate
> > this behavior when debugging a problem that results from it (such as
> > incorrect L4 checksum calculation).
> > 
> > Padding is unnecessary for most applications so leave it off by
> > default. Enable padding only when the otherwise unused IFF_AUTOMEDIA
> > flag is set (e.g. by writing 0x5003 to flags in sysfs).
> 
> This seems like a weird overload of AUTOMEDIA, which no other driver
> uses for this purpose.  Seems like the only other user of AUTOMEDIA is
> 8390/etherh.c for some 10BaseT/10Base2 stuff.
> 
> I'm not sure what the interface should be, but perhaps a sysfs
> attribute would be better than overloading IFF_AUTOMEDIA?

What about using some tc action (i.e. skbmod) for this?

  Marcelo

> 
> Dan
> 
> > Signed-off-by: Ed Swierk 
> > ---
> >  drivers/net/veth.c | 6 ++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/net/veth.c b/drivers/net/veth.c
> > index f5438d0978ca..292029bf4bb2 100644
> > --- a/drivers/net/veth.c
> > +++ b/drivers/net/veth.c
> > @@ -111,6 +111,12 @@ static netdev_tx_t veth_xmit(struct sk_buff
> > *skb, struct net_device *dev)
> >     goto drop;
> >     }
> >  
> > +   if (unlikely(dev->flags & IFF_AUTOMEDIA)) {
> > +   /* if eth_skb_pad returns an error the skb was freed
> > */
> > +   if (eth_skb_pad(skb))
> > +   goto drop;
> > +   }
> > +
> >     if (likely(dev_forward_skb(rcv, skb) == NET_RX_SUCCESS)) {
> >     struct pcpu_vstats *stats = this_cpu_ptr(dev-
> > >vstats);
> >  
> 

Re: [PATCH] veth: Optionally pad packets to minimum Ethernet length

[Dan Williams]

On Tue, 2017-12-12 at 08:13 -0800, Ed Swierk wrote:
> Most physical Ethernet devices pad short packets to the minimum
> length
> of 64 bytes (including FCS) on transmit. It can be useful to simulate
> this behavior when debugging a problem that results from it (such as
> incorrect L4 checksum calculation).
> 
> Padding is unnecessary for most applications so leave it off by
> default. Enable padding only when the otherwise unused IFF_AUTOMEDIA
> flag is set (e.g. by writing 0x5003 to flags in sysfs).

This seems like a weird overload of AUTOMEDIA, which no other driver
uses for this purpose.  Seems like the only other user of AUTOMEDIA is
8390/etherh.c for some 10BaseT/10Base2 stuff.

I'm not sure what the interface should be, but perhaps a sysfs
attribute would be better than overloading IFF_AUTOMEDIA?

Dan

> Signed-off-by: Ed Swierk 
> ---
>  drivers/net/veth.c | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/net/veth.c b/drivers/net/veth.c
> index f5438d0978ca..292029bf4bb2 100644
> --- a/drivers/net/veth.c
> +++ b/drivers/net/veth.c
> @@ -111,6 +111,12 @@ static netdev_tx_t veth_xmit(struct sk_buff
> *skb, struct net_device *dev)
>   goto drop;
>   }
>  
> + if (unlikely(dev->flags & IFF_AUTOMEDIA)) {
> + /* if eth_skb_pad returns an error the skb was freed
> */
> + if (eth_skb_pad(skb))
> + goto drop;
> + }
> +
>   if (likely(dev_forward_skb(rcv, skb) == NET_RX_SUCCESS)) {
>   struct pcpu_vstats *stats = this_cpu_ptr(dev-
> >vstats);
>