DPDK patches and discussions
 help / color / mirror / Atom feed
* [PATCH] bus/pci: don't open uio device in secondary process
@ 2024-08-28 10:40 Konrad Sztyber
  2024-08-29  5:53 ` Chaoyong He
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Konrad Sztyber @ 2024-08-28 10:40 UTC (permalink / raw)
  To: dev
  Cc: Konrad Sztyber, Chenbo Xia, Nipun Gupta, Peng Zhang, Long Wu,
	Zerun Fu, Chaoyong He

The uio_pci_generic driver clears the bus master bit when the device
file is closed.  So, when the secondary process terminates after probing
a device, that device becomes unusable in the primary process.

To avoid that, the device file is now opened only in the primary
process.  The commit that introduced this regression, 847d78fb95
("bus/pci: fix FD in secondary process"), only mentioned enabling access
to config space from secondary process, which still works, as it doesn't
rely on the device file.

Fixes: 847d78fb95 ("bus/pci: fix FD in secondary process")

Signed-off-by: Konrad Sztyber <konrad.sztyber@intel.com>
---
 drivers/bus/pci/linux/pci_uio.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
index 4c1d3327a9..432316afcc 100644
--- a/drivers/bus/pci/linux/pci_uio.c
+++ b/drivers/bus/pci/linux/pci_uio.c
@@ -232,18 +232,6 @@ pci_uio_alloc_resource(struct rte_pci_device *dev,
 			loc->domain, loc->bus, loc->devid, loc->function);
 		return 1;
 	}
-	snprintf(devname, sizeof(devname), "/dev/uio%u", uio_num);
-
-	/* save fd */
-	fd = open(devname, O_RDWR);
-	if (fd < 0) {
-		PCI_LOG(ERR, "Cannot open %s: %s", devname, strerror(errno));
-		goto error;
-	}
-
-	if (rte_intr_fd_set(dev->intr_handle, fd))
-		goto error;
-
 	snprintf(cfgname, sizeof(cfgname),
 			"/sys/class/uio/uio%u/device/config", uio_num);
 
@@ -273,6 +261,19 @@ pci_uio_alloc_resource(struct rte_pci_device *dev,
 	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
 		return 0;
 
+	/* the uio_pci_generic driver clears the bus master enable bit when the device file is
+	 * closed, so open it only in the primary process */
+	snprintf(devname, sizeof(devname), "/dev/uio%u", uio_num);
+	/* save fd */
+	fd = open(devname, O_RDWR);
+	if (fd < 0) {
+		PCI_LOG(ERR, "Cannot open %s: %s", devname, strerror(errno));
+		goto error;
+	}
+
+	if (rte_intr_fd_set(dev->intr_handle, fd))
+		goto error;
+
 	/* allocate the mapping details for secondary processes*/
 	*uio_res = rte_zmalloc("UIO_RES", sizeof(**uio_res), 0);
 	if (*uio_res == NULL) {
-- 
2.45.0


^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH] bus/pci: don't open uio device in secondary process
  2024-08-28 10:40 [PATCH] bus/pci: don't open uio device in secondary process Konrad Sztyber
@ 2024-08-29  5:53 ` Chaoyong He
  2024-08-29  8:06 ` Chenbo Xia
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 11+ messages in thread
From: Chaoyong He @ 2024-08-29  5:53 UTC (permalink / raw)
  To: Konrad Sztyber, dev; +Cc: Chenbo Xia, Nipun Gupta, Long Wu, Zerun Fu

> 
> The uio_pci_generic driver clears the bus master bit when the device file is
> closed.  So, when the secondary process terminates after probing a device,
> that device becomes unusable in the primary process.
> 
> To avoid that, the device file is now opened only in the primary process.  The
> commit that introduced this regression, 847d78fb95
> ("bus/pci: fix FD in secondary process"), only mentioned enabling access to
> config space from secondary process, which still works, as it doesn't rely on
> the device file.

Yes, we can still access the config space from secondary process.

> 
> Fixes: 847d78fb95 ("bus/pci: fix FD in secondary process")

Maybe here also need a 'Cc: stable@dpdk.org' ?

With this, It looks good to me, thanks.
Acked-by: Chaoyong He <chaoyong.he@corigine.com>

> 
> Signed-off-by: Konrad Sztyber <konrad.sztyber@intel.com>
> ---
>  drivers/bus/pci/linux/pci_uio.c | 25 +++++++++++++------------
>  1 file changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
> index 4c1d3327a9..432316afcc 100644
> --- a/drivers/bus/pci/linux/pci_uio.c
> +++ b/drivers/bus/pci/linux/pci_uio.c
> @@ -232,18 +232,6 @@ pci_uio_alloc_resource(struct rte_pci_device *dev,
>                         loc->domain, loc->bus, loc->devid, loc->function);
>                 return 1;
>         }
> -       snprintf(devname, sizeof(devname), "/dev/uio%u", uio_num);
> -
> -       /* save fd */
> -       fd = open(devname, O_RDWR);
> -       if (fd < 0) {
> -               PCI_LOG(ERR, "Cannot open %s: %s", devname, strerror(errno));
> -               goto error;
> -       }
> -
> -       if (rte_intr_fd_set(dev->intr_handle, fd))
> -               goto error;
> -
>         snprintf(cfgname, sizeof(cfgname),
>                         "/sys/class/uio/uio%u/device/config", uio_num);
> 
> @@ -273,6 +261,19 @@ pci_uio_alloc_resource(struct rte_pci_device *dev,
>         if (rte_eal_process_type() != RTE_PROC_PRIMARY)
>                 return 0;
> 
> +       /* the uio_pci_generic driver clears the bus master enable bit when the
> device file is
> +        * closed, so open it only in the primary process */
> +       snprintf(devname, sizeof(devname), "/dev/uio%u", uio_num);
> +       /* save fd */
> +       fd = open(devname, O_RDWR);
> +       if (fd < 0) {
> +               PCI_LOG(ERR, "Cannot open %s: %s", devname, strerror(errno));
> +               goto error;
> +       }
> +
> +       if (rte_intr_fd_set(dev->intr_handle, fd))
> +               goto error;
> +
>         /* allocate the mapping details for secondary processes*/
>         *uio_res = rte_zmalloc("UIO_RES", sizeof(**uio_res), 0);
>         if (*uio_res == NULL) {
> --
> 2.45.0


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] bus/pci: don't open uio device in secondary process
  2024-08-28 10:40 [PATCH] bus/pci: don't open uio device in secondary process Konrad Sztyber
  2024-08-29  5:53 ` Chaoyong He
