[dpdk-dev] [PATCH] net/nfp: fix lock file usage

[dpdk-dev] [PATCH] net/nfp: fix lock file usage

[Alejandro Lucero]

DPDK apps can be executed as non-root users but current NFP lock
file for avoiding concurrent accesses to CPP interface is precluding
this option or requires to modify system file permissions.

When the NFP device is bound to VFIO, this driver does not allow this
concurrent access, so the lock file is not required at all.

OVS-DPDK as executed in RedHat distributions is the main NFP user
needing this fix.

Fixes: c7e9729da6b5 ("net/nfp: support CPP")

Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
---
 drivers/net/nfp/nfp_net.c                  | 13 ++++++++++++-
 drivers/net/nfp/nfpcore/nfp_cpp.h          |  5 ++++-
 drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c | 11 +++++++----
 drivers/net/nfp/nfpcore/nfp_cppcore.c      |  7 ++++---
 4 files changed, 27 insertions(+), 9 deletions(-)

diff --git a/drivers/net/nfp/nfp_net.c b/drivers/net/nfp/nfp_net.c
index 34a8cad..faad1ee 100644
--- a/drivers/net/nfp/nfp_net.c
+++ b/drivers/net/nfp/nfp_net.c
@@ -3118,7 +3118,18 @@ static int nfp_pf_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 	if (!dev)
 		return ret;
 
