Messages in this thread | | | Date | Tue, 25 Jan 2022 15:45:13 +0200 | Subject | Re: [PATCH] net: ena: Do not waste napi skb cache | From | Julian Wiedmann <> |
| |
On 24.01.22 22:50, Shay Agroskin wrote: > > Julian Wiedmann <jwiedmann.dev@gmail.com> writes: > >> On 24.01.22 10:57, Julian Wiedmann wrote: >>> On 23.01.22 13:56, Hyeonggon Yoo wrote: >>>> By profiling, discovered that ena device driver allocates skb by >>>> build_skb() and frees by napi_skb_cache_put(). Because the driver >>>> does not use napi skb cache in allocation path, napi skb cache is >>>> periodically filled and flushed. This is waste of napi skb cache. >>>> >>>> As ena_alloc_skb() is called only in napi, Use napi_build_skb() >>>> instead of build_skb() to when allocating skb. >>>> >>>> This patch was tested on aws a1.metal instance. >>>> >>>> Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com> >>>> --- >>>> drivers/net/ethernet/amazon/ena/ena_netdev.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c >>>> index c72f0c7ff4aa..2c67fb1703c5 100644 >>>> --- a/drivers/net/ethernet/amazon/ena/ena_netdev.c >>>> +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c >>>> @@ -1407,7 +1407,7 @@ static struct sk_buff *ena_alloc_skb(struct ena_ring *rx_ring, void *first_frag) >>>> skb = netdev_alloc_skb_ip_align(rx_ring->netdev, >>>> rx_ring->rx_copybreak); >>> >>> To keep things consistent, this should then also be napi_alloc_skb(). >>> >> >> And on closer look, this copybreak path also looks buggy. If rx_copybreak >> gets reduced _while_ receiving a frame, the allocated skb can end up too >> small to take all the data. >> >> @ ena maintainers: can you please fix this? >> > > Updating the copybreak value is done through ena_ethtool.c (ena_set_tunable()) which updates `adapter->rx_copybreak`. > The adapter->rx_copybreak value is "propagated back" to the ring local attributes (rx_ring->rx_copybreak) only after an interface toggle which stops the napi routine first. >
That's unfortunate. ena_get_tunable() returns the updated adapter->rx_copybreak, how would a user know that their update isn't actually live yet?
> Unless I'm missing something here I don't think the bug you're describing exists. >
ack, thanks for double-checking!
> I agree that the netdev_alloc_skb_ip_align() can become napi_alloc_skb(). Hyeonggon Yoo, can you please apply this change as well to this patch? > > Thanks, > Shay > > >>>> else >>>> - skb = build_skb(first_frag, ENA_PAGE_SIZE); >>>> + skb = napi_build_skb(first_frag, ENA_PAGE_SIZE); >>>> >>>> if (unlikely(!skb)) { >>>> ena_increase_stat(&rx_ring->rx_stats.skb_alloc_fail, 1, >>>
| |