[PATCH net-next] nfp: flower: prioritize stats updates

[PATCH net-next] nfp: flower: prioritize stats updates

[Jakub Kicinski]

From: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>

Previously it was possible to interrupt processing stats updates because
they were handled in a work queue. Interrupting the stats updates could
lead to a situation where we backup the control message queue. This patch
moves the stats update processing out of the work queue to be processed as
soon as hardware sends a request.

Reported-by: Louis Peens <louis.peens@netronome.com>
Signed-off-by: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
Reviewed-by: Dirk van der Merwe <dirk.vandermerwe@netronome.com>
Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 drivers/net/ethernet/netronome/nfp/flower/cmsg.c | 29 +++++++++++++++---------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/netronome/nfp/flower/cmsg.c b/drivers/net/ethernet/netronome/nfp/flower/cmsg.c
index 615314d9e7c6..baaea6f1a9d8 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/cmsg.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/cmsg.c
@@ -211,12 +211,6 @@ nfp_flower_cmsg_process_one_rx(struct nfp_app *app, struct sk_buff *skb)
 
 	cmsg_hdr = nfp_flower_cmsg_get_hdr(skb);
 
-	if (unlikely(cmsg_hdr->version != NFP_FLOWER_CMSG_VER1)) {
-		nfp_flower_cmsg_warn(app, "Cannot handle repr control version %u\n",
-				     cmsg_hdr->version);
-		goto out;
-	}
-
 	type = cmsg_hdr->type;
 	switch (type) {
 	case NFP_FLOWER_CMSG_TYPE_PORT_REIFY:
@@ -225,9 +219,6 @@ nfp_flower_cmsg_process_one_rx(struct nfp_app *app, struct sk_buff *skb)
 	case NFP_FLOWER_CMSG_TYPE_PORT_MOD:
 		nfp_flower_cmsg_portmod_rx(app, skb);
 		break;
-	case NFP_FLOWER_CMSG_TYPE_FLOW_STATS:
-		nfp_flower_rx_flow_stats(app, skb);
-		break;
 	case NFP_FLOWER_CMSG_TYPE_NO_NEIGH:
 		nfp_tunnel_request_route(app, skb);
 		break;
@@ -263,7 +254,23 @@ void nfp_flower_cmsg_process_rx(struct work_struct *work)
 void nfp_flower_cmsg_rx(struct nfp_app *app, struct sk_buff *skb)
 {
 	struct nfp_flower_priv *priv = app->priv;
+	struct nfp_flower_cmsg_hdr *cmsg_hdr;
+
+	cmsg_hdr = nfp_flower_cmsg_get_hdr(skb);
+
+	if (unlikely(cmsg_hdr->version != NFP_FLOWER_CMSG_VER1)) {
+		nfp_flower_cmsg_warn(app, "Cannot handle repr control version %u\n",
+				     cmsg_hdr->version);
+		dev_kfree_skb_any(skb);
+		return;
+	}
 
-	skb_queue_tail(&priv->cmsg_skbs, skb);
-	schedule_work(&priv->cmsg_work);
+	if (cmsg_hdr->type == NFP_FLOWER_CMSG_TYPE_FLOW_STATS) {
+		/* We need to deal with stats updates from HW asap */
+		nfp_flower_rx_flow_stats(app, skb);
+		dev_consume_skb_any(skb);
+	} else {
+		skb_queue_tail(&priv->cmsg_skbs, skb);
+		schedule_work(&priv->cmsg_work);
+	}
 }
-- 
2.15.1

Re: [PATCH net-next] nfp: flower: prioritize stats updates

[David Miller]

From: Jakub Kicinski <jakub.kicinski@netronome.com>
Date: Fri, 19 Jan 2018 17:54:08 -0800

> From: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
> 
> Previously it was possible to interrupt processing stats updates because
> they were handled in a work queue. Interrupting the stats updates could
> lead to a situation where we backup the control message queue. This patch
> moves the stats update processing out of the work queue to be processed as
> soon as hardware sends a request.
> 
> Reported-by: Louis Peens <louis.peens@netronome.com>
> Signed-off-by: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
> Reviewed-by: Dirk van der Merwe <dirk.vandermerwe@netronome.com>
> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>

Applied, thanks Jakub.

Just a reminder that it really makes things more difficult that one
can't just go:

	make drivers/net/ethernet/netronome/nfp/flower/cmsg.o

to test a specific NFP object build like one can with pretty much the
rest of the entire kernel tree.

Re: [PATCH net-next] nfp: flower: prioritize stats updates

[Jakub Kicinski]

On Sun, Jan 21, 2018 at 3:10 PM, David Miller <davem@davemloft.net> wrote:
> From: Jakub Kicinski <jakub.kicinski@netronome.com>
> Date: Fri, 19 Jan 2018 17:54:08 -0800
>
>> From: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
>>
>> Previously it was possible to interrupt processing stats updates because
>> they were handled in a work queue. Interrupting the stats updates could
>> lead to a situation where we backup the control message queue. This patch
>> moves the stats update processing out of the work queue to be processed as
>> soon as hardware sends a request.
>>
>> Reported-by: Louis Peens <louis.peens@netronome.com>
>> Signed-off-by: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
>> Reviewed-by: Dirk van der Merwe <dirk.vandermerwe@netronome.com>
>> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>
> Applied, thanks Jakub.

Thank you!

> Just a reminder that it really makes things more difficult that one
> can't just go:
>
>         make drivers/net/ethernet/netronome/nfp/flower/cmsg.o
>
> to test a specific NFP object build like one can with pretty much the
> rest of the entire kernel tree.

Oh, thanks for letting me know, I wasn't aware the build system seems
to require a separate Makefile in each directory..  I'll fix it first
thing in the morning!

Re: [PATCH net-next] nfp: flower: prioritize stats updates

[Jakub Kicinski]

On Sun, 21 Jan 2018 23:53:05 -0800, Jakub Kicinski wrote:
> > Just a reminder that it really makes things more difficult that one
> > can't just go:
> >
> >         make drivers/net/ethernet/netronome/nfp/flower/cmsg.o
> >
> > to test a specific NFP object build like one can with pretty much the
> > rest of the entire kernel tree.  
> 
> Oh, thanks for letting me know, I wasn't aware the build system seems
> to require a separate Makefile in each directory..  I'll fix it first
> thing in the morning!

I failed to find a way to describe a multiple multi-dir module with
Kbuild cleanly.  I asked on linux-kbuild:

https://www.spinics.net/lists/linux-kbuild/msg16509.html

and unless someone speaks up I'll just create empty Makefiles :(