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

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

[Dean Nelson]

In attempting to optimize receive buffer page recycling for XDP, commit
773225388dae15e72790 ("net: thunderx: Optimize page recycling for XDP")
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 v2 1/2] thunderx: enable page recycling for non-XDP case

[Dean Nelson]

Commit 773225388dae15e72790 ("net: thunderx: Optimize page recycling for XDP")
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.

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

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) {

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

[Dean Nelson]

For the non-XDP case, commit 773225388dae15e72790 ("net: thunderx: Optimize
page recycling for XDP") 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).

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

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 v2 0/2] thunderx: fix receive buffer page recycling

[Dean Nelson]

On 3/26/19 10:53 AM, Dean Nelson wrote:
> In attempting to optimize receive buffer page recycling for XDP, commit
> 773225388dae15e72790 ("net: thunderx: Optimize page recycling for XDP")
> 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 really sorry, I completely forgot to add a description
of what the changes were made by v2 (which was just to add
the 'Fixes:' lines to the changelogs).

And I forgot to add you to the CC-list. I'm assuming I
should repost? Right? (And do you want to be added?)

It's just one of those days.

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

[Jesper Dangaard Brouer]

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

> On 3/26/19 10:53 AM, Dean Nelson wrote:
> > In attempting to optimize receive buffer page recycling for XDP, commit
> > 773225388dae15e72790 ("net: thunderx: Optimize page recycling for XDP")
> > 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 really sorry, I completely forgot to add a description
> of what the changes were made by v2 (which was just to add
> the 'Fixes:' lines to the changelogs).

That should be okay, for such a small change.

> And I forgot to add you to the CC-list. I'm assuming I
> should repost? Right? (And do you want to be added?)
> 
> It's just one of those days.

I can live with not getting Cc'ed (but I would prefer it), but some
other adjustments.

There should not be a newline between "Fixes:" and "Signed-off-by:".
And nitpicking, I believe that we usually only show the first 12 chars
of the commit id.  I have setup git to use: 'git show --pretty=fixes'

From my ~/.gitconfig:
---------------------
[core]
        abbrev = 12

[pretty]
        fixes = Fixes: %h (\"%s\")


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

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

[Dean Nelson]

On 3/26/19 11:18 AM, Jesper Dangaard Brouer wrote:
> On Tue, 26 Mar 2019 11:09:10 -0500
> Dean Nelson <dnelson@redhat.com> wrote:
> 
>> On 3/26/19 10:53 AM, Dean Nelson wrote:
>>> In attempting to optimize receive buffer page recycling for XDP, commit
>>> 773225388dae15e72790 ("net: thunderx: Optimize page recycling for XDP")
>>> 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 really sorry, I completely forgot to add a description
>> of what the changes were made by v2 (which was just to add
>> the 'Fixes:' lines to the changelogs).
> 
> That should be okay, for such a small change.

Okay.


>> And I forgot to add you to the CC-list. I'm assuming I
>> should repost? Right? (And do you want to be added?)
>>
>> It's just one of those days.
> 
> I can live with not getting Cc'ed (but I would prefer it), but some
> other adjustments.

By your phrase "but some other adjustments", are you indicating that
I should post a v3 with the below items addressed (and while I'm at
it fix the above two)? Or is the following just for my education for the
future?

> 
> There should not be a newline between "Fixes:" and "Signed-off-by:".

Good to know. I'd actually searched netdev mailing list for examples.
And the one patch I happened to look at, had a blank line separating it
and the "Signed-off-by:" line that followed. And just looking again, I
saw a couple that way. But they're definitely in the minority.

> And nitpicking, I believe that we usually only show the first 12 chars
> of the commit id.  I have setup git to use: 'git show --pretty=fixes'

I had looked at Documentation/process/submitting-patches.rst and had
read the following...

   If you want to refer to a specific commit, don't just refer to the
   SHA-1 ID of the commit. Please also include the oneline summary of
   the commit, to make it easier for reviewers to know what it is about.
   Example::

           Commit e21d2170f36602ae2708 ("video: remove unnecessary
           platform_set_drvdata()") removed the unnecessary
           platform_set_drvdata(), but left the variable "dev" unused,
           delete it.

   You should also be sure to use at least the first twelve characters 
of the
   SHA-1 ID.  The kernel repository holds a *lot* of objects, making
   collisions with shorter IDs a real possibility.  Bear in mind that, 
even if
   there is no collision with your six-character ID now, that condition may
   change five years from now.

Which has the first 20 chars in its example, and speaks of using at
least 12 chars.

So now I just went back and re-read that section and the paragraph that
followed it (which previously had been outside of the view of my
terminal window)...

   If your patch fixes a bug in a specific commit, e.g. you found an 
issue using
   ``git bisect``, please use the 'Fixes:' tag with the first 12 
characters of
   the SHA-1 ID, and the one line summary.  Do not split the tag across 
multiple
   lines, tags are exempt from the "wrap at 75 columns" rule in order to 
simplify
   parsing scripts.  For example::

           Fixes: 54a4f0239f2e ("KVM: MMU: make kvm_mmu_zap_page() 
return the number of pages it actually freed")

Which does say that you are correct. Just the first 12 chars should be
used.

Like I said earlier... just one of those days.  :-)



> 
>  From my ~/.gitconfig:
> ---------------------
> [core]
>          abbrev = 12
> 
> [pretty]
>          fixes = Fixes: %h (\"%s\")
> 
> 

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

[David Miller]

From: Dean Nelson <dnelson@redhat.com>
Date: 

> In attempting to optimize receive buffer page recycling for XDP, commit
> 773225388dae15e72790 ("net: thunderx: Optimize page recycling for XDP")
> inadvertently introduced two problems for the non-XDP case, that will be
> addressed by this patch series.

Series applied and queued up for -stable.