[PATCH net 0/2] thunderx: fix receive buffer page recycling

[PATCH net 0/2] thunderx: fix receive buffer page recycling

[Dean Nelson]

In attempting to optimize receive buffer page recycling for XDP, commit
773225388dae15e72790 inadvertently introduced two problems for the non-XDP
case, that will be addressed by this patch series.

Dean Nelson (2):
  thunderx: enable page recycling for non-XDP case
  thunderx: eliminate extra calls to put_page() for pages held for recycling

 drivers/net/ethernet/cavium/thunder/nicvf_queues.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

[PATCH net 1/2] thunderx: enable page recycling for non-XDP case

[Dean Nelson]

Commit 773225388dae15e72790 added code to nicvf_alloc_page() that inadvertently
disables receive buffer page recycling for the non-XDP case by always NULL'ng
the page pointer.

This patch corrects two if-conditionals to allow for the recycling of non-XDP
mode pages by only setting the page pointer to NULL when the page is not ready
for recycling.

Signed-off-by: Dean Nelson <dnelson@redhat.com>
---
 drivers/net/ethernet/cavium/thunder/nicvf_queues.c | 23 +++++++++++-----------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_queues.c b/drivers/net/ethernet/cavium/thunder/nicvf_queues.c
index 5b4d3badcb73..55dbf02c42af 100644
--- a/drivers/net/ethernet/cavium/thunder/nicvf_queues.c
+++ b/drivers/net/ethernet/cavium/thunder/nicvf_queues.c
@@ -105,20 +105,19 @@ static inline struct pgcache *nicvf_alloc_page(struct nicvf *nic,
 	/* Check if page can be recycled */
 	if (page) {
 		ref_count = page_ref_count(page);
-		/* Check if this page has been used once i.e 'put_page'
-		 * called after packet transmission i.e internal ref_count
-		 * and page's ref_count are equal i.e page can be recycled.
+		/* This page can be recycled if internal ref_count and page's
+		 * ref_count are equal, indicating that the page has been used
+		 * once for packet transmission. For non-XDP mode, internal
+		 * ref_count is always '1'.
 		 */
-		if (rbdr->is_xdp && (ref_count == pgcache->ref_count))
-			pgcache->ref_count--;
-		else
-			page = NULL;
-
-		/* In non-XDP mode, page's ref_count needs to be '1' for it
-		 * to be recycled.
-		 */
-		if (!rbdr->is_xdp && (ref_count != 1))
+		if (rbdr->is_xdp) {
+			if (ref_count == pgcache->ref_count)
+				pgcache->ref_count--;
+			else
+				page = NULL;
+		} else if (ref_count != 1) {
 			page = NULL;
+		}
 	}
 
 	if (!page) {

Re: [PATCH net 1/2] thunderx: enable page recycling for non-XDP case

[Jesper Dangaard Brouer]

On Tue, 26 Mar 2019 10:04:47 -0400
Dean Nelson <dnelson@redhat.com> wrote:

> Commit 773225388dae15e72790 added code to nicvf_alloc_page() that inadvertently
> disables receive buffer page recycling for the non-XDP case by always NULL'ng
> the page pointer.
> 
> This patch corrects two if-conditionals to allow for the recycling of non-XDP
> mode pages by only setting the page pointer to NULL when the page is not ready
> for recycling.
> 
> Signed-off-by: Dean Nelson <dnelson@redhat.com>

You need to add a "fixes" line like this:

Fixes: 773225388dae ("net: thunderx: Optimize page recycling for XDP")

And in-general reference a commit in the same way in the description.
-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

Re: [PATCH net 1/2] thunderx: enable page recycling for non-XDP case

[Jesper Dangaard Brouer]

On Tue, 26 Mar 2019 10:04:47 -0400
Dean Nelson <dnelson@redhat.com> wrote:

> Commit 773225388dae15e72790 added code to nicvf_alloc_page() that inadvertently
> disables receive buffer page recycling for the non-XDP case by always NULL'ng
> the page pointer.
> 
> This patch corrects two if-conditionals to allow for the recycling of non-XDP
> mode pages by only setting the page pointer to NULL when the page is not ready
> for recycling.
> 
> Signed-off-by: Dean Nelson <dnelson@redhat.com>

You need to add a "fixes" line like this:

Fixes: 773225388dae ("net: thunderx: Optimize page recycling for XDP")

And in-general reference a commit in the same way in the description.
-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net 1/2] thunderx: enable page recycling for non-XDP case

[Dean Nelson]

On 3/26/19 9:39 AM, Jesper Dangaard Brouer wrote:
> On Tue, 26 Mar 2019 10:04:47 -0400
> Dean Nelson <dnelson@redhat.com> wrote:
> 
>> Commit 773225388dae15e72790 added code to nicvf_alloc_page() that inadvertently
>> disables receive buffer page recycling for the non-XDP case by always NULL'ng
>> the page pointer.
>>
>> This patch corrects two if-conditionals to allow for the recycling of non-XDP
>> mode pages by only setting the page pointer to NULL when the page is not ready
>> for recycling.
>>
>> Signed-off-by: Dean Nelson <dnelson@redhat.com>
> 
> You need to add a "fixes" line like this:
> 
> Fixes: 773225388dae ("net: thunderx: Optimize page recycling for XDP")
> 
> And in-general reference a commit in the same way in the description.

Sorry, for some reason I thought the 'Fixes' line was optional.

Thank you for setting me straight. Should I repost the patchset?

Re: [PATCH net 1/2] thunderx: enable page recycling for non-XDP case

[Dean Nelson]

On 3/26/19 9:39 AM, Jesper Dangaard Brouer wrote:
> On Tue, 26 Mar 2019 10:04:47 -0400
> Dean Nelson <dnelson@redhat.com> wrote:
> 
>> Commit 773225388dae15e72790 added code to nicvf_alloc_page() that inadvertently
>> disables receive buffer page recycling for the non-XDP case by always NULL'ng
>> the page pointer.
>>
>> This patch corrects two if-conditionals to allow for the recycling of non-XDP
>> mode pages by only setting the page pointer to NULL when the page is not ready
>> for recycling.
>>
>> Signed-off-by: Dean Nelson <dnelson@redhat.com>
> 
> You need to add a "fixes" line like this:
> 
> Fixes: 773225388dae ("net: thunderx: Optimize page recycling for XDP")
> 
> And in-general reference a commit in the same way in the description.

Sorry, for some reason I thought the 'Fixes' line was optional.

Thank you for setting me straight. Should I repost the patchset?



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net 1/2] thunderx: enable page recycling for non-XDP case