@ 2024-08-29  8:06 ` Chenbo Xia
  2024-08-29  8:57 ` [PATCH v2] " Konrad Sztyber
  2024-10-07 17:49 ` [PATCH] " Stephen Hemminger
  3 siblings, 0 replies; 11+ messages in thread
From: Chenbo Xia @ 2024-08-29  8:06 UTC (permalink / raw)
  To: Konrad Sztyber
  Cc: dev, Nipun Gupta, Peng Zhang, Long Wu, Zerun Fu, Chaoyong He



> On Aug 28, 2024, at 18:40, Konrad Sztyber <konrad.sztyber@intel.com> wrote:
> 
> External email: Use caution opening links or attachments
> 
> 
> The uio_pci_generic driver clears the bus master bit when the device
> file is closed.  So, when the secondary process terminates after probing

Should be one space before ‘So'

> a device, that device becomes unusable in the primary process.
> 
> To avoid that, the device file is now opened only in the primary
> process.  The commit that introduced this regression, 847d78fb95
> ("bus/pci: fix FD in secondary process"), only mentioned enabling access
> to config space from secondary process, which still works, as it doesn't
> rely on the device file.
> 
> Fixes: 847d78fb95 ("bus/pci: fix FD in secondary process")

Besides the cc stable tag mentioned by Chaoyong, commit ID should be 12-digit.

Please also fix the coding style:

WARNING:BLOCK_COMMENT_STYLE: Block comments use a trailing */ on a separate line
#176: FILE: drivers/bus/pci/linux/pci_uio.c:265:
+ * closed, so open it only in the primary process */

With above fixed:

Reviewed-by: Chenbo Xia <chenbox@nvidia.com>

> 
> Signed-off-by: Konrad Sztyber <konrad.sztyber@intel.com>
> ---
> drivers/bus/pci/linux/pci_uio.c | 25 +++++++++++++------------
> 1 file changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
> index 4c1d3327a9..432316afcc 100644
> --- a/drivers/bus/pci/linux/pci_uio.c
> +++ b/drivers/bus/pci/linux/pci_uio.c
> @@ -232,18 +232,6 @@ pci_uio_alloc_resource(struct rte_pci_device *dev,
>                        loc->domain, loc->bus, loc->devid, loc->function);
>                return 1;
>        }
> -       snprintf(devname, sizeof(devname), "/dev/uio%u", uio_num);
> -
> -       /* save fd */
> -       fd = open(devname, O_RDWR);
> -       if (fd < 0) {
> -               PCI_LOG(ERR, "Cannot open %s: %s", devname, strerror(errno));
> -               goto error;
> -       }
> -
> -       if (rte_intr_fd_set(dev->intr_handle, fd))
> -               goto error;
> -
>        snprintf(cfgname, sizeof(cfgname),
>                        "/sys/class/uio/uio%u/device/config", uio_num);
> 
> @@ -273,6 +261,19 @@ pci_uio_alloc_resource(struct rte_pci_device *dev,
>        if (rte_eal_process_type() != RTE_PROC_PRIMARY)
>                return 0;
> 
> +       /* the uio_pci_generic driver clears the bus master enable bit when the device file is
> +        * closed, so open it only in the primary process */
> +       snprintf(devname, sizeof(devname), "/dev/uio%u", uio_num);
> +       /* save fd */
> +       fd = open(devname, O_RDWR);
> +       if (fd < 0) {
> +               PCI_LOG(ERR, "Cannot open %s: %s", devname, strerror(errno));
> +               goto error;
> +       }
> +
> +       if (rte_intr_fd_set(dev->intr_handle, fd))
> +               goto error;
> +
>        /* allocate the mapping details for secondary processes*/
>        *uio_res = rte_zmalloc("UIO_RES", sizeof(**uio_res), 0);
>        if (*uio_res == NULL) {
> --
> 2.45.0
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2] bus/pci: don't open uio device in secondary process
  2024-08-28 10:40 [PATCH] bus/pci: don't open uio device in secondary process Konrad Sztyber
  2024-08-29  5:53 ` Chaoyong He
  2024-08-29  8:06 ` Chenbo Xia
@ 2024-08-29  8:57 ` Konrad Sztyber
  2024-08-30  3:48   ` Chenbo Xia
  2024-10-11 11:15   ` [PATCH v3] " Konrad Sztyber
  2024-10-07 17:49 ` [PATCH] " Stephen Hemminger
  3 siblings, 2 replies; 11+ messages in thread
From: Konrad Sztyber @ 2024-08-29  8:57 UTC (permalink / raw)
  To: dev
  Cc: chaoyong.he, Konrad Sztyber, stable, Chenbo Xia, Nipun Gupta,
	Zerun Fu, Anatoly Burakov, Peng Zhang, Long Wu

The uio_pci_generic driver clears the bus master bit when the device
file is closed. So, when the secondary process terminates after probing
a device, that device becomes unusable in the primary process.

To avoid that, the device file is now opened only in the primary
process. The commit that introduced this regression, 847d78fb9530
("bus/pci: fix FD in secondary process"), only mentioned enabling access
to config space from secondary process, which still works, as it doesn't
rely on the device file.

Fixes: 847d78fb9530 ("bus/pci: fix FD in secondary process")
Cc: stable@dpdk.org

Signed-off-by: Konrad Sztyber <konrad.sztyber@intel.com>
---
 drivers/bus/pci/linux/pci_uio.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
index 4c1d3327a9..5c4ba8098c 100644
--- a/drivers/bus/pci/linux/pci_uio.c
+++ b/drivers/bus/pci/linux/pci_uio.c
@@ -232,18 +232,6 @@ pci_uio_alloc_resource(struct rte_pci_device *dev,
 			loc->domain, loc->bus, loc->devid, loc->function);
 		return 1;
 	}
-	snprintf(devname, sizeof(devname), "/dev/uio%u", uio_num);
-
-	/* save fd */
-	fd = open(devname, O_RDWR);
-	if (fd < 0) {
-		PCI_LOG(ERR, "Cannot open %s: %s", devname, strerror(errno));
-		goto error;
-	}
-
-	if (rte_intr_fd_set(dev->intr_handle, fd))
-		goto error;
-
 	snprintf(cfgname, sizeof(cfgname),
 			"/sys/class/uio/uio%u/device/config", uio_num);
 
@@ -273,6 +261,21 @@ pci_uio_alloc_resource(struct rte_pci_device *dev,
 	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
 		return 0;
 
+	/*
+	 * the uio_pci_generic driver clears the bus master enable bit when the device file is
+	 * closed, so open it only in the primary process
+	 */
+	snprintf(devname, sizeof(devname), "/dev/uio%u", uio_num);
+	/* save fd */
+	fd = open(devname, O_RDWR);
+	if (fd < 0) {
+		PCI_LOG(ERR, "Cannot open %s: %s", devname, strerror(errno));
+		goto error;
+	}
+
+	if (rte_intr_fd_set(dev->intr_handle, fd))
+		goto error;
+
 	/* allocate the mapping details for secondary processes*/
 	*uio_res = rte_zmalloc("UIO_RES", sizeof(**uio_res), 0);
 	if (*uio_res == NULL) {
-- 
2.45.0


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] bus/pci: don't open uio device in secondary process
  2024-08-29  8:57 ` [PATCH v2] " Konrad Sztyber
