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

[Costin Lupu <costin.lupu@xxxxxxxxx>]

Hi Simon,

Actually I'd prefer my authorship removed from this patch since these
changes are different enough than the changes I submitted.

Cheers,
Costin

On 11/6/20 3: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?
> 
> 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);
>