[Jesper Dangaard Brouer]

On Tue, 26 Mar 2019 09:44:19 -0500
Dean Nelson <dnelson@redhat.com> wrote:

> On 3/26/19 9:39 AM, Jesper Dangaard Brouer wrote:
> > On Tue, 26 Mar 2019 10:04:47 -0400
> > Dean Nelson <dnelson@redhat.com> wrote:
> >   
> >> Commit 773225388dae15e72790 added code to nicvf_alloc_page() that inadvertently
> >> disables receive buffer page recycling for the non-XDP case by always NULL'ng
> >> the page pointer.
> >>
> >> This patch corrects two if-conditionals to allow for the recycling of non-XDP
> >> mode pages by only setting the page pointer to NULL when the page is not ready
> >> for recycling.
> >>
> >> Signed-off-by: Dean Nelson <dnelson@redhat.com>  
> > 
> > You need to add a "fixes" line like this:
> > 
> > Fixes: 773225388dae ("net: thunderx: Optimize page recycling for XDP")
> > 
> > And in-general reference a commit in the same way in the description.  
> 
> Sorry, for some reason I thought the 'Fixes' line was optional.
> 
> Thank you for setting me straight. Should I repost the patchset?

I would say, yes please. (Given the amount of patches DaveM have to
process, lets make it easy for him).

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

Re: [PATCH net 1/2] thunderx: enable page recycling for non-XDP case

[Jesper Dangaard Brouer]

On Tue, 26 Mar 2019 09:44:19 -0500
Dean Nelson <dnelson@redhat.com> wrote:

> On 3/26/19 9:39 AM, Jesper Dangaard Brouer wrote:
> > On Tue, 26 Mar 2019 10:04:47 -0400
> > Dean Nelson <dnelson@redhat.com> wrote:
> >   
> >> Commit 773225388dae15e72790 added code to nicvf_alloc_page() that inadvertently
> >> disables receive buffer page recycling for the non-XDP case by always NULL'ng
> >> the page pointer.
> >>
> >> This patch corrects two if-conditionals to allow for the recycling of non-XDP
> >> mode pages by only setting the page pointer to NULL when the page is not ready
> >> for recycling.
> >>
> >> Signed-off-by: Dean Nelson <dnelson@redhat.com>  
> > 
> > You need to add a "fixes" line like this:
> > 
> > Fixes: 773225388dae ("net: thunderx: Optimize page recycling for XDP")
> > 
> > And in-general reference a commit in the same way in the description.  
> 
> Sorry, for some reason I thought the 'Fixes' line was optional.
> 
> Thank you for setting me straight. Should I repost the patchset?

I would say, yes please. (Given the amount of patches DaveM have to
process, lets make it easy for him).

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net 1/2] thunderx: enable page recycling for non-XDP case

[Dean Nelson]

On 3/26/19 9:59 AM, Jesper Dangaard Brouer wrote:
> On Tue, 26 Mar 2019 09:44:19 -0500
> Dean Nelson <dnelson@redhat.com> wrote:
> 
>> On 3/26/19 9:39 AM, Jesper Dangaard Brouer wrote:
>>> On Tue, 26 Mar 2019 10:04:47 -0400
>>> Dean Nelson <dnelson@redhat.com> wrote:
>>>    
>>>> Commit 773225388dae15e72790 added code to nicvf_alloc_page() that inadvertently
>>>> disables receive buffer page recycling for the non-XDP case by always NULL'ng
>>>> the page pointer.
>>>>
>>>> This patch corrects two if-conditionals to allow for the recycling of non-XDP
>>>> mode pages by only setting the page pointer to NULL when the page is not ready
>>>> for recycling.
>>>>
>>>> Signed-off-by: Dean Nelson <dnelson@redhat.com>
>>>
>>> You need to add a "fixes" line like this:
>>>
>>> Fixes: 773225388dae ("net: thunderx: Optimize page recycling for XDP")
>>>
>>> And in-general reference a commit in the same way in the description.
>>
>> Sorry, for some reason I thought the 'Fixes' line was optional.
>>
>> Thank you for setting me straight. Should I repost the patchset?
> 
> I would say, yes please. (Given the amount of patches DaveM have to
> process, lets make it easy for him).

Would be glad to, as I definitely agree.
Thanks for responding to my question.

Re: [PATCH net 1/2] thunderx: enable page recycling for non-XDP case

[Dean Nelson]

On 3/26/19 9:59 AM, Jesper Dangaard Brouer wrote:
> On Tue, 26 Mar 2019 09:44:19 -0500
> Dean Nelson <dnelson@redhat.com> wrote:
> 
>> On 3/26/19 9:39 AM, Jesper Dangaard Brouer wrote:
>>> On Tue, 26 Mar 2019 10:04:47 -0400
>>> Dean Nelson <dnelson@redhat.com> wrote:
>>>    
>>>> Commit 773225388dae15e72790 added code to nicvf_alloc_page() that inadvertently
>>>> disables receive buffer page recycling for the non-XDP case by always NULL'ng
>>>> the page pointer.
>>>>
>>>> This patch corrects two if-conditionals to allow for the recycling of non-XDP
>>>> mode pages by only setting the page pointer to NULL when the page is not ready
>>>> for recycling.
>>>>
>>>> Signed-off-by: Dean Nelson <dnelson@redhat.com>
>>>
>>> You need to add a "fixes" line like this:
>>>
>>> Fixes: 773225388dae ("net: thunderx: Optimize page recycling for XDP")
>>>
>>> And in-general reference a commit in the same way in the description.
>>
>> Sorry, for some reason I thought the 'Fixes' line was optional.
>>
>> Thank you for setting me straight. Should I repost the patchset?
> 
> I would say, yes please. (Given the amount of patches DaveM have to
> process, lets make it easy for him).