@ 2024-08-30  3:48   ` Chenbo Xia
  2024-10-11 11:15   ` [PATCH v3] " Konrad Sztyber
  1 sibling, 0 replies; 11+ messages in thread
From: Chenbo Xia @ 2024-08-30  3:48 UTC (permalink / raw)
  To: Konrad Sztyber
  Cc: dev, Chaoyong He, stable, Nipun Gupta, Zerun Fu, Anatoly Burakov,
	Peng Zhang, Long Wu


> On Aug 29, 2024, at 16:57, Konrad Sztyber <konrad.sztyber@intel.com> wrote:
> 
> External email: Use caution opening links or attachments
> 
> 
> The uio_pci_generic driver clears the bus master bit when the device
> file is closed. So, when the secondary process terminates after probing
> a device, that device becomes unusable in the primary process.
> 
> To avoid that, the device file is now opened only in the primary
> process. The commit that introduced this regression, 847d78fb9530
> ("bus/pci: fix FD in secondary process"), only mentioned enabling access
> to config space from secondary process, which still works, as it doesn't
> rely on the device file.
> 
> Fixes: 847d78fb9530 ("bus/pci: fix FD in secondary process")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Konrad Sztyber <konrad.sztyber@intel.com>
> ---
> drivers/bus/pci/linux/pci_uio.c | 27 +++++++++++++++------------
> 1 file changed, 15 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
> index 4c1d3327a9..5c4ba8098c 100644
> --- a/drivers/bus/pci/linux/pci_uio.c
> +++ b/drivers/bus/pci/linux/pci_uio.c
> @@ -232,18 +232,6 @@ pci_uio_alloc_resource(struct rte_pci_device *dev,
>                       loc->domain, loc->bus, loc->devid, loc->function);
>               return 1;
>       }
> -       snprintf(devname, sizeof(devname), "/dev/uio%u", uio_num);
> -
> -       /* save fd */
> -       fd = open(devname, O_RDWR);
> -       if (fd < 0) {
> -               PCI_LOG(ERR, "Cannot open %s: %s", devname, strerror(errno));
> -               goto error;
> -       }
> -
> -       if (rte_intr_fd_set(dev->intr_handle, fd))
> -               goto error;
> -
>       snprintf(cfgname, sizeof(cfgname),
>                       "/sys/class/uio/uio%u/device/config", uio_num);
> 
> @@ -273,6 +261,21 @@ pci_uio_alloc_resource(struct rte_pci_device *dev,
>       if (rte_eal_process_type() != RTE_PROC_PRIMARY)
>               return 0;
> 
> +       /*
> +        * the uio_pci_generic driver clears the bus master enable bit when the device file is
> +        * closed, so open it only in the primary process
> +        */
> +       snprintf(devname, sizeof(devname), "/dev/uio%u", uio_num);
> +       /* save fd */
> +       fd = open(devname, O_RDWR);
> +       if (fd < 0) {
> +               PCI_LOG(ERR, "Cannot open %s: %s", devname, strerror(errno));
> +               goto error;
> +       }
> +
> +       if (rte_intr_fd_set(dev->intr_handle, fd))
> +               goto error;
> +
>       /* allocate the mapping details for secondary processes*/
>       *uio_res = rte_zmalloc("UIO_RES", sizeof(**uio_res), 0);
>       if (*uio_res == NULL) {
> --
> 2.45.0
> 

Reviewed-by: Chenbo Xia <chenbox@nvidia.com>


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] bus/pci: don't open uio device in secondary process
  2024-08-28 10:40 [PATCH] bus/pci: don't open uio device in secondary process Konrad Sztyber
                   ` (2 preceding siblings ...)
  2024-08-29  8:57 ` [PATCH v2] " Konrad Sztyber
@ 2024-10-07 17:49 ` Stephen Hemminger
  2024-10-09 10:11   ` Konrad Sztyber
  3 siblings, 1 reply; 11+ messages in thread
From: Stephen Hemminger @ 2024-10-07 17:49 UTC (permalink / raw)
  To: Konrad Sztyber
  Cc: dev, Chenbo Xia, Nipun Gupta, Peng Zhang, Long Wu, Zerun Fu, Chaoyong He

On Wed, 28 Aug 2024 12:40:02 +0200
Konrad Sztyber <konrad.sztyber@intel.com> wrote:

> The uio_pci_generic driver clears the bus master bit when the device
> file is closed.  So, when the secondary process terminates after probing
> a device, that device becomes unusable in the primary process.
> 
> To avoid that, the device file is now opened only in the primary
> process.  The commit that introduced this regression, 847d78fb95
> ("bus/pci: fix FD in secondary process"), only mentioned enabling access
> to config space from secondary process, which still works, as it doesn't
> rely on the device file.
> 
> Fixes: 847d78fb95 ("bus/pci: fix FD in secondary process")
> 
> Signed-off-by: Konrad Sztyber <konrad.sztyber@intel.com>

Wouldn't this break use of interrupts in the secondary process?

The patch does need the minor fix of the comment style.
So resubmit

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] bus/pci: don't open uio device in secondary process
  2024-10-07 17:49 ` [PATCH] " Stephen Hemminger
@ 2024-10-09 10:11   ` Konrad Sztyber
  2024-10-09 15:12     ` Stephen Hemminger
  0 siblings, 1 reply; 11+ messages in thread
From: Konrad Sztyber @ 2024-10-09 10:11 UTC (permalink / raw)
  To: Stephen Hemminger
  Cc: dev, Chenbo Xia, Nipun Gupta, Peng Zhang, Long Wu, Zerun Fu, Chaoyong He

On 10/7/24 19:49, Stephen Hemminger wrote:
> On Wed, 28 Aug 2024 12:40:02 +0200
> Konrad Sztyber <konrad.sztyber@intel.com> wrote:
> 
>> The uio_pci_generic driver clears the bus master bit when the device
>> file is closed.  So, when the secondary process terminates after probing
>> a device, that device becomes unusable in the primary process.
>>
>> To avoid that, the device file is now opened only in the primary
>> process.  The commit that introduced this regression, 847d78fb95
>> ("bus/pci: fix FD in secondary process"), only mentioned enabling access
>> to config space from secondary process, which still works, as it doesn't
>> rely on the device file.
>>
>> Fixes: 847d78fb95 ("bus/pci: fix FD in secondary process")
>>
>> Signed-off-by: Konrad Sztyber <konrad.sztyber@intel.com>
> 
> Wouldn't this break use of interrupts in the secondary process?

Yes, it will. But I don't think we can support interrupts in the 
secondary process *and*, at the same time, keep the device usable in the 
primary process when secondary terminates. Maybe we could pass the fd 
via SCM_RIGHTS? But I don't know if that results in the same struct file 
being used by both processes.

> The patch does need the minor fix of the comment style.
> So resubmit

I already did, see: 
https://inbox.dpdk.org/dev/20240829085724.270041-1-konrad.sztyber@intel.com

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] bus/pci: don't open uio device in secondary process
  2024-10-09 10:11   ` Konrad Sztyber
