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

[Alejandro Lucero <alejandro.lucero@netronome.com>]

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