Hi Christophe,
Thanks for the patch.
Just a question, As per [1], former can be allocated from interrupt context.
But nothing mentioned for the allocation using the patch you mentioned[2]. I agree GFP_KERNEL
gives more opportunities of successful allocation.
Q1) Here it allocates 8K instead of 1K on each loop, Is there any limitation for netdev_alloc_skb for allocating 8K size?
Q2) In terms of allocation performance which is better netdev_alloc_skb or __netdev_alloc_skb?
[1] https://www.kernel.org/doc/htmldocs/networking/API-netdev-alloc-skb.html
[2] https://www.kernel.org/doc/htmldocs/networking/API---netdev-alloc-skb.html
Regards,
Biju
> Subject: [PATCH] ravb: Use GFP_KERNEL instead of GFP_ATOMIC when possible
>
> 'max_rx_len' can be up to GBETH_RX_BUFF_MAX (i.e. 8192) (see
> 'gbeth_hw_info').
> The default value of 'num_rx_ring' can be BE_RX_RING_SIZE (i.e. 1024).
>
> So this loop can allocate 8 Mo of memory.
>
> Previous memory allocations in this function already use GFP_KERNEL, so
> use __netdev_alloc_skb() and an explicit GFP_KERNEL instead of a implicit
> GFP_ATOMIC.
>
> This gives more opportunities of successful allocation.
>
> Signed-off-by: Christophe JAILLET <[email protected]>
> ---
> drivers/net/ethernet/renesas/ravb_main.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c
> b/drivers/net/ethernet/renesas/ravb_main.c
> index 24e2635c4c80..525d66f71f02 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -475,7 +475,7 @@ static int ravb_ring_init(struct net_device *ndev, int
> q)
> goto error;
>
> for (i = 0; i < priv->num_rx_ring[q]; i++) {
> - skb = netdev_alloc_skb(ndev, info->max_rx_len);
> + skb = __netdev_alloc_skb(ndev, info->max_rx_len, GFP_KERNEL);
> if (!skb)
> goto error;
> ravb_set_buffer_align(skb);
> --
> 2.32.0
Le 20/02/2022 à 08:53, Biju Das a écrit :
> Hi Christophe,
>
> Thanks for the patch.
>
> Just a question, As per [1], former can be allocated from interrupt context.
> But nothing mentioned for the allocation using the patch you mentioned[2]. I agree GFP_KERNEL
> gives more opportunities of successful allocation.
Hi,
netdev_alloc_skb() uses an implicit GFP_ATOMIC, that is why it can be
safely called from an interrupt context.
__netdev_alloc_skb() is the same as netdev_alloc_skb(), except that you
can choose the GFP flag you want to use. ([1])
Here, the netdev_alloc_skb() is called just after some
"kcalloc(GFP_KERNEL);"
So this function can already NOT be called from interrupt context.
So if GFP_KERNEL is fine here for kcalloc(), it is fine also for
netdev_alloc_skb(), hence __netdev_alloc_skb(GFP_KERNEL).
>
> Q1) Here it allocates 8K instead of 1K on each loop, Is there any limitation for netdev_alloc_skb for allocating 8K size?
Not sure to understand.
My patch does NOT change anything on the amount of memory allocated. it
only changes a GFP_ATOMIC into a GFP_KERNEL.
I'm not aware of specific limitation for netdev_alloc_skb().
My understanding is that in the worst case, it will behave just like
malloc() ([3])
So, if it was an issue before, it is still an issue after my patch.
> Q2) In terms of allocation performance which is better netdev_alloc_skb or __netdev_alloc_skb?
AFAIK, there should be no difference, but __netdev_alloc_skb(GFP_KERNEL)
can succeed where netdev_alloc_skb() can fail. In such a case, it would
be slower but most importantly, it would succeed.
CJ
[1]:
https://elixir.bootlin.com/linux/v5.17-rc4/source/include/linux/skbuff.h#L2945
[2]:
https://elixir.bootlin.com/linux/v5.17-rc4/source/drivers/net/ethernet/renesas/ravb_main.c#L470
[3]:
https://elixir.bootlin.com/linux/v5.17-rc3/source/net/core/skbuff.c#L488
>
> [1] https://www.kernel.org/doc/htmldocs/networking/API-netdev-alloc-skb.html
> [2] https://www.kernel.org/doc/htmldocs/networking/API---netdev-alloc-skb.html
>
> Regards,
> Biju
>
>> Subject: [PATCH] ravb: Use GFP_KERNEL instead of GFP_ATOMIC when possible
>>
>> 'max_rx_len' can be up to GBETH_RX_BUFF_MAX (i.e. 8192) (see
>> 'gbeth_hw_info').
>> The default value of 'num_rx_ring' can be BE_RX_RING_SIZE (i.e. 1024).
>>
>> So this loop can allocate 8 Mo of memory.
>>
>> Previous memory allocations in this function already use GFP_KERNEL, so
>> use __netdev_alloc_skb() and an explicit GFP_KERNEL instead of a implicit
>> GFP_ATOMIC.
>>
>> This gives more opportunities of successful allocation.
>>
>> Signed-off-by: Christophe JAILLET <[email protected]>
>> ---
>> drivers/net/ethernet/renesas/ravb_main.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c
>> b/drivers/net/ethernet/renesas/ravb_main.c
>> index 24e2635c4c80..525d66f71f02 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -475,7 +475,7 @@ static int ravb_ring_init(struct net_device *ndev, int
>> q)
>> goto error;
>>
>> for (i = 0; i < priv->num_rx_ring[q]; i++) {
>> - skb = netdev_alloc_skb(ndev, info->max_rx_len);
>> + skb = __netdev_alloc_skb(ndev, info->max_rx_len, GFP_KERNEL);
>> if (!skb)
>> goto error;
>> ravb_set_buffer_align(skb);
>> --
>> 2.32.0
>
>
Hi Christophe,
Thanks for your clarification.
Patch Looks good to me.
Reviewed-by: Biju Das <[email protected]>
Cheers,
Biju
> -----Original Message-----
> From: Christophe JAILLET <[email protected]>
> Sent: 20 February 2022 08:49
> To: Biju Das <[email protected]>; Sergey Shtylyov
> <[email protected]>; David S. Miller <[email protected]>; Jakub Kicinski
> <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: Re: [PATCH] ravb: Use GFP_KERNEL instead of GFP_ATOMIC when
> possible
>
> Le 20/02/2022 ? 08:53, Biju Das a ?crit?:
> > Hi Christophe,
> >
> > Thanks for the patch.
> >
> > Just a question, As per [1], former can be allocated from interrupt
> context.
> > But nothing mentioned for the allocation using the patch you
> > mentioned[2]. I agree GFP_KERNEL gives more opportunities of successful
> allocation.
>
> Hi,
>
> netdev_alloc_skb() uses an implicit GFP_ATOMIC, that is why it can be
> safely called from an interrupt context.
> __netdev_alloc_skb() is the same as netdev_alloc_skb(), except that you
> can choose the GFP flag you want to use. ([1])
>
> Here, the netdev_alloc_skb() is called just after some
> "kcalloc(GFP_KERNEL);"
>
> So this function can already NOT be called from interrupt context.
>
> So if GFP_KERNEL is fine here for kcalloc(), it is fine also for
> netdev_alloc_skb(), hence __netdev_alloc_skb(GFP_KERNEL).
>
> >
> > Q1) Here it allocates 8K instead of 1K on each loop, Is there any
> limitation for netdev_alloc_skb for allocating 8K size?
>
> Not sure to understand.
> My patch does NOT change anything on the amount of memory allocated. it
> only changes a GFP_ATOMIC into a GFP_KERNEL.
>
> I'm not aware of specific limitation for netdev_alloc_skb().
> My understanding is that in the worst case, it will behave just like
> malloc() ([3])
>
> So, if it was an issue before, it is still an issue after my patch.
>
> > Q2) In terms of allocation performance which is better netdev_alloc_skb
> or __netdev_alloc_skb?
>
> AFAIK, there should be no difference, but __netdev_alloc_skb(GFP_KERNEL)
> can succeed where netdev_alloc_skb() can fail. In such a case, it would be
> slower but most importantly, it would succeed.
>
>
> CJ
>
> [1]:
> https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.b
> ootlin.com%2Flinux%2Fv5.17-
> rc4%2Fsource%2Finclude%2Flinux%2Fskbuff.h%23L2945&data=04%7C01%7Cbiju.
> das.jz%40bp.renesas.com%7C5fab03422ea84da79f5308d9f44dd4fd%7C53d82571da194
> 7e49cb4625a166a4a2a%7C0%7C0%7C637809437402718622%7CUnknown%7CTWFpbGZsb3d8e
> yJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&a
> mp;sdata=0GC7vUwDkz5n7wESWC2gK3x6e8RalA%2FCg%2FmokTv%2BIHE%3D&reserved
> =0
>
> [2]:
> https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.b
> ootlin.com%2Flinux%2Fv5.17-
> rc4%2Fsource%2Fdrivers%2Fnet%2Fethernet%2Frenesas%2Fravb_main.c%23L470&
> ;data=04%7C01%7Cbiju.das.jz%40bp.renesas.com%7C5fab03422ea84da79f5308d9f44
> dd4fd%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C637809437402718622%7CUn
> known%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLC
> JXVCI6Mn0%3D%7C3000&sdata=KsLMiap6E%2BBEl4DOqBvPE%2BVsfCtTpZqAa4PvyKFq
> B9E%3D&reserved=0
>
> [3]:
> https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.b
> ootlin.com%2Flinux%2Fv5.17-
> rc3%2Fsource%2Fnet%2Fcore%2Fskbuff.c%23L488&data=04%7C01%7Cbiju.das.jz
> %40bp.renesas.com%7C5fab03422ea84da79f5308d9f44dd4fd%7C53d82571da1947e49cb
> 4625a166a4a2a%7C0%7C0%7C637809437402718622%7CUnknown%7CTWFpbGZsb3d8eyJWIjo
> iMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sda
> ta=kwR2VU5sT%2FiD9y5VUMTztet1btFjlHqU1j5pCXF%2F1Vk%3D&reserved=0
>
>
> >
> > [1]
> > https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.
> > kernel.org%2Fdoc%2Fhtmldocs%2Fnetworking%2FAPI-netdev-alloc-skb.html&a
> > mp;data=04%7C01%7Cbiju.das.jz%40bp.renesas.com%7C5fab03422ea84da79f530
> > 8d9f44dd4fd%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C6378094374027
> > 18622%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJB
> > TiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=AhBE6136UO98boBjSE3bpBSDv8
> > 2EsuvgzXbOHy%2FIM1U%3D&reserved=0
> > [2]
> > https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.
> > kernel.org%2Fdoc%2Fhtmldocs%2Fnetworking%2FAPI---netdev-alloc-skb.html
> > &data=04%7C01%7Cbiju.das.jz%40bp.renesas.com%7C5fab03422ea84da79f5
> > 308d9f44dd4fd%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C63780943740
> > 2718622%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLC
> > JBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=IqNfl5kZoQzO%2FB%2Frfq7q
> > 4QbgxKzoCy6iWB1ZXER7zO4%3D&reserved=0
> >
> > Regards,
> > Biju
> >
> >> Subject: [PATCH] ravb: Use GFP_KERNEL instead of GFP_ATOMIC when
> >> possible
> >>
> >> 'max_rx_len' can be up to GBETH_RX_BUFF_MAX (i.e. 8192) (see
> >> 'gbeth_hw_info').
> >> The default value of 'num_rx_ring' can be BE_RX_RING_SIZE (i.e. 1024).
> >>
> >> So this loop can allocate 8 Mo of memory.
> >>
> >> Previous memory allocations in this function already use GFP_KERNEL,
> >> so use __netdev_alloc_skb() and an explicit GFP_KERNEL instead of a
> >> implicit GFP_ATOMIC.
> >>
> >> This gives more opportunities of successful allocation.
> >>
> >> Signed-off-by: Christophe JAILLET <[email protected]>
> >> ---
> >> drivers/net/ethernet/renesas/ravb_main.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/net/ethernet/renesas/ravb_main.c
> >> b/drivers/net/ethernet/renesas/ravb_main.c
> >> index 24e2635c4c80..525d66f71f02 100644
> >> --- a/drivers/net/ethernet/renesas/ravb_main.c
> >> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> >> @@ -475,7 +475,7 @@ static int ravb_ring_init(struct net_device
> >> *ndev, int
> >> q)
> >> goto error;
> >>
> >> for (i = 0; i < priv->num_rx_ring[q]; i++) {
> >> - skb = netdev_alloc_skb(ndev, info->max_rx_len);
> >> + skb = __netdev_alloc_skb(ndev, info->max_rx_len, GFP_KERNEL);
> >> if (!skb)
> >> goto error;
> >> ravb_set_buffer_align(skb);
> >> --
> >> 2.32.0
> >
> >