@ 2024-10-09 15:12     ` Stephen Hemminger
  2024-10-11  6:38       ` Konrad Sztyber
  0 siblings, 1 reply; 11+ messages in thread
From: Stephen Hemminger @ 2024-10-09 15:12 UTC (permalink / raw)
  To: Konrad Sztyber
  Cc: dev, Chenbo Xia, Nipun Gupta, Peng Zhang, Long Wu, Zerun Fu, Chaoyong He

On Wed, 9 Oct 2024 12:11:32 +0200
Konrad Sztyber <konrad.sztyber@intel.com> wrote:

> On 10/7/24 19:49, Stephen Hemminger wrote:
> > On Wed, 28 Aug 2024 12:40:02 +0200
> > Konrad Sztyber <konrad.sztyber@intel.com> wrote:
> >   
> >> The uio_pci_generic driver clears the bus master bit when the device
> >> file is closed.  So, when the secondary process terminates after probing
> >> a device, that device becomes unusable in the primary process.
> >>
> >> To avoid that, the device file is now opened only in the primary
> >> process.  The commit that introduced this regression, 847d78fb95
> >> ("bus/pci: fix FD in secondary process"), only mentioned enabling access
> >> to config space from secondary process, which still works, as it doesn't
> >> rely on the device file.
> >>
> >> Fixes: 847d78fb95 ("bus/pci: fix FD in secondary process")
> >>
> >> Signed-off-by: Konrad Sztyber <konrad.sztyber@intel.com>  
> > 
> > Wouldn't this break use of interrupts in the secondary process?  
> 
> Yes, it will. But I don't think we can support interrupts in the 
> secondary process *and*, at the same time, keep the device usable in the 
> primary process when secondary terminates. Maybe we could pass the fd 
> via SCM_RIGHTS? But I don't know if that results in the same struct file 
> being used by both processes.

That is what tap, and xdp are doing.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] bus/pci: don't open uio device in secondary process
  2024-10-09 15:12     ` Stephen Hemminger
@ 2024-10-11  6:38       ` Konrad Sztyber
  2024-10-11  6:46         ` David Marchand
  0 siblings, 1 reply; 11+ messages in thread
From: Konrad Sztyber @ 2024-10-11  6:38 UTC (permalink / raw)
  To: Stephen Hemminger
  Cc: dev, Chenbo Xia, Nipun Gupta, Peng Zhang, Long Wu, Zerun Fu, Chaoyong He

On 10/9/24 17:12, Stephen Hemminger wrote:
> That is what tap, and xdp are doing.

Thanks for the info. I'll take a look and will send a next rev doing 
something similar in uio.

Konrad


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] bus/pci: don't open uio device in secondary process
  2024-10-11  6:38       ` Konrad Sztyber
@ 2024-10-11  6:46         ` David Marchand
  0 siblings, 0 replies; 11+ messages in thread
From: David Marchand @ 2024-10-11  6:46 UTC (permalink / raw)
  To: Konrad Sztyber
  Cc: Stephen Hemminger, dev, Chenbo Xia, Nipun Gupta, Peng Zhang,
	Long Wu, Zerun Fu, Chaoyong He

On Fri, Oct 11, 2024 at 8:38 AM Konrad Sztyber <konrad.sztyber@intel.com> wrote:
>
> On 10/9/24 17:12, Stephen Hemminger wrote:
> > That is what tap, and xdp are doing.
>
> Thanks for the info. I'll take a look and will send a next rev doing
> something similar in uio.

My two cents.
net/tap uses the rte_mp_* infrastructure and this is what you need.
net/af_xdp has two cases, so don't get confused with the custom
SCM_RIGHTS stuff and stick to rte_mp_*.


-- 
David Marchand


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v3] bus/pci: don't open uio device in secondary process
  2024-08-29  8:57 ` [PATCH v2] " Konrad Sztyber
  2024-08-30  3:48   ` Chenbo Xia
@ 2024-10-11 11:15   ` Konrad Sztyber
  1 sibling, 0 replies; 11+ messages in thread
From: Konrad Sztyber @ 2024-10-11 11:15 UTC (permalink / raw)
  To: dev
  Cc: Chaoyong He, Stephen Hemminger, David Marchand, Konrad Sztyber,
	stable, Chenbo Xia, Nipun Gupta, Anatoly Burakov, Long Wu,
	Zerun Fu

The uio_pci_generic driver clears the bus master bit when the device
file is closed. So, when the secondary process terminates after probing
a device, that device becomes unusable in the primary process.

To avoid that, the device file is now opened only in the primary process
and the secondary gets it over UNIX domain socket via SCM_RIGHTS.

Fixes: 847d78fb9530 ("bus/pci: fix FD in secondary process")
Cc: stable@dpdk.org

Signed-off-by: Konrad Sztyber <konrad.sztyber@intel.com>
---
v3:
  Use the rte_mp_* infrastructure to pass the uio fd from the primary
  process to the secondary.
v2:
  Fixed coding style issues.
---
 drivers/bus/pci/linux/pci_uio.c | 140 ++++++++++++++++++++++++++++----
 1 file changed, 126 insertions(+), 14 deletions(-)

diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
index 4c1d3327a9..220390d921 100644
--- a/drivers/bus/pci/linux/pci_uio.c
+++ b/drivers/bus/pci/linux/pci_uio.c
@@ -21,14 +21,22 @@
 #include <rte_bus_pci.h>
 #include <rte_common.h>
 #include <rte_malloc.h>
+#include <rte_eal.h>
+#include <rte_errno.h>
 
 #include "eal_filesystem.h"
 #include "pci_init.h"
 #include "private.h"
 
 void *pci_map_addr = NULL;
+static int pci_uio_dev_count;
 
 #define OFF_MAX              ((uint64_t)(off_t)-1)
+#define SEND_FD_MP_KEY       "pci_uio_send_fd"
+
+struct pci_uio_send_fd_param {
+	struct rte_pci_addr addr;
+};
 
 int
 pci_uio_read_config(const struct rte_intr_handle *intr_handle,
@@ -211,6 +219,93 @@ pci_uio_free_resource(struct rte_pci_device *dev,
 		rte_intr_fd_set(dev->intr_handle, -1);
 		rte_intr_type_set(dev->intr_handle, RTE_INTR_HANDLE_UNKNOWN);
 	}
+
+	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
+		assert(pci_uio_dev_count > 0);
+		pci_uio_dev_count--;
+		if (pci_uio_dev_count == 0)
+			rte_mp_action_unregister(SEND_FD_MP_KEY);
+	}
+}
+
+static int
+pci_uio_send_fd(const struct rte_mp_msg *request, const void *peer)
+{
+	struct rte_pci_device *dev;
+	const struct pci_uio_send_fd_param *param =
+		(const struct pci_uio_send_fd_param *)request->param;
+	struct rte_mp_msg reply = {};
+	int fd;
+
+	strlcpy(reply.name, request->name, sizeof(reply.name));
+	TAILQ_FOREACH(dev, &rte_pci_bus.device_list, next) {
+		if (!rte_pci_addr_cmp(&dev->addr, &param->addr))
+			break;
+	}
+
+	if (dev == NULL) {
+		PCI_LOG(ERR, "Could not find PCI device (" PCI_PRI_FMT ")",
+			param->addr.domain, param->addr.bus,
+			param->addr.devid, param->addr.function);
+		goto reply;
+	}
+
+	fd = rte_intr_fd_get(dev->intr_handle);
+	if (fd < 0) {
+		PCI_LOG(ERR, "Could not get fd (" PCI_PRI_FMT ")",
+			param->addr.domain, param->addr.bus,
+			param->addr.devid, param->addr.function);
+		goto reply;
+	}
+
+	reply.num_fds = 1;
+	reply.fds[0] = fd;
+reply:
+	if (rte_mp_reply(&reply, peer) != 0) {
+		PCI_LOG(ERR, "Failed to send reply: %d (" PCI_PRI_FMT ")",
+			rte_errno, param->addr.domain, param->addr.bus,
+			param->addr.devid, param->addr.function);
+		return -1;
+	}
+
+	return 0;
+}
+
+static int
+pci_uio_request_fd(struct rte_pci_device *dev)
+{
+	struct rte_mp_msg request = {}, *reply;
+	struct timespec timeout = {.tv_sec = 1, .tv_nsec = 0};
+	struct pci_uio_send_fd_param *param =
+		(struct pci_uio_send_fd_param *)request.param;
+	struct rte_mp_reply replies;
+	int rc;
+
+	strlcpy(request.name, SEND_FD_MP_KEY, sizeof(request.name));
+	memcpy(&param->addr, &dev->addr, sizeof(param->addr));
+	request.len_param = sizeof(*param);
+
+	rc = rte_mp_request_sync(&request, &replies, &timeout);
+	if (rc != 0 || replies.nb_received != 1) {
+		PCI_LOG(ERR, "Failed to request fd from primary: %d (" PCI_PRI_FMT ")",
+			rte_errno, dev->addr.domain, dev->addr.bus,
+			dev->addr.devid, dev->addr.function);
+		return -1;
+	}
+
+	reply = replies.msgs;
+	if (reply->num_fds != 1) {
+		PCI_LOG(ERR, "Received unexpected number of fds: %d (" PCI_PRI_FMT ")",
+			reply->num_fds, dev->addr.domain, dev->addr.bus,
+			dev->addr.devid, dev->addr.function);
+		free(reply);
+		return -1;
+	}
+
+	rte_intr_fd_set(dev->intr_handle, reply->fds[0]);
+	free(reply);
+
+	return 0;
 }
 
 int
