| Message ID | 20220123115623.94843-1-42.hyeyoo@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ena: Do not waste napi skb cache | expand |
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(). > 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,
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? >> 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, >
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. Unless I'm missing something here I don't think the bug you're describing exists. 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, >>
On Mon, Jan 24, 2022 at 10:50:05PM +0200, 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(). > > > Right. I missed this. Thank you for pointing this! > > > > 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? > > > Shay and Julian, Thank you for reviewing 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. > > Unless I'm missing something here I don't think the bug you're describing > exists. > > 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? > Okay. I'll update and test it again. BTW, It seems netdev_alloc_skb_ip_align() is used to make some fields be aligned. It's okay to just ignore this? Thanks, Hyeonggon. > 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, > > >
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, >>>
On 25.01.22 11:34, Hyeonggon Yoo wrote: > On Mon, Jan 24, 2022 at 10:50:05PM +0200, Shay Agroskin wrote: >> [...] >> >> 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? >> > > Okay. I'll update and test it again. > > BTW, It seems netdev_alloc_skb_ip_align() is used to make some fields > be aligned. It's okay to just ignore this? > napi_alloc_skb() adds NET_IP_ALIGN internally, so you end up with the same alignment as before. > Thanks, > Hyeonggon. > >> 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, >>>>
On Tue, Jan 25, 2022 at 03:50:30PM +0200, Julian Wiedmann wrote: > On 25.01.22 11:34, Hyeonggon Yoo wrote: > > On Mon, Jan 24, 2022 at 10:50:05PM +0200, Shay Agroskin wrote: > >> > > [...] > > >> > >> 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? > >> > > > > Okay. I'll update and test it again. > > > > BTW, It seems netdev_alloc_skb_ip_align() is used to make some fields > > be aligned. It's okay to just ignore this? > > > > napi_alloc_skb() adds NET_IP_ALIGN internally, so you end up with the same alignment as before. > Oh I was missing that. I updated the patch and tested again. Thank you! > > > Thanks, > > Hyeonggon. > > > >> 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, > >>>> >
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); 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,
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(-)