[PATCH v2] net: ethernet: mtk_eth_soc: fix misuse of mem alloc interface netdev[napi]_alloc_frag

Chen Lin chen45464546 at 163.com
Sat Jun 4 19:22:26 PDT 2022


At 2022-06-03 23:33:25, "Alexander Duyck" <alexander.duyck at gmail.com> wrote:
>On Fri, Jun 3, 2022 at 8:25 AM Alexander Duyck
><alexander.duyck at gmail.com> wrote:
>>
>> On Fri, Jun 3, 2022 at 2:03 AM Chen Lin <chen45464546 at 163.com> wrote:
>> >
>> > When rx_flag == MTK_RX_FLAGS_HWLRO,
>> > rx_data_len = MTK_MAX_LRO_RX_LENGTH(4096 * 3) > PAGE_SIZE.
>> > netdev_alloc_frag is for alloction of page fragment only.
>> > Reference to other drivers and Documentation/vm/page_frags.rst
>> >
>> > Branch to use alloc_pages when ring->frag_size > PAGE_SIZE.
>> >
>> > Signed-off-by: Chen Lin <chen45464546 at 163.com>
>> > ---
>> >  drivers/net/ethernet/mediatek/mtk_eth_soc.c |   22 ++++++++++++++++++++--
>> >  1 file changed, 20 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
>> > index b3b3c07..772d903 100644
>> > --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
>> > +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
>> > @@ -1467,7 +1467,16 @@ static int mtk_poll_rx(struct napi_struct *napi, int budget,
>> >                         goto release_desc;
>> >
>> >                 /* alloc new buffer */
>> > -               new_data = napi_alloc_frag(ring->frag_size);
>> > +               if (ring->frag_size <= PAGE_SIZE) {
>> > +                       new_data = napi_alloc_frag(ring->frag_size);
>> > +               } else {
>> > +                       struct page *page;
>> > +                       unsigned int order = get_order(ring->frag_size);
>> > +
>> > +                       page = alloc_pages(GFP_ATOMIC | __GFP_COMP |
>> > +                                           __GFP_NOWARN, order);
>> > +                       new_data = page ? page_address(page) : NULL;
>> > +               }
>> >                 if (unlikely(!new_data)) {
>> >                         netdev->stats.rx_dropped++;
>> >                         goto release_desc;
>> > @@ -1914,7 +1923,16 @@ static int mtk_rx_alloc(struct mtk_eth *eth, int ring_no, int rx_flag)
>> >                 return -ENOMEM;
>> >
>> >         for (i = 0; i < rx_dma_size; i++) {
>> > -               ring->data[i] = netdev_alloc_frag(ring->frag_size);
>> > +               if (ring->frag_size <= PAGE_SIZE) {
>> > +                       ring->data[i] = netdev_alloc_frag(ring->frag_size);
>> > +               } else {
>> > +                       struct page *page;
>> > +                       unsigned int order = get_order(ring->frag_size);
>> > +
>> > +                       page = alloc_pages(GFP_KERNEL | __GFP_COMP |
>> > +                                           __GFP_NOWARN, order);
>> > +                       ring->data[i] = page ? page_address(page) : NULL;
>> > +               }
>> >                 if (!ring->data[i])
>> >                         return -ENOMEM;
>> >         }
>>
>> Actually I looked closer at this driver. Is it able to receive frames
>> larger than 2K? If not there isn't any point in this change.
>>
>> Based on commit 4fd59792097a ("net: ethernet: mediatek: support
>> setting MTU") it looks like it doesn't, so odds are this patch is not
>> necessary.
>
>I spoke too soon. I had overlooked the LRO part. With that being the
>case you can probably optimize this code to do away with the get_order
>piece entirely, at least during runtime. My main concern is that doing
>that in the fast-path will be expensive so you would be much better
>off doing something like
>get_order(mtk_max_frag_size(MTK_RX_FLAGS_HWLRO)) which would be
>converted into a constant at compile time since everything else would
>be less than 1 page in size.
>
>Also you could then replace alloc_pages with __get_free_pages which
>would take care of the page_address call for you.

Thanks for the tips. I'll try again.
It can also be seen from here it is easy to make mistakes in parameter fragsz.


More information about the Linux-mediatek mailing list