@@ -220,7 +315,7 @@ pci_uio_alloc_resource(struct rte_pci_device *dev,
 	char dirname[PATH_MAX];
 	char cfgname[PATH_MAX];
 	char devname[PATH_MAX]; /* contains the /dev/uioX */
-	int uio_num, fd, uio_cfg_fd;
+	int rc, uio_num, fd, uio_cfg_fd;
 	struct rte_pci_addr *loc;
 
 	loc = &dev->addr;
@@ -232,18 +327,6 @@ pci_uio_alloc_resource(struct rte_pci_device *dev,
 			loc->domain, loc->bus, loc->devid, loc->function);
 		return 1;
 	}
-	snprintf(devname, sizeof(devname), "/dev/uio%u", uio_num);
-
-	/* save fd */
-	fd = open(devname, O_RDWR);
-	if (fd < 0) {
-		PCI_LOG(ERR, "Cannot open %s: %s", devname, strerror(errno));
-		goto error;
-	}
-
-	if (rte_intr_fd_set(dev->intr_handle, fd))
-		goto error;
-
 	snprintf(cfgname, sizeof(cfgname),
 			"/sys/class/uio/uio%u/device/config", uio_num);
 
@@ -270,8 +353,27 @@ pci_uio_alloc_resource(struct rte_pci_device *dev,
 		}
 	}
 
-	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+	if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
+		if (pci_uio_request_fd(dev) != 0)
+			goto error;
 		return 0;
+	}
+
+	/*
+	 * The uio_pci_generic driver clears the bus master enable bit when the
+	 * device file is closed, so open it only in the primary process. The
+	 * secondary will get it via SCM_RIGHTS.
+	 */
+	snprintf(devname, sizeof(devname), "/dev/uio%u", uio_num);
+	/* save fd */
+	fd = open(devname, O_RDWR);
+	if (fd < 0) {
+		PCI_LOG(ERR, "Cannot open %s: %s", devname, strerror(errno));
+		goto error;
+	}
+
+	if (rte_intr_fd_set(dev->intr_handle, fd))
+		goto error;
 
 	/* allocate the mapping details for secondary processes*/
 	*uio_res = rte_zmalloc("UIO_RES", sizeof(**uio_res), 0);
@@ -280,6 +382,16 @@ pci_uio_alloc_resource(struct rte_pci_device *dev,
 		goto error;
 	}
 
+	if (pci_uio_dev_count == 0) {
+		rc = rte_mp_action_register(SEND_FD_MP_KEY, pci_uio_send_fd);
+		if (rc != 0 && rte_errno != ENOTSUP) {
+			PCI_LOG(ERR, "Failed to register multi-process callback: %d",
+				rte_errno);
+			goto error;
+		}
+	}
+
+	pci_uio_dev_count++;
 	strlcpy((*uio_res)->path, devname, sizeof((*uio_res)->path));
 	memcpy(&(*uio_res)->pci_addr, &dev->addr, sizeof((*uio_res)->pci_addr));
 
-- 
2.46.2


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2024-10-11 11:16 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-08-28 10:40 [PATCH] bus/pci: don't open uio device in secondary process Konrad Sztyber
2024-08-29  5:53 ` Chaoyong He
2024-08-29  8:06 ` Chenbo Xia
2024-08-29  8:57 ` [PATCH v2] " Konrad Sztyber
2024-08-30  3:48   ` Chenbo Xia
2024-10-11 11:15   ` [PATCH v3] " Konrad Sztyber
2024-10-07 17:49 ` [PATCH] " Stephen Hemminger
2024-10-09 10:11   ` Konrad Sztyber
2024-10-09 15:12     ` Stephen Hemminger
2024-10-11  6:38       ` Konrad Sztyber
2024-10-11  6:46         ` David Marchand

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).