* [PATCH] ravb: Use GFP_KERNEL instead of GFP_ATOMIC when possible
@ 2022-02-20 7:27 Christophe JAILLET
2022-02-20 7:53 ` Biju Das
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Christophe JAILLET @ 2022-02-20 7:27 UTC (permalink / raw)
To: Sergey Shtylyov, David S. Miller, Jakub Kicinski
Cc: linux-kernel, kernel-janitors, Christophe JAILLET, netdev,
linux-renesas-soc
'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 <christophe.jaillet@wanadoo.fr>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* RE: [PATCH] ravb: Use GFP_KERNEL instead of GFP_ATOMIC when possible 2022-02-20 7:27 [PATCH] ravb: Use GFP_KERNEL instead of GFP_ATOMIC when possible Christophe JAILLET @ 2022-02-20 7:53 ` Biju Das 2022-02-20 8:48 ` Christophe JAILLET 2022-02-20 10:03 ` Sergey Shtylyov 2022-02-21 12:10 ` patchwork-bot+netdevbpf 2 siblings, 1 reply; 6+ messages in thread From: Biju Das @ 2022-02-20 7:53 UTC (permalink / raw) To: Christophe JAILLET, Sergey Shtylyov, David S. Miller, Jakub Kicinski Cc: linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org, netdev@vger.kernel.org, linux-renesas-soc@vger.kernel.org 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 <christophe.jaillet@wanadoo.fr> > --- > 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ravb: Use GFP_KERNEL instead of GFP_ATOMIC when possible 2022-02-20 7:53 ` Biju Das @ 2022-02-20 8:48 ` Christophe JAILLET 2022-02-20 11:32 ` Biju Das 0 siblings, 1 reply; 6+ messages in thread From: Christophe JAILLET @ 2022-02-20 8:48 UTC (permalink / raw) To: Biju Das, Sergey Shtylyov, David S. Miller, Jakub Kicinski Cc: linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org, netdev@vger.kernel.org, linux-renesas-soc@vger.kernel.org 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 <christophe.jaillet@wanadoo.fr> >> --- >> 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 > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] ravb: Use GFP_KERNEL instead of GFP_ATOMIC when possible 2022-02-20 8:48 ` Christophe JAILLET @ 2022-02-20 11:32 ` Biju Das 0 siblings, 0 replies; 6+ messages in thread From: Biju Das @ 2022-02-20 11:32 UTC (permalink / raw) To: Christophe JAILLET, Sergey Shtylyov, David S. Miller, Jakub Kicinski Cc: linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org, netdev@vger.kernel.org, linux-renesas-soc@vger.kernel.org Hi Christophe, Thanks for your clarification. Patch Looks good to me. Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com> Cheers, Biju > -----Original Message----- > From: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > Sent: 20 February 2022 08:49 > To: Biju Das <biju.das.jz@bp.renesas.com>; Sergey Shtylyov > <s.shtylyov@omp.ru>; David S. Miller <davem@davemloft.net>; Jakub Kicinski > <kuba@kernel.org> > Cc: linux-kernel@vger.kernel.org; kernel-janitors@vger.kernel.org; > netdev@vger.kernel.org; linux-renesas-soc@vger.kernel.org > 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 <christophe.jaillet@wanadoo.fr> > >> --- > >> 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 > > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ravb: Use GFP_KERNEL instead of GFP_ATOMIC when possible 2022-02-20 7:27 [PATCH] ravb: Use GFP_KERNEL instead of GFP_ATOMIC when possible Christophe JAILLET 2022-02-20 7:53 ` Biju Das @ 2022-02-20 10:03 ` Sergey Shtylyov 2022-02-21 12:10 ` patchwork-bot+netdevbpf 2 siblings, 0 replies; 6+ messages in thread From: Sergey Shtylyov @ 2022-02-20 10:03 UTC (permalink / raw) To: Christophe JAILLET, David S. Miller, Jakub Kicinski Cc: linux-kernel, kernel-janitors, netdev, linux-renesas-soc Hello! On 2/20/22 10:27 AM, Christophe JAILLET wrote: > '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. MB? :-) > > 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 <christophe.jaillet@wanadoo.fr> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru> [...] MBR, Sergey ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ravb: Use GFP_KERNEL instead of GFP_ATOMIC when possible 2022-02-20 7:27 [PATCH] ravb: Use GFP_KERNEL instead of GFP_ATOMIC when possible Christophe JAILLET 2022-02-20 7:53 ` Biju Das 2022-02-20 10:03 ` Sergey Shtylyov @ 2022-02-21 12:10 ` patchwork-bot+netdevbpf 2 siblings, 0 replies; 6+ messages in thread From: patchwork-bot+netdevbpf @ 2022-02-21 12:10 UTC (permalink / raw) To: Christophe JAILLET Cc: s.shtylyov, davem, kuba, linux-kernel, kernel-janitors, netdev, linux-renesas-soc Hello: This patch was applied to netdev/net-next.git (master) by David S. Miller <davem@davemloft.net>: On Sun, 20 Feb 2022 08:27:15 +0100 you wrote: > '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. > > [...] Here is the summary with links: - ravb: Use GFP_KERNEL instead of GFP_ATOMIC when possible https://git.kernel.org/netdev/net-next/c/91398a960edf You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-02-21 12:13 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-02-20 7:27 [PATCH] ravb: Use GFP_KERNEL instead of GFP_ATOMIC when possible Christophe JAILLET 2022-02-20 7:53 ` Biju Das 2022-02-20 8:48 ` Christophe JAILLET 2022-02-20 11:32 ` Biju Das 2022-02-20 10:03 ` Sergey Shtylyov 2022-02-21 12:10 ` patchwork-bot+netdevbpf
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox