[dpdk-dev] [PATCH] nfp: avoiding concurrency when hardware reconfig

[dpdk-dev] [PATCH] nfp: avoiding concurrency when hardware reconfig

[Alejandro Lucero]

Some apps calling some functions from different threads at the
same time could lead to reconfig problems. Reconfig mechanism is
based on a hardware queue where incrementing a counter signals the
firmware to do the reconfig. If there are two increments before the
first one has been processed the firmware will stop and a device
reset is necessary.

Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
---
 drivers/net/nfp/nfp_net.c     | 8 ++++++++
 drivers/net/nfp/nfp_net_pmd.h | 1 +
 2 files changed, 9 insertions(+)

diff --git a/drivers/net/nfp/nfp_net.c b/drivers/net/nfp/nfp_net.c
index bc0a3d8..ba0ee04 100644
--- a/drivers/net/nfp/nfp_net.c
+++ b/drivers/net/nfp/nfp_net.c
@@ -58,6 +58,7 @@
 #include "nfp_net_pmd.h"
 #include "nfp_net_logs.h"
 #include "nfp_net_ctrl.h"
+#include <rte_spinlock.h>
 
 /* Prototypes */
 static void nfp_net_close(struct rte_eth_dev *dev);
@@ -407,6 +408,8 @@ nfp_net_reconfig(struct nfp_net_hw *hw, uint32_t ctrl, uint32_t update)
 	PMD_DRV_LOG(DEBUG, "nfp_net_reconfig: ctrl=%08x update=%08x\n",
 		    ctrl, update);
 
+	rte_spinlock_lock(&hw->reconfig_lock);
+
 	nn_cfg_writel(hw, NFP_NET_CFG_CTRL, ctrl);
 	nn_cfg_writel(hw, NFP_NET_CFG_UPDATE, update);
 
@@ -414,6 +417,8 @@ nfp_net_reconfig(struct nfp_net_hw *hw, uint32_t ctrl, uint32_t update)
 
 	err = __nfp_net_reconfig(hw, update);
 
+	rte_spinlock_unlock(&hw->reconfig_lock);
+
 	if (!err)
 		return 0;
 
@@ -2399,6 +2404,9 @@ nfp_net_init(struct rte_eth_dev *eth_dev)
 	PMD_INIT_LOG(INFO, "max_rx_queues: %u, max_tx_queues: %u\n",
 		     hw->max_rx_queues, hw->max_tx_queues);
 