-	cpp = nfp_cpp_from_device_name(dev->device.name);
+	/*
+	 * When device bound to UIO, the device could be used, by mistake,
+	 * by two DPDK apps, and the UIO driver does not avoid it. This
+	 * could lead to a serious problem when configuring the NFP CPP
+	 * interface. Here we avoid this telling to the CPP init code to
+	 * use a lock file if UIO is being used.
+	 */
+	if (dev->kdrv == RTE_KDRV_VFIO)
+		cpp = nfp_cpp_from_device_name(dev->device.name, 0);
+	else
+		cpp = nfp_cpp_from_device_name(dev->device.name, 1);
+
 	if (!cpp) {
 		PMD_DRV_LOG(ERR, "A CPP handle can not be obtained");
 		ret = -EIO;
diff --git a/drivers/net/nfp/nfpcore/nfp_cpp.h b/drivers/net/nfp/nfpcore/nfp_cpp.h
index 7e86214..de2ff84 100644
--- a/drivers/net/nfp/nfpcore/nfp_cpp.h
+++ b/drivers/net/nfp/nfpcore/nfp_cpp.h
@@ -31,6 +31,8 @@ struct nfp_cpp {
 	 * island XPB CSRs.
 	 */
 	uint32_t imb_cat_table[16];
+
+	int driver_lock_needed;
 };
 
 /*
@@ -179,7 +181,8 @@ int nfp_cpp_serial_set(struct nfp_cpp *cpp, const uint8_t *serial,
  *
  * @return NFP CPP handle, or NULL on failure (and set errno accordingly).
  */
-struct nfp_cpp *nfp_cpp_from_device_name(const char *devname);
+struct nfp_cpp *nfp_cpp_from_device_name(const char *devname,
+					 int driver_lock_needed);
 
 /*
  * Free a NFP CPP handle
diff --git a/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c b/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c
index 52b2948..2f5e7f6 100644
--- a/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c
+++ b/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c
@@ -850,9 +850,11 @@ struct nfp6000_area_priv {
 	memset(desc->busdev, 0, BUSDEV_SZ);
 	strlcpy(desc->busdev, devname, sizeof(desc->busdev));
 
-	ret = nfp_acquire_process_lock(desc);
-	if (ret)
-		return -1;
+	if (cpp->driver_lock_needed) {
+		ret = nfp_acquire_process_lock(desc);
+		if (ret)
+			return -1;
+	}
 
 	snprintf(tmp_str, sizeof(tmp_str), "%s/%s/driver", PCI_DEVICES,
 		 desc->busdev);
@@ -912,7 +914,8 @@ struct nfp6000_area_priv {
 		if (desc->bar[x - 1].iomem)
 			munmap(desc->bar[x - 1].iomem, 1 << (desc->barsz - 3));
 	}
-	close(desc->lock);
+	if (cpp->driver_lock_needed)
+		close(desc->lock);
 	close(desc->device);
 	free(desc);
 }
diff --git a/drivers/net/nfp/nfpcore/nfp_cppcore.c b/drivers/net/nfp/nfpcore/nfp_cppcore.c
index 94d4a0b..f61143f 100644
--- a/drivers/net/nfp/nfpcore/nfp_cppcore.c
+++ b/drivers/net/nfp/nfpcore/nfp_cppcore.c
@@ -542,7 +542,7 @@ struct nfp_cpp_area *
 }
 
 static struct nfp_cpp *
-nfp_cpp_alloc(const char *devname)
+nfp_cpp_alloc(const char *devname, int driver_lock_needed)
 {
 	const struct nfp_cpp_operations *ops;
 	struct nfp_cpp *cpp;
@@ -558,6 +558,7 @@ struct nfp_cpp_area *
 		return NULL;
 
 	cpp->op = ops;
+	cpp->driver_lock_needed = driver_lock_needed;
 
 	if (cpp->op->init) {
 		err = cpp->op->init(cpp, devname);
@@ -603,9 +604,9 @@ struct nfp_cpp_area *
 }
 
 struct nfp_cpp *
-nfp_cpp_from_device_name(const char *devname)
+nfp_cpp_from_device_name(const char *devname, int driver_lock_needed)
 {
-	return nfp_cpp_alloc(devname);
+	return nfp_cpp_alloc(devname, driver_lock_needed);
 }
 
 /*
-- 
1.9.1

Re: [dpdk-dev] [PATCH] net/nfp: fix lock file usage

[Ferruh Yigit]

On 5/23/2018 1:28 PM, Alejandro Lucero wrote:
> DPDK apps can be executed as non-root users but current NFP lock
> file for avoiding concurrent accesses to CPP interface is precluding
> this option or requires to modify system file permissions.
> 
> When the NFP device is bound to VFIO, this driver does not allow this
> concurrent access, so the lock file is not required at all.
> 
> OVS-DPDK as executed in RedHat distributions is the main NFP user
> needing this fix.
> 
> Fixes: c7e9729da6b5 ("net/nfp: support CPP")
> 
> Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>

Hi Alejandro,

As far as I understand this is to fix a common use case for nfp, but it looks
like there is already a workaround and only for non-root users.

What is the priority of the patch, only critical but fixes allowed at this
point, can we push this one to next release?

Thanks,
ferruh

Re: [dpdk-dev] [PATCH] net/nfp: fix lock file usage

[Alejandro Lucero]

On Wed, May 23, 2018 at 4:57 PM, Ferruh Yigit <ferruh.yigit@intel.com>
wrote:

> On 5/23/2018 1:28 PM, Alejandro Lucero wrote:
> > DPDK apps can be executed as non-root users but current NFP lock
> > file for avoiding concurrent accesses to CPP interface is precluding
> > this option or requires to modify system file permissions.
> >
> > When the NFP device is bound to VFIO, this driver does not allow this
> > concurrent access, so the lock file is not required at all.
> >
> > OVS-DPDK as executed in RedHat distributions is the main NFP user
> > needing this fix.
> >
> > Fixes: c7e9729da6b5 ("net/nfp: support CPP")
> >
> > Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
>
> Hi Alejandro,
>
> As far as I understand this is to fix a common use case for nfp, but it
> looks
> like there is already a workaround and only for non-root users.
>
>
There is a patch submitted to stable versions because this lock was also
with the old NSPU interface, but as far as I know, there is no patch yet
for the current upstream tip.



> What is the priority of the patch, only critical but fixes allowed at this
> point, can we push this one to next release?
>

This is critical for us because RedHat wants to support OVS with our card,
and when OVS-DPDK is used, this problem is precluding non-root users to
execute OVS-DPDK.


>
> Thanks,
> ferruh
>
>
>

Re: [dpdk-dev] [PATCH] net/nfp: fix lock file usage

[Ferruh Yigit]

On 5/23/2018 5:50 PM, Alejandro Lucero wrote:
> 
> 
> On Wed, May 23, 2018 at 4:57 PM, Ferruh Yigit <ferruh.yigit@intel.com
> <mailto:ferruh.yigit@intel.com>> wrote:
> 
>     On 5/23/2018 1:28 PM, Alejandro Lucero wrote:
>     > DPDK apps can be executed as non-root users but current NFP lock
>     > file for avoiding concurrent accesses to CPP interface is precluding
>     > this option or requires to modify system file permissions.
>     > 
>     > When the NFP device is bound to VFIO, this driver does not allow this
>     > concurrent access, so the lock file is not required at all.
>     > 
>     > OVS-DPDK as executed in RedHat distributions is the main NFP user
>     > needing this fix.
>     > 
>     > Fixes: c7e9729da6b5 ("net/nfp: support CPP")
>     > 
>     > Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com <mailto:alejandro.lucero@netronome.com>>
> 
>     Hi Alejandro,
> 
>     As far as I understand this is to fix a common use case for nfp, but it looks
>     like there is already a workaround and only for non-root users.
> 
> 
> There is a patch submitted to stable versions because this lock was also with
> the old NSPU interface, but as far as I know, there is no patch yet for the
> current upstream tip.
> 
>  
> 
>     What is the priority of the patch, only critical but fixes allowed at this
>     point, can we push this one to next release?
> 
> 
> This is critical for us because RedHat wants to support OVS with our card, and
> when OVS-DPDK is used, this problem is precluding non-root users to execute
> OVS-DPDK.

What exactly this lock for? Does it to prevent multiple primary process to
access CPP interface?

If so this is the know limitation in DPDK, not two separate process can driver
same hardware, this is valid for all devices, why adding a lock unique to nfp?

Re: [dpdk-dev] [PATCH] net/nfp: fix lock file usage

[Alejandro Lucero]

On Thu, May 24, 2018 at 11:18 AM, Ferruh Yigit <ferruh.yigit@intel.com>
wrote:

> On 5/23/2018 5:50 PM, Alejandro Lucero wrote:
> >
> >
> > On Wed, May 23, 2018 at 4:57 PM, Ferruh Yigit <ferruh.yigit@intel.com
> > <mailto:ferruh.yigit@intel.com>> wrote:
> >
> >     On 5/23/2018 1:28 PM, Alejandro Lucero wrote:
> >     > DPDK apps can be executed as non-root users but current NFP lock
> >     > file for avoiding concurrent accesses to CPP interface is
> precluding
> >     > this option or requires to modify system file permissions.
> >     >
> >     > When the NFP device is bound to VFIO, this driver does not allow
> this
> >     > concurrent access, so the lock file is not required at all.
> >     >
> >     > OVS-DPDK as executed in RedHat distributions is the main NFP user
> >     > needing this fix.
> >     >
> >     > Fixes: c7e9729da6b5 ("net/nfp: support CPP")
> >     >
> >     > Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com
> <mailto:alejandro.lucero@netronome.com>>
> >
> >     Hi Alejandro,
> >
> >     As far as I understand this is to fix a common use case for nfp, but
> it looks
> >     like there is already a workaround and only for non-root users.
> >
> >
> > There is a patch submitted to stable versions because this lock was also
> with
> > the old NSPU interface, but as far as I know, there is no patch yet for
> the
> > current upstream tip.
> >
> >
> >
> >     What is the priority of the patch, only critical but fixes allowed
> at this
> >     point, can we push this one to next release?
> >
> >
> > This is critical for us because RedHat wants to support OVS with our
> card, and
> > when OVS-DPDK is used, this problem is precluding non-root users to
> execute
> > OVS-DPDK.
>
> What exactly this lock for? Does it to prevent multiple primary process to
> access CPP interface?
>
> If so this is the know limitation in DPDK, not two separate process can
> driver
> same hardware, this is valid for all devices, why adding a lock unique to
> nfp?
>

Time ago I had, by mistake, two different DPDK processes using same device,
and with UIO, there is no one avoiding this.

You can bound a device to UIO, igb_uio, and then use two different
processes opening the /dev/uiox file, and it works.

The VFIO driver does avoid this situation, but this lock is required for
UIO.

Re: [dpdk-dev] [PATCH] net/nfp: fix lock file usage

[Ferruh Yigit]

On 5/24/2018 3:02 PM, Alejandro Lucero wrote:
> 
> 
> On Thu, May 24, 2018 at 11:18 AM, Ferruh Yigit <ferruh.yigit@intel.com
> <mailto:ferruh.yigit@intel.com>> wrote:
> 
>     On 5/23/2018 5:50 PM, Alejandro Lucero wrote:
>     > 
>     > 
>     > On Wed, May 23, 2018 at 4:57 PM, Ferruh Yigit <ferruh.yigit@intel.com <mailto:ferruh.yigit@intel.com>
>     > <mailto:ferruh.yigit@intel.com <mailto:ferruh.yigit@intel.com>>> wrote:
>     > 
>     >     On 5/23/2018 1:28 PM, Alejandro Lucero wrote:
>     >     > DPDK apps can be executed as non-root users but current NFP lock
>     >     > file for avoiding concurrent accesses to CPP interface is precluding
>     >     > this option or requires to modify system file permissions.
>     >     > 
>     >     > When the NFP device is bound to VFIO, this driver does not allow this
>     >     > concurrent access, so the lock file is not required at all.
>     >     > 
>     >     > OVS-DPDK as executed in RedHat distributions is the main NFP user
>     >     > needing this fix.
>     >     > 
>     >     > Fixes: c7e9729da6b5 ("net/nfp: support CPP")
>     >     > 
>     >     > Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com
>     <mailto:alejandro.lucero@netronome.com>
>     <mailto:alejandro.lucero@netronome.com <mailto:alejandro.lucero@netronome.com>>>
>     > 
>     >     Hi Alejandro,
>     > 
>     >     As far as I understand this is to fix a common use case for nfp, but it looks
>     >     like there is already a workaround and only for non-root users.
>     > 
>     > 
>     > There is a patch submitted to stable versions because this lock was also with
>     > the old NSPU interface, but as far as I know, there is no patch yet for the
>     > current upstream tip.
>     > 
>     >  
>     > 
>     >     What is the priority of the patch, only critical but fixes allowed at this
>     >     point, can we push this one to next release?
>     > 
>     > 
>     > This is critical for us because RedHat wants to support OVS with our card, and
>     > when OVS-DPDK is used, this problem is precluding non-root users to execute
>     > OVS-DPDK.
> 
>     What exactly this lock for? Does it to prevent multiple primary process to
>     access CPP interface?
> 
>     If so this is the know limitation in DPDK, not two separate process can driver
>     same hardware, this is valid for all devices, why adding a lock unique to nfp?
> 
> 
> Time ago I had, by mistake, two different DPDK processes using same device, and
> with UIO, there is no one avoiding this.
> 
> You can bound a device to UIO, igb_uio, and then use two different processes
> opening the /dev/uiox file, and it works. 

But this is not anything specific to nfp, isn't it?

> 
> The VFIO driver does avoid this situation, but this lock is required for UIO.
> 

Re: [dpdk-dev] [PATCH] net/nfp: fix lock file usage

[Ferruh Yigit]

On 5/24/2018 3:13 PM, Ferruh Yigit wrote:
> On 5/24/2018 3:02 PM, Alejandro Lucero wrote:
>>
>>
>> On Thu, May 24, 2018 at 11:18 AM, Ferruh Yigit <ferruh.yigit@intel.com
>> <mailto:ferruh.yigit@intel.com>> wrote:
>>
>>     On 5/23/2018 5:50 PM, Alejandro Lucero wrote:
>>     > 
>>     > 
>>     > On Wed, May 23, 2018 at 4:57 PM, Ferruh Yigit <ferruh.yigit@intel.com <mailto:ferruh.yigit@intel.com>
>>     > <mailto:ferruh.yigit@intel.com <mailto:ferruh.yigit@intel.com>>> wrote:
>>     > 
>>     >     On 5/23/2018 1:28 PM, Alejandro Lucero wrote:
>>     >     > DPDK apps can be executed as non-root users but current NFP lock
>>     >     > file for avoiding concurrent accesses to CPP interface is precluding
>>     >     > this option or requires to modify system file permissions.
>>     >     > 
>>     >     > When the NFP device is bound to VFIO, this driver does not allow this
>>     >     > concurrent access, so the lock file is not required at all.
>>     >     > 
>>     >     > OVS-DPDK as executed in RedHat distributions is the main NFP user
>>     >     > needing this fix.
>>     >     > 
>>     >     > Fixes: c7e9729da6b5 ("net/nfp: support CPP")
>>     >     > 
>>     >     > Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com
>>     <mailto:alejandro.lucero@netronome.com>
>>     <mailto:alejandro.lucero@netronome.com <mailto:alejandro.lucero@netronome.com>>>
>>     > 
>>     >     Hi Alejandro,
>>     > 
>>     >     As far as I understand this is to fix a common use case for nfp, but it looks
>>     >     like there is already a workaround and only for non-root users.
>>     > 
>>     > 
>>     > There is a patch submitted to stable versions because this lock was also with
>>     > the old NSPU interface, but as far as I know, there is no patch yet for the
>>     > current upstream tip.
>>     > 
>>     >  
>>     > 
>>     >     What is the priority of the patch, only critical but fixes allowed at this
>>     >     point, can we push this one to next release?
>>     > 
>>     > 
>>     > This is critical for us because RedHat wants to support OVS with our card, and
>>     > when OVS-DPDK is used, this problem is precluding non-root users to execute
>>     > OVS-DPDK.
>>
>>     What exactly this lock for? Does it to prevent multiple primary process to
>>     access CPP interface?
>>
>>     If so this is the know limitation in DPDK, not two separate process can driver
>>     same hardware, this is valid for all devices, why adding a lock unique to nfp?
>>
>>
>> Time ago I had, by mistake, two different DPDK processes using same device, and
>> with UIO, there is no one avoiding this.
>>
>> You can bound a device to UIO, igb_uio, and then use two different processes
>> opening the /dev/uiox file, and it works. 
> 
> But this is not anything specific to nfp, isn't it?

Or let me ask something else, is this a fix for ovs-dpdk regular use-case with
nfp? Or this is just an extra protection in case multiple process may try to use
the NIC. If second, why it is critical?

> 
>>
>> The VFIO driver does avoid this situation, but this lock is required for UIO.
>>
> 

Re: [dpdk-dev] [PATCH] net/nfp: fix lock file usage

[Alejandro Lucero]

On Thu, May 24, 2018 at 3:15 PM, Ferruh Yigit <ferruh.yigit@intel.com>
wrote:

> On 5/24/2018 3:13 PM, Ferruh Yigit wrote:
> > On 5/24/2018 3:02 PM, Alejandro Lucero wrote:
> >>
> >>
> >> On Thu, May 24, 2018 at 11:18 AM, Ferruh Yigit <ferruh.yigit@intel.com
> >> <mailto:ferruh.yigit@intel.com>> wrote:
> >>
> >>     On 5/23/2018 5:50 PM, Alejandro Lucero wrote:
> >>     >
> >>     >
> >>     > On Wed, May 23, 2018 at 4:57 PM, Ferruh Yigit <
> ferruh.yigit@intel.com <mailto:ferruh.yigit@intel.com>
> >>     > <mailto:ferruh.yigit@intel.com <mailto:ferruh.yigit@intel.com>>>
> wrote:
> >>     >
> >>     >     On 5/23/2018 1:28 PM, Alejandro Lucero wrote:
> >>     >     > DPDK apps can be executed as non-root users but current NFP
> lock
> >>     >     > file for avoiding concurrent accesses to CPP interface is
> precluding
> >>     >     > this option or requires to modify system file permissions.
> >>     >     >
> >>     >     > When the NFP device is bound to VFIO, this driver does not
> allow this
> >>     >     > concurrent access, so the lock file is not required at all.
> >>     >     >
> >>     >     > OVS-DPDK as executed in RedHat distributions is the main
> NFP user
> >>     >     > needing this fix.
> >>     >     >
> >>     >     > Fixes: c7e9729da6b5 ("net/nfp: support CPP")
> >>     >     >
> >>     >     > Signed-off-by: Alejandro Lucero <
> alejandro.lucero@netronome.com
> >>     <mailto:alejandro.lucero@netronome.com>
> >>     <mailto:alejandro.lucero@netronome.com <mailto:alejandro.lucero@
> netronome.com>>>
> >>     >
> >>     >     Hi Alejandro,
> >>     >
> >>     >     As far as I understand this is to fix a common use case for
> nfp, but it looks
> >>     >     like there is already a workaround and only for non-root
> users.
> >>     >
> >>     >
> >>     > There is a patch submitted to stable versions because this lock
> was also with
> >>     > the old NSPU interface, but as far as I know, there is no patch
> yet for the
> >>     > current upstream tip.
> >>     >
> >>     >
> >>     >
> >>     >     What is the priority of the patch, only critical but fixes
> allowed at this
> >>     >     point, can we push this one to next release?
> >>     >
> >>     >
> >>     > This is critical for us because RedHat wants to support OVS with
> our card, and
> >>     > when OVS-DPDK is used, this problem is precluding non-root users
> to execute
> >>     > OVS-DPDK.
> >>
> >>     What exactly this lock for? Does it to prevent multiple primary
> process to
> >>     access CPP interface?
> >>
> >>     If so this is the know limitation in DPDK, not two separate process
> can driver
> >>     same hardware, this is valid for all devices, why adding a lock
> unique to nfp?
> >>
> >>
> >> Time ago I had, by mistake, two different DPDK processes using same
> device, and
> >> with UIO, there is no one avoiding this.
> >>
> >> You can bound a device to UIO, igb_uio, and then use two different
> processes
> >> opening the /dev/uiox file, and it works.
> >
> > But this is not anything specific to nfp, isn't it?
>
> Or let me ask something else, is this a fix for ovs-dpdk regular use-case
> with
> nfp? Or this is just an extra protection in case multiple process may try
> to use
> the NIC. If second, why it is critical?
>
>
I think any device bound to UIO could end up being used by two different
DPDK processes. So this is a protection against that possibility, because
accessing the NFP through the new CPP interface could make the NFP a brick
even it could crash the system.

RH is configuring OVS-DPDK for running as non-root, and the lock was
precluding this because it is set at /var/lock which a non-root user has
not access by default. This patch solves the problem, because when using
the device with VFIO, that lock is not necessary.  And with UIO, the lock
is needed and because it is not possible to run DPDK apps with UIO as
non-root, the lock path is fine.




> >
> >>
> >> The VFIO driver does avoid this situation, but this lock is required
> for UIO.
> >>
> >
>
>

Re: [dpdk-dev] [PATCH] net/nfp: fix lock file usage

[Ferruh Yigit]

On 5/24/2018 4:39 PM, Alejandro Lucero wrote:
> 
> 
> On Thu, May 24, 2018 at 3:15 PM, Ferruh Yigit <ferruh.yigit@intel.com
> <mailto:ferruh.yigit@intel.com>> wrote:
> 
>     On 5/24/2018 3:13 PM, Ferruh Yigit wrote:
>     > On 5/24/2018 3:02 PM, Alejandro Lucero wrote:
>     >>
>     >>
>     >> On Thu, May 24, 2018 at 11:18 AM, Ferruh Yigit <ferruh.yigit@intel.com
>     <mailto:ferruh.yigit@intel.com>
>     >> <mailto:ferruh.yigit@intel.com <mailto:ferruh.yigit@intel.com>>> wrote:
>     >>
>     >>     On 5/23/2018 5:50 PM, Alejandro Lucero wrote:
>     >>     >
>     >>     >
>     >>     > On Wed, May 23, 2018 at 4:57 PM, Ferruh Yigit
>     <ferruh.yigit@intel.com <mailto:ferruh.yigit@intel.com>
>     <mailto:ferruh.yigit@intel.com <mailto:ferruh.yigit@intel.com>>
>     >>     > <mailto:ferruh.yigit@intel.com <mailto:ferruh.yigit@intel.com>
>     <mailto:ferruh.yigit@intel.com <mailto:ferruh.yigit@intel.com>>>> wrote:
>     >>     >
>     >>     >     On 5/23/2018 1:28 PM, Alejandro Lucero wrote:
>     >>     >     > DPDK apps can be executed as non-root users but current NFP lock
>     >>     >     > file for avoiding concurrent accesses to CPP interface is
>     precluding
>     >>     >     > this option or requires to modify system file permissions.
>     >>     >     >
>     >>     >     > When the NFP device is bound to VFIO, this driver does not
>     allow this
>     >>     >     > concurrent access, so the lock file is not required at all.
>     >>     >     >
>     >>     >     > OVS-DPDK as executed in RedHat distributions is the main NFP user
>     >>     >     > needing this fix.
>     >>     >     >
>     >>     >     > Fixes: c7e9729da6b5 ("net/nfp: support CPP")
>     >>     >     >
>     >>     >     > Signed-off-by: Alejandro Lucero
>     <alejandro.lucero@netronome.com <mailto:alejandro.lucero@netronome.com>
>     >>     <mailto:alejandro.lucero@netronome.com
>     <mailto:alejandro.lucero@netronome.com>>
>     >>     <mailto:alejandro.lucero@netronome.com
>     <mailto:alejandro.lucero@netronome.com>
>     <mailto:alejandro.lucero@netronome.com
>     <mailto:alejandro.lucero@netronome.com>>>>
>     >>     >
>     >>     >     Hi Alejandro,
>     >>     >
>     >>     >     As far as I understand this is to fix a common use case for
>     nfp, but it looks
>     >>     >     like there is already a workaround and only for non-root users.
>     >>     >
>     >>     >
>     >>     > There is a patch submitted to stable versions because this lock was
>     also with
>     >>     > the old NSPU interface, but as far as I know, there is no patch yet
>     for the
>     >>     > current upstream tip.
>     >>     >
>     >>     >  
>     >>     >
>     >>     >     What is the priority of the patch, only critical but fixes
>     allowed at this
>     >>     >     point, can we push this one to next release?
>     >>     >
>     >>     >
>     >>     > This is critical for us because RedHat wants to support OVS with
>     our card, and
>     >>     > when OVS-DPDK is used, this problem is precluding non-root users to
>     execute
>     >>     > OVS-DPDK.
>     >>
>     >>     What exactly this lock for? Does it to prevent multiple primary
>     process to
>     >>     access CPP interface?
>     >>
>     >>     If so this is the know limitation in DPDK, not two separate process
>     can driver
>     >>     same hardware, this is valid for all devices, why adding a lock
>     unique to nfp?
>     >>
>     >>
>     >> Time ago I had, by mistake, two different DPDK processes using same
>     device, and
>     >> with UIO, there is no one avoiding this.
>     >>
>     >> You can bound a device to UIO, igb_uio, and then use two different processes
>     >> opening the /dev/uiox file, and it works.
>     >
>     > But this is not anything specific to nfp, isn't it?
> 
>     Or let me ask something else, is this a fix for ovs-dpdk regular use-case with
>     nfp? Or this is just an extra protection in case multiple process may try to use
>     the NIC. If second, why it is critical?
> 
> 
> I think any device bound to UIO could end up being used by two different DPDK
> processes. So this is a protection against that possibility, because accessing
> the NFP through the new CPP interface could make the NFP a brick even it could
> crash the system.

Yes and I was suggesting if we solve this, we should solve in higher level
instead of PMD, but that is already in PMD this patch is not introducing it.
 
> 
> RH is configuring OVS-DPDK for running as non-root, and the lock was precluding
> this because it is set at /var/lock which a non-root user has not access by
> default. This patch solves the problem, because when using the device with VFIO,
> that lock is not necessary.  And with UIO, the lock is needed and because it is
> not possible to run DPDK apps with UIO as non-root, the lock path is fine.

I see, this is to enable running as non-root by relaxing locking for vfio. OK,
let me check the patch.

But I still believe that locking shouldn't be in driver at fist place...

>     >
>     >>
>     >> The VFIO driver does avoid this situation, but this lock is required for UIO.
>     >>
>     >
> 
> 

Re: [dpdk-dev] [PATCH] net/nfp: fix lock file usage

[Alejandro Lucero]

On Thu, May 24, 2018 at 5:30 PM, Ferruh Yigit <ferruh.yigit@intel.com>
wrote:

> On 5/24/2018 4:39 PM, Alejandro Lucero wrote:
> >
> >
> > On Thu, May 24, 2018 at 3:15 PM, Ferruh Yigit <ferruh.yigit@intel.com
> > <mailto:ferruh.yigit@intel.com>> wrote:
> >
> >     On 5/24/2018 3:13 PM, Ferruh Yigit wrote:
> >     > On 5/24/2018 3:02 PM, Alejandro Lucero wrote:
> >     >>
> >     >>
> >     >> On Thu, May 24, 2018 at 11:18 AM, Ferruh Yigit <
> ferruh.yigit@intel.com
> >     <mailto:ferruh.yigit@intel.com>
> >     >> <mailto:ferruh.yigit@intel.com <mailto:ferruh.yigit@intel.com>>>
> wrote:
> >     >>
> >     >>     On 5/23/2018 5:50 PM, Alejandro Lucero wrote:
> >     >>     >
> >     >>     >
> >     >>     > On Wed, May 23, 2018 at 4:57 PM, Ferruh Yigit
> >     <ferruh.yigit@intel.com <mailto:ferruh.yigit@intel.com>
> >     <mailto:ferruh.yigit@intel.com <mailto:ferruh.yigit@intel.com>>
> >     >>     > <mailto:ferruh.yigit@intel.com <mailto:
> ferruh.yigit@intel.com>
> >     <mailto:ferruh.yigit@intel.com <mailto:ferruh.yigit@intel.com>>>>
> wrote:
> >     >>     >
> >     >>     >     On 5/23/2018 1:28 PM, Alejandro Lucero wrote:
> >     >>     >     > DPDK apps can be executed as non-root users but
> current NFP lock
> >     >>     >     > file for avoiding concurrent accesses to CPP
> interface is
> >     precluding
> >     >>     >     > this option or requires to modify system file
> permissions.
> >     >>     >     >
> >     >>     >     > When the NFP device is bound to VFIO, this driver
> does not
> >     allow this
> >     >>     >     > concurrent access, so the lock file is not required
> at all.
> >     >>     >     >
> >     >>     >     > OVS-DPDK as executed in RedHat distributions is the
> main NFP user
> >     >>     >     > needing this fix.
> >     >>     >     >
> >     >>     >     > Fixes: c7e9729da6b5 ("net/nfp: support CPP")
> >     >>     >     >
> >     >>     >     > Signed-off-by: Alejandro Lucero
> >     <alejandro.lucero@netronome.com <mailto:alejandro.lucero@
> netronome.com>
> >     >>     <mailto:alejandro.lucero@netronome.com
> >     <mailto:alejandro.lucero@netronome.com>>
> >     >>     <mailto:alejandro.lucero@netronome.com
> >     <mailto:alejandro.lucero@netronome.com>
> >     <mailto:alejandro.lucero@netronome.com
> >     <mailto:alejandro.lucero@netronome.com>>>>
> >     >>     >
> >     >>     >     Hi Alejandro,
> >     >>     >
> >     >>     >     As far as I understand this is to fix a common use case
> for
> >     nfp, but it looks
> >     >>     >     like there is already a workaround and only for
> non-root users.
> >     >>     >
> >     >>     >
> >     >>     > There is a patch submitted to stable versions because this
> lock was
> >     also with
> >     >>     > the old NSPU interface, but as far as I know, there is no
> patch yet
> >     for the
> >     >>     > current upstream tip.
> >     >>     >
> >     >>     >
> >     >>     >
> >     >>     >     What is the priority of the patch, only critical but
> fixes
> >     allowed at this
> >     >>     >     point, can we push this one to next release?
> >     >>     >
> >     >>     >
> >     >>     > This is critical for us because RedHat wants to support OVS
> with
> >     our card, and
> >     >>     > when OVS-DPDK is used, this problem is precluding non-root
> users to
> >     execute
> >     >>     > OVS-DPDK.
> >     >>
> >     >>     What exactly this lock for? Does it to prevent multiple
> primary
> >     process to
> >     >>     access CPP interface?
> >     >>
> >     >>     If so this is the know limitation in DPDK, not two separate
> process
> >     can driver
> >     >>     same hardware, this is valid for all devices, why adding a
> lock
> >     unique to nfp?
> >     >>
> >     >>
> >     >> Time ago I had, by mistake, two different DPDK processes using
> same
> >     device, and
> >     >> with UIO, there is no one avoiding this.
> >     >>
> >     >> You can bound a device to UIO, igb_uio, and then use two
> different processes
> >     >> opening the /dev/uiox file, and it works.
> >     >
> >     > But this is not anything specific to nfp, isn't it?
> >
> >     Or let me ask something else, is this a fix for ovs-dpdk regular
> use-case with
> >     nfp? Or this is just an extra protection in case multiple process
> may try to use
> >     the NIC. If second, why it is critical?
> >
> >
> > I think any device bound to UIO could end up being used by two different
> DPDK
> > processes. So this is a protection against that possibility, because
> accessing
> > the NFP through the new CPP interface could make the NFP a brick even it
> could
> > crash the system.
>
> Yes and I was suggesting if we solve this, we should solve in higher level
> instead of PMD, but that is already in PMD this patch is not introducing
> it.
>
> >
> > RH is configuring OVS-DPDK for running as non-root, and the lock was
> precluding
> > this because it is set at /var/lock which a non-root user has not access
> by
> > default. This patch solves the problem, because when using the device
> with VFIO,
> > that lock is not necessary.  And with UIO, the lock is needed and
> because it is
> > not possible to run DPDK apps with UIO as non-root, the lock path is
> fine.
>
> I see, this is to enable running as non-root by relaxing locking for vfio.
> OK,
> let me check the patch.
>
> But I still believe that locking shouldn't be in driver at fist place...
>
>
Agree. I think the UIO driver should be avoiding it, but not sure if there
are other reasons for not doing it.


> >     >
> >     >>
> >     >> The VFIO driver does avoid this situation, but this lock is
> required for UIO.
> >     >>
> >     >
> >
> >
>
>

Re: [dpdk-dev] [PATCH] net/nfp: fix lock file usage

[Ferruh Yigit]

On 5/23/2018 1:28 PM, Alejandro Lucero wrote:
> DPDK apps can be executed as non-root users but current NFP lock
> file for avoiding concurrent accesses to CPP interface is precluding
> this option or requires to modify system file permissions.
> 
> When the NFP device is bound to VFIO, this driver does not allow this
> concurrent access, so the lock file is not required at all.
> 
> OVS-DPDK as executed in RedHat distributions is the main NFP user
> needing this fix.
> 
> Fixes: c7e9729da6b5 ("net/nfp: support CPP")
> 
> Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>

Applied to dpdk-next-net/master, thanks.