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

[Ferruh Yigit <ferruh.yigit@intel.com>]

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.
>     >>
>     >
> 
>