Would be glad to, as I definitely agree.
Thanks for responding to my question.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH net 2/2] thunderx: eliminate extra calls to put_page() for pages held for recycling

[Dean Nelson]

For the non-XDP case, commit 773225388dae15e72790 added code to
nicvf_free_rbdr() that, when releasing the additional receive buffer page
reference held for recycling, repeatedly calls put_page() until the page's
_refcount goes to zero. Which results in the page being freed.

This is not okay if the page's _refcount was greater than 1 (in the non-XDP
case), because nicvf_free_rbdr() should not be subtracting more than what
nicvf_alloc_page() had previously added to the page's _refcount, which was
only 1 (in the non-XDP case).

This can arise if a received packet is still being processed and the receive
buffer (i.e., skb->head) has not yet been freed via skb_free_head() when
nicvf_free_rbdr() is spinning through the aforementioned put_page() loop.

If this should occur, when the received packet finishes processing and
skb_free_head() is called, various problems can ensue. Exactly what, depends on
whether the page has already been reallocated or not, anything from "BUG: Bad
page state ... ", to "Unable to handle kernel NULL pointer dereference ..." or
"Unable to handle kernel paging request...".

So this patch changes nicvf_free_rbdr() to only call put_page() once for pages
held for recycling (in the non-XDP case).

Signed-off-by: Dean Nelson <dnelson@redhat.com>
---
 drivers/net/ethernet/cavium/thunder/nicvf_queues.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_queues.c b/drivers/net/ethernet/cavium/thunder/nicvf_queues.c
index 55dbf02c42af..e246f9733bb8 100644
--- a/drivers/net/ethernet/cavium/thunder/nicvf_queues.c
+++ b/drivers/net/ethernet/cavium/thunder/nicvf_queues.c
@@ -364,11 +364,10 @@ static void nicvf_free_rbdr(struct nicvf *nic, struct rbdr *rbdr)
 	while (head < rbdr->pgcnt) {
 		pgcache = &rbdr->pgcache[head];
 		if (pgcache->page && page_ref_count(pgcache->page) != 0) {
-			if (!rbdr->is_xdp) {
-				put_page(pgcache->page);
-				continue;
+			if (rbdr->is_xdp) {
+				page_ref_sub(pgcache->page,
+					     pgcache->ref_count - 1);
 			}
-			page_ref_sub(pgcache->page, pgcache->ref_count - 1);
 			put_page(pgcache->page);
 		}
 		head++;

Re: [PATCH net 0/2] thunderx: fix receive buffer page recycling

[Dean Nelson]

On 3/26/19 9:04 AM, Dean Nelson wrote:
> In attempting to optimize receive buffer page recycling for XDP, commit
> 773225388dae15e72790 inadvertently introduced two problems for the non-XDP
> case, that will be addressed by this patch series.
> 
> Dean Nelson (2):
>    thunderx: enable page recycling for non-XDP case
>    thunderx: eliminate extra calls to put_page() for pages held for recycling
> 
>   drivers/net/ethernet/cavium/thunder/nicvf_queues.c | 30 ++++++++++++++----------------
>   1 file changed, 14 insertions(+), 16 deletions(-)
> 
I'm self-NACK'ng this patchset in order to add the omitted 'Fixes' lines.

Re: [PATCH net 0/2] thunderx: fix receive buffer page recycling

[Dean Nelson]

On 3/26/19 9:04 AM, Dean Nelson wrote:
> In attempting to optimize receive buffer page recycling for XDP, commit
> 773225388dae15e72790 inadvertently introduced two problems for the non-XDP
> case, that will be addressed by this patch series.
> 
> Dean Nelson (2):
>    thunderx: enable page recycling for non-XDP case
>    thunderx: eliminate extra calls to put_page() for pages held for recycling
> 
>   drivers/net/ethernet/cavium/thunder/nicvf_queues.c | 30 ++++++++++++++----------------
>   1 file changed, 14 insertions(+), 16 deletions(-)
> 
I'm self-NACK'ng this patchset in order to add the omitted 'Fixes' lines.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel