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

[Costin Lupu <costin.lupu@xxxxxxxxx>]

That's right, I already said it in a reply for v4.

Cheers,
Costin

On 11/13/20 12:41 AM, Simon Kuenzer wrote:
> Hey Sharan, hey Costin,
> 
> as Costin discussed with me offline, we will remove his authorship from
> this patch while upstreaming. @Costin, can you give your okay to this
> mail? It is just to have a public confirmation.
> 
> Thanks a lot,
> 
> Simon
> 
> On 09.11.20 17:06, Simon Kuenzer wrote:
>> From: Costin Lupu <costin.lupu@xxxxxxxxx>
>>
>> We set ETH_PAD_SIZE to 0 and remove support from uknetdev.c. This is done
>> because some uknetdev drivers will not support it (e.g., netfront) and
>> according to
>> https://secure-web.cisco.com/1I6SnmQvRU2gMej0l9HlXP44qAF9pwZO-IReMAMz5mwNylavsl44K1LTKspaQN8XzFM8QwZseMxqmadSprZ91FF9pIoIPEe_M75jBqoLR2Xww_-9b8E47RdfWRyCByu3JBIkXNHojrew6VcjWCQXkfISvn_PHJ6tRSe0hZpdEnlClhLfyRuSKQ1WaHr7wXZklxeMrrFl6IZFfid11TVs7skdT6p_9LEpRGC_JHe8Cln4d8iSAaFvVYCNz9KBo57-l6FX7_4ND8bx3QFK-zOrBFYVbgKv4_EJFnw8a96JAec3UTog43O1CkoBhYLDFlfwa/https%3A%2F%2Flwn.net%2FArticles%2F89597%2F,
>> the gains of doing padding
>> is negligible on the CPU architectures that we currently support while
>> potentially hurting DMA engine performances that may get involved with
>> the respective driver and/or hypervisor.
>>
>> Signed-off-by: Costin Lupu <costin.lupu@xxxxxxxxx>
>> Signed-off-by: Simon Kuenzer <simon.kuenzer@xxxxxxxxx>
>> ---
>>   include/arch/cc.h |  7 ++++++-
>>   uknetdev.c        | 42 ++++++++++++++++++------------------------
>>   2 files changed, 24 insertions(+), 25 deletions(-)
>>
>> diff --git a/include/arch/cc.h b/include/arch/cc.h
>> index a1d0c34..0593f05 100644
>> --- a/include/arch/cc.h
>> +++ b/include/arch/cc.h
>> @@ -51,7 +51,12 @@
>>     /* 32 bit checksum calculation */
>>   #define LWIP_CHKSUM_ALGORITHM 3
>> -#define ETH_PAD_SIZE 2
>> +
>> +/*
>> + * Disable padding on Ethernet frames
>> + * (not supported by uknetdev driver)
>> + */
>> +#define ETH_PAD_SIZE 0
>>     /* rand */
>>   #define LWIP_RAND() uk_swrand_randr()
>> diff --git a/uknetdev.c b/uknetdev.c
>> index 15464f0..ff61462 100644
>> --- a/uknetdev.c
>> +++ b/uknetdev.c
>> @@ -56,6 +56,22 @@
>>     #include <uk/essentials.h>
>>   +/*
>> + * NOTE: We do not support ETH_PAD_SIZE with uknetdev.
>> + *       According to
>> https://secure-web.cisco.com/1I6SnmQvRU2gMej0l9HlXP44qAF9pwZO-IReMAMz5mwNylavsl44K1LTKspaQN8XzFM8QwZseMxqmadSprZ91FF9pIoIPEe_M75jBqoLR2Xww_-9b8E47RdfWRyCByu3JBIkXNHojrew6VcjWCQXkfISvn_PHJ6tRSe0hZpdEnlClhLfyRuSKQ1WaHr7wXZklxeMrrFl6IZFfid11TVs7skdT6p_9LEpRGC_JHe8Cln4d8iSAaFvVYCNz9KBo57-l6FX7_4ND8bx3QFK-zOrBFYVbgKv4_EJFnw8a96JAec3UTog43O1CkoBhYLDFlfwa/https%3A%2F%2Flwn.net%2FArticles%2F89597%2F,
>> the gain of doing padding
>> + *       seems to be negligible on the CPU architectures that we
>> currently
>> + *       support and may hurt DMA engine performances that may get
>> involved with
>> + *       the driver and/or hypervisor. Additionally, it will cause
>> difficulties
>> + *       with the current netfront driver because the protocol does
>> not support
>> + *       setting an offset for page-aligned receive buffers. Aligning
>> the frame
>> + *       could then only be achieved by moving the packet bytes in
>> memory which
>> + *       is obviously ways too costly. Because of this, we disable
>> this feature
>> + *       completely.
>> + */
>> +#if ETH_PAD_SIZE
>> +#error ETH_PAD_SIZE is not '0'. This is not supported.
>> +#endif
>> +
>>   #define UKNETDEV_BPS 1000000000u
>>   #define UKNETDEV_BUFLEN 2048
>>   @@ -97,8 +113,8 @@ UK_CTASSERT((sizeof(struct lwip_netdev_data)) <=
>> CONFIG_UK_NETDEV_SCRATCH_SIZE);
>>    *       `struct uk_netdev` in order to avoid another allocation for
>> these
>>    *       per-device fields.
>>    */
>> -static uint16_t rx_headroom = ETH_PAD_SIZE;
>> -static uint16_t tx_headroom = ETH_PAD_SIZE;
>> +static uint16_t rx_headroom = 0;
>> +static uint16_t tx_headroom = 0;
>>     #define netif_to_uknetdev(nf) \
>>       ((struct uk_netdev *) (nf)->state)
>> @@ -160,9 +176,6 @@ static err_t uknetdev_output(struct netif *nf,
>> struct pbuf *p)
>>           return ERR_MEM;
>>       }
>>   -#if ETH_PAD_SIZE
>> -    pbuf_header(p, -ETH_PAD_SIZE); /* drop the padding word */
>> -#endif /* ETH_PAD_SIZE */
>>       /*
>>        * Copy pbuf to netbuf
>>        * NOTE: Unfortunately, lwIP seems not to support zero-copy
>> transmit,
>> @@ -174,26 +187,16 @@ static err_t uknetdev_output(struct netif *nf,
>> struct pbuf *p)
>>           wpos += q->len;
>>       }
>>       nb->len = p->tot_len;
>> -#if ETH_PAD_SIZE
>> -    pbuf_header(p, ETH_PAD_SIZE); /* reclaim the padding word */
>> -#endif /* ETH_PAD_SIZE */
>>         /* Transmit packet */
>>       do {
>>           ret = uk_netdev_tx_one(dev, 0, nb);
>>       } while (uk_netdev_status_notready(ret));
>>       if (unlikely(ret < 0)) {
>> -#if ETH_PAD_SIZE
>> -        LWIP_DEBUGF(NETIF_DEBUG,
>> -                ("%s: %c%c%u: Failed to send %"PRIu16" bytes\n",
>> -                 __func__, nf->name[0], nf->name[1], nf->num,
>> -                 p->tot_len - ETH_PAD_SIZE));
>> -#else /* ETH_PAD_SIZE */
>>           LWIP_DEBUGF(NETIF_DEBUG,
>>                   ("%s: %c%c%u: Failed to send %"PRIu16" bytes\n",
>>                    __func__, nf->name[0], nf->name[1], nf->num,
>>                    p->tot_len));
>> -#endif /* ETH_PAD_SIZE */
>>           /*
>>            * Decrease refcount again because in
>>            * the error case the netdev did not consume the pbuf
>> @@ -201,15 +204,9 @@ static err_t uknetdev_output(struct netif *nf,
>> struct pbuf *p)
>>           uk_netbuf_free_single(nb);
>>           return ERR_IF;
>>       }
>> -#if ETH_PAD_SIZE
>> -    LWIP_DEBUGF(NETIF_DEBUG, ("%s: %c%c%u: Sent %"PRIu16" bytes\n",
>> -                  __func__, nf->name[0], nf->name[1], nf->num,
>> -                  p->tot_len - ETH_PAD_SIZE));
>> -#else /* ETH_PAD_SIZE */
>>       LWIP_DEBUGF(NETIF_DEBUG, ("%s: %c%c%u: Sent %"PRIu16" bytes\n",
>>                     __func__, nf->name[0], nf->name[1], nf->num,
>>                     p->tot_len));
>> -#endif /* ETH_PAD_SIZE */
>>         return ERR_OK;
>>   }
>> @@ -265,9 +262,6 @@ static void uknetdev_input(struct uk_netdev *dev,
>>                    nb->len));
>>             /* Send packet to lwip */
>> -#if ETH_PAD_SIZE
>> -        uk_netbuf_header(nb, ETH_PAD_SIZE);
>> -#endif /* ETH_PAD_SIZE */
>>           p = lwip_netbuf_to_pbuf(nb);
>>           p->payload = nb->data;
>>           p->tot_len = p->len = nb->len;
>>
>