Re: [UNIKRAFT/LWIP PATCH v4 2/5] lwipopts.h: Disable Ethernet frame padding

[Simon Kuenzer <simon.kuenzer@xxxxxxxxx>]

On 06.11.20 12:28, Sharan Santhanam wrote:
> Hello Simon,
>
> Please find the comment inline.
>
> Thanks & Regards
> Sharan
>
> On 10/30/20 7:31 PM, Simon Kuenzer wrote:
> > From: Costin Lupu <costin.lupu@xxxxxxxxx>
> >
> > We set ETH_PAD_SIZE to 0 considering that padding and alignment is
> > handled according to each driver capabilities.
> >
> > Signed-off-by: Costin Lupu <costin.lupu@xxxxxxxxx>
> > Signed-off-by: Simon Kuenzer <simon.kuenzer@xxxxxxxxx>
> > ---
> >   include/arch/cc.h | 4 +++-
> >   uknetdev.c        | 6 ++++++
> >   2 files changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/arch/cc.h b/include/arch/cc.h
> > index a1d0c34..b4526e0 100644
> > --- a/include/arch/cc.h
> > +++ b/include/arch/cc.h
> > @@ -51,7 +51,9 @@
> >   /* 32 bit checksum calculation */
> >   #define LWIP_CHKSUM_ALGORITHM 3
> > -#define ETH_PAD_SIZE 2
> > +
> > +/* Disable padding on Ethernet frames (only some uknetdev driver 
> > support it) */
> > +#define ETH_PAD_SIZE 0
> If we make this driver dependent, shouldn't this be driven by either 
> KConfig  hidden option.
Our idea was to set this as a global fixed thing which cannot be 
configured. According to https://lwn.net/Articles/89597/, the gain of 
doing padding seems to be negligible on the CPU architecture that we 
support but it may hurt DMA engines that may get involved on the 
hypervisor side. Additionally, it will cause issues with netfront 
because the netfront protocol does not support setting an offset for 
reception. Padding can then only be achieved by moving the packet bytes 
in memory which is obviously ways too costly. So, we think it should 
disabled it completely.
I can add this as a comment to the commit message or as note to the 
code. What do you think?
Thanks,

Simon

> >   /* rand */
> >   #define LWIP_RAND() uk_swrand_randr()
> > diff --git a/uknetdev.c b/uknetdev.c
> > index e645ac1..ccb7368 100644
> > --- a/uknetdev.c
> > +++ b/uknetdev.c
> > @@ -259,6 +259,12 @@ static void uknetdev_input(struct uk_netdev *dev,
> >           /* Send packet to lwip */
> >   #if ETH_PAD_SIZE
> > +        /* If the following assertion fails, the driver most likely does
> > +         * not support padded receive. In such a case ETH_PAD_SIZE has
> > +         * to be set to 0.
> > +         */
> > +        UK_ASSERT(uk_netbuf_headroom(nb) >= ETH_PAD_SIZE);
> > +
> >           uk_netbuf_header(nb, ETH_PAD_SIZE);
> >   #endif /* ETH_PAD_SIZE */
> >           p = lwip_netbuf_to_pbuf(nb);