+	/* Initializing spinlock for reconfigs */
+	rte_spinlock_init(&hw->reconfig_lock);
+
 	/* Allocating memory for mac addr */
 	eth_dev->data->mac_addrs = rte_zmalloc("mac_addr", ETHER_ADDR_LEN, 0);
 	if (eth_dev->data->mac_addrs == NULL) {
diff --git a/drivers/net/nfp/nfp_net_pmd.h b/drivers/net/nfp/nfp_net_pmd.h
index 232ce5c..c180972 100644
--- a/drivers/net/nfp/nfp_net_pmd.h
+++ b/drivers/net/nfp/nfp_net_pmd.h
@@ -406,6 +406,7 @@ struct nfp_net_hw {
 	int stride_tx;
 
 	uint8_t *qcp_cfg;
+	rte_spinlock_t reconfig_lock;
 
 	uint32_t max_tx_queues;
 	uint32_t max_rx_queues;
-- 
1.9.1

Re: [dpdk-dev] [PATCH] nfp: avoiding concurrency when hardware reconfig

[Bruce Richardson]

On Tue, Apr 26, 2016 at 01:14:15PM +0100, Alejandro Lucero wrote:
> Some apps calling some functions from different threads at the
> same time could lead to reconfig problems. Reconfig mechanism is
> based on a hardware queue where incrementing a counter signals the
> firmware to do the reconfig. If there are two increments before the
> first one has been processed the firmware will stop and a device
> reset is necessary.
> 
> Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
> ---
>  drivers/net/nfp/nfp_net.c     | 8 ++++++++
>  drivers/net/nfp/nfp_net_pmd.h | 1 +
>  2 files changed, 9 insertions(+)
> 
> diff --git a/drivers/net/nfp/nfp_net.c b/drivers/net/nfp/nfp_net.c
> index bc0a3d8..ba0ee04 100644
> --- a/drivers/net/nfp/nfp_net.c
> +++ b/drivers/net/nfp/nfp_net.c
> @@ -58,6 +58,7 @@
>  #include "nfp_net_pmd.h"
>  #include "nfp_net_logs.h"
>  #include "nfp_net_ctrl.h"
> +#include <rte_spinlock.h>

Hi Alejandro,

I think this header addition is in the wrong place in the code. When I apply
this patch to next-net and try a recompile I get the error:

  CC nfp_net.o
  In file included from /home/bruce/next-net/dpdk-next-net/drivers/net/nfp/nfp_net.c:58:0:
  /home/bruce/next-net/dpdk-next-net/drivers/net/nfp/nfp_net_pmd.h:409:2: error: unknown type name ‘rte_spinlock_t’
    rte_spinlock_t reconfig_lock;
    ^

You either need to put the spinlock include before the nfp_net_pmd.h include
or, perhaps better, put the spinlock include inside the nfp_net_pmd header file
since that is where the spinlock variable is being defined.

/Bruce

Re: [dpdk-dev] [PATCH] nfp: avoiding concurrency when hardware reconfig

[Alejandro Lucero]

Hi Bruce,

Sorry about this. I sent a v2 for this patch but not in the same thread:

http://www.dpdk.org/ml/archives/dev/2016-April/037996.html

On Tue, May 3, 2016 at 12:01 PM, Bruce Richardson <
bruce.richardson@intel.com> wrote:

> On Tue, Apr 26, 2016 at 01:14:15PM +0100, Alejandro Lucero wrote:
> > Some apps calling some functions from different threads at the
> > same time could lead to reconfig problems. Reconfig mechanism is
> > based on a hardware queue where incrementing a counter signals the
> > firmware to do the reconfig. If there are two increments before the
> > first one has been processed the firmware will stop and a device
> > reset is necessary.
> >
> > Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
> > ---
> >  drivers/net/nfp/nfp_net.c     | 8 ++++++++
> >  drivers/net/nfp/nfp_net_pmd.h | 1 +
> >  2 files changed, 9 insertions(+)
> >
> > diff --git a/drivers/net/nfp/nfp_net.c b/drivers/net/nfp/nfp_net.c
> > index bc0a3d8..ba0ee04 100644
> > --- a/drivers/net/nfp/nfp_net.c
> > +++ b/drivers/net/nfp/nfp_net.c
> > @@ -58,6 +58,7 @@
> >  #include "nfp_net_pmd.h"
> >  #include "nfp_net_logs.h"
> >  #include "nfp_net_ctrl.h"
> > +#include <rte_spinlock.h>
>
> Hi Alejandro,
>
> I think this header addition is in the wrong place in the code. When I
> apply
> this patch to next-net and try a recompile I get the error:
>
>   CC nfp_net.o
>   In file included from
> /home/bruce/next-net/dpdk-next-net/drivers/net/nfp/nfp_net.c:58:0:
>   /home/bruce/next-net/dpdk-next-net/drivers/net/nfp/nfp_net_pmd.h:409:2:
> error: unknown type name ‘rte_spinlock_t’
>     rte_spinlock_t reconfig_lock;
>     ^
>
> You either need to put the spinlock include before the nfp_net_pmd.h
> include
> or, perhaps better, put the spinlock include inside the nfp_net_pmd header
> file
> since that is where the spinlock variable is being defined.
>
> /Bruce
>
>

Re: [dpdk-dev] [PATCH] nfp: avoiding concurrency when hardware reconfig

[Bruce Richardson]

On Tue, May 03, 2016 at 12:14:26PM +0100, Alejandro Lucero wrote:
> Hi Bruce,
> 
> Sorry about this. I sent a v2 for this patch but not in the same thread:
> 
> http://www.dpdk.org/ml/archives/dev/2016-April/037996.html
> 

Ok, I see it in patchwork now, thanks.

When sending a v2, please keep the mails threaded, and please also update the
original patch submission in patchwork to "superceded" so that it drops off the
list of patches for consideration in DPDK. [That way at least if you forget one,
the other will help hint to committers that they are not looking at the latest
version. :-)]

/Bruce

> On Tue, May 3, 2016 at 12:01 PM, Bruce Richardson <
> bruce.richardson@intel.com> wrote:
> 
> > On Tue, Apr 26, 2016 at 01:14:15PM +0100, Alejandro Lucero wrote:
> > > Some apps calling some functions from different threads at the
> > > same time could lead to reconfig problems. Reconfig mechanism is
> > > based on a hardware queue where incrementing a counter signals the
> > > firmware to do the reconfig. If there are two increments before the
> > > first one has been processed the firmware will stop and a device
> > > reset is necessary.
> > >
> > > Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
> > > ---
> > >  drivers/net/nfp/nfp_net.c     | 8 ++++++++
> > >  drivers/net/nfp/nfp_net_pmd.h | 1 +
> > >  2 files changed, 9 insertions(+)
> > >
> > > diff --git a/drivers/net/nfp/nfp_net.c b/drivers/net/nfp/nfp_net.c
> > > index bc0a3d8..ba0ee04 100644
> > > --- a/drivers/net/nfp/nfp_net.c
> > > +++ b/drivers/net/nfp/nfp_net.c
> > > @@ -58,6 +58,7 @@
> > >  #include "nfp_net_pmd.h"
> > >  #include "nfp_net_logs.h"
> > >  #include "nfp_net_ctrl.h"
> > > +#include <rte_spinlock.h>
> >
> > Hi Alejandro,
> >
> > I think this header addition is in the wrong place in the code. When I
> > apply
> > this patch to next-net and try a recompile I get the error:
> >
> >   CC nfp_net.o
> >   In file included from
> > /home/bruce/next-net/dpdk-next-net/drivers/net/nfp/nfp_net.c:58:0:
> >   /home/bruce/next-net/dpdk-next-net/drivers/net/nfp/nfp_net_pmd.h:409:2:
> > error: unknown type name ‘rte_spinlock_t’
> >     rte_spinlock_t reconfig_lock;
> >     ^
> >
> > You either need to put the spinlock include before the nfp_net_pmd.h
> > include
> > or, perhaps better, put the spinlock include inside the nfp_net_pmd header
> > file
> > since that is where the spinlock variable is being defined.
> >
> > /Bruce
> >
> >