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

[Sharan Santhanam <sharan.santhanam@xxxxxxxxx>]

On 11/6/20 2:19 PM, Simon Kuenzer wrote:
> 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?
Sure, we can add a comment in code.

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