DPDK patches and discussions
 help / color / mirror / Atom feed
* [PATCH 1/2] eal/linux: fix VFIO hotplug with multiprocess
@ 2024-09-16 12:30 David Marchand
  2024-09-16 12:30 ` [PATCH 2/2] bus/pci: enhance hotplug for VFIO bound devices David Marchand
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: David Marchand @ 2024-09-16 12:30 UTC (permalink / raw)
  To: dev; +Cc: Anatoly Burakov

At the moment, if VFIO is not available at DPDK init, it won't be
available unless a subsequent rte_vfio_enable() is done.

Yet, even if rte_vfio_enable() is called again in primary and secondary
processes, a secondary process will never get to know that VFIO has been
enabled in the primary process as the MP requests handler is only
registered in EAL init.

On the other hand, moving the MP requests handler registration earlier
in EAL init is ok, as secondary process are supposed to be waiting on
eal_mcfg_wait_complete() until the primary process calls
eal_mcfg_complete().

Move vfio_mp_sync_setup() in rte_vfio_enable().

Besides, rte_eal_vfio_setup() is useless and its name with a rte_ prefix
is ambiguous as it gives the impression it is an exported/public symbol.
Remove it and directly call rte_vfio_enable() where needed.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 lib/eal/linux/eal.c      | 18 +-----------------
 lib/eal/linux/eal_vfio.c |  9 ++++++---
 2 files changed, 7 insertions(+), 20 deletions(-)

diff --git a/lib/eal/linux/eal.c b/lib/eal/linux/eal.c
index d742cc98e2..54577b7718 100644
--- a/lib/eal/linux/eal.c
+++ b/lib/eal/linux/eal.c
@@ -867,16 +867,6 @@ rte_eal_iopl_init(void)
 	return 0;
 }
 
-#ifdef VFIO_PRESENT
-static int rte_eal_vfio_setup(void)
-{
-	if (rte_vfio_enable("vfio"))
-		return -1;
-
-	return 0;
-}
-#endif
-
 static void rte_eal_init_alert(const char *msg)
 {
 	fprintf(stderr, "EAL: FATAL: %s\n", msg);
@@ -1162,7 +1152,7 @@ rte_eal_init(int argc, char **argv)
 	}
 
 #ifdef VFIO_PRESENT
-	if (rte_eal_vfio_setup() < 0) {
+	if (rte_vfio_enable("vfio")) {
 		rte_eal_init_alert("Cannot init VFIO");
 		rte_errno = EAGAIN;
 		rte_atomic_store_explicit(&run_once, 0, rte_memory_order_relaxed);
@@ -1291,12 +1281,6 @@ rte_eal_init(int argc, char **argv)
 		return -1;
 	}
 
-#ifdef VFIO_PRESENT
-	/* Register mp action after probe() so that we got enough info */
-	if (rte_vfio_is_enabled("vfio") && vfio_mp_sync_setup() < 0)
-		return -1;
-#endif
-
 	/* initialize default service/lcore mappings and start running. Ignore
 	 * -ENOTSUP, as it indicates no service coremask passed to EAL.
 	 */
diff --git a/lib/eal/linux/eal_vfio.c b/lib/eal/linux/eal_vfio.c
index 4e69e72e3b..7132e24cba 100644
--- a/lib/eal/linux/eal_vfio.c
+++ b/lib/eal/linux/eal_vfio.c
@@ -1120,9 +1120,12 @@ rte_vfio_enable(const char *modname)
 	}
 
 	if (internal_conf->process_type == RTE_PROC_PRIMARY) {
-		/* open a new container */
-		default_vfio_cfg->vfio_container_fd =
-				rte_vfio_get_container_fd();
+		if (vfio_mp_sync_setup() == -1) {
+			default_vfio_cfg->vfio_container_fd = -1;
+		} else {
+			/* open a new container */
+			default_vfio_cfg->vfio_container_fd = rte_vfio_get_container_fd();
+		}
 	} else {
 		/* get the default container from the primary process */
 		default_vfio_cfg->vfio_container_fd =
-- 
2.46.0


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

* [PATCH 2/2] bus/pci: enhance hotplug for VFIO bound devices
  2024-09-16 12:30 [PATCH 1/2] eal/linux: fix VFIO hotplug with multiprocess David Marchand
@ 2024-09-16 12:30 ` David Marchand
  2024-09-17  8:57   ` Maxime Coquelin
  2024-09-17  8:20 ` [PATCH 1/2] eal/linux: fix VFIO hotplug with multiprocess Maxime Coquelin
  2024-10-04 14:34 ` David Marchand
  2 siblings, 1 reply; 8+ messages in thread
From: David Marchand @ 2024-09-16 12:30 UTC (permalink / raw)
  To: dev; +Cc: Anatoly Burakov, Chenbo Xia, Nipun Gupta

VFIO modules may get loaded/initialized after DPDK initialized (like when
starting an application without knowing which devices will be used, and
whether their drivers require VFIO).

Retry enabling VFIO if not available.
This way, it is not required to restart the DPDK application anymore.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 drivers/bus/pci/linux/pci_vfio.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
index adc34a5c51..5317170231 100644
--- a/drivers/bus/pci/linux/pci_vfio.c
+++ b/drivers/bus/pci/linux/pci_vfio.c
@@ -1319,6 +1319,12 @@ pci_vfio_mmio_write(const struct rte_pci_device *dev, int bar,
 int
 pci_vfio_is_enabled(void)
 {
-	return rte_vfio_is_enabled("vfio_pci");
+	int status = rte_vfio_is_enabled("vfio_pci");
+
+	if (!status) {
+		rte_vfio_enable("vfio");
+		status = rte_vfio_is_enabled("vfio_pci");
+	}
+	return status;
 }
 #endif
-- 
2.46.0


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

* Re: [PATCH 1/2] eal/linux: fix VFIO hotplug with multiprocess
  2024-09-16 12:30 [PATCH 1/2] eal/linux: fix VFIO hotplug with multiprocess David Marchand
  2024-09-16 12:30 ` [PATCH 2/2] bus/pci: enhance hotplug for VFIO bound devices David Marchand
@ 2024-09-17  8:20 ` Maxime Coquelin
  2024-10-02 18:31   ` David Marchand
  2024-10-14 18:58   ` David Marchand
  2024-10-04 14:34 ` David Marchand
  2 siblings, 2 replies; 8+ messages in thread
From: Maxime Coquelin @ 2024-09-17  8:20 UTC (permalink / raw)
  To: David Marchand, dev; +Cc: Anatoly Burakov



On 9/16/24 14:30, David Marchand wrote:
> At the moment, if VFIO is not available at DPDK init, it won't be
> available unless a subsequent rte_vfio_enable() is done.
> 
> Yet, even if rte_vfio_enable() is called again in primary and secondary
> processes, a secondary process will never get to know that VFIO has been
> enabled in the primary process as the MP requests handler is only
> registered in EAL init.
> 
> On the other hand, moving the MP requests handler registration earlier
> in EAL init is ok, as secondary process are supposed to be waiting on
> eal_mcfg_wait_complete() until the primary process calls
> eal_mcfg_complete().
> 
> Move vfio_mp_sync_setup() in rte_vfio_enable().
> 
> Besides, rte_eal_vfio_setup() is useless and its name with a rte_ prefix
> is ambiguous as it gives the impression it is an exported/public symbol.
> Remove it and directly call rte_vfio_enable() where needed.
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>   lib/eal/linux/eal.c      | 18 +-----------------
>   lib/eal/linux/eal_vfio.c |  9 ++++++---
>   2 files changed, 7 insertions(+), 20 deletions(-)
> 

Should it be considered as a fix, and so a candidate for stable?
Or do you think it is too risky to change the behaviour?

Other than that, it looks good to me:
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime


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

* Re: [PATCH 2/2] bus/pci: enhance hotplug for VFIO bound devices
  2024-09-16 12:30 ` [PATCH 2/2] bus/pci: enhance hotplug for VFIO bound devices David Marchand
@ 2024-09-17  8:57   ` Maxime Coquelin
  0 siblings, 0 replies; 8+ messages in thread
From: Maxime Coquelin @ 2024-09-17  8:57 UTC (permalink / raw)
  To: David Marchand, dev; +Cc: Anatoly Burakov, Chenbo Xia, Nipun Gupta



On 9/16/24 14:30, David Marchand wrote:
> VFIO modules may get loaded/initialized after DPDK initialized (like when
> starting an application without knowing which devices will be used, and
> whether their drivers require VFIO).
> 
> Retry enabling VFIO if not available.
> This way, it is not required to restart the DPDK application anymore.
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>   drivers/bus/pci/linux/pci_vfio.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
> index adc34a5c51..5317170231 100644
> --- a/drivers/bus/pci/linux/pci_vfio.c
> +++ b/drivers/bus/pci/linux/pci_vfio.c
> @@ -1319,6 +1319,12 @@ pci_vfio_mmio_write(const struct rte_pci_device *dev, int bar,
>   int
>   pci_vfio_is_enabled(void)
>   {
> -	return rte_vfio_is_enabled("vfio_pci");
> +	int status = rte_vfio_is_enabled("vfio_pci");
> +
> +	if (!status) {
> +		rte_vfio_enable("vfio");
> +		status = rte_vfio_is_enabled("vfio_pci");
> +	}
> +	return status;
>   }
>   #endif

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Also wondering if it could be considered a fix.

Thanks,
Maxime


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

* Re: [PATCH 1/2] eal/linux: fix VFIO hotplug with multiprocess
  2024-09-17  8:20 ` [PATCH 1/2] eal/linux: fix VFIO hotplug with multiprocess Maxime Coquelin
@ 2024-10-02 18:31   ` David Marchand
  2024-10-03  8:25     ` Kevin Traynor
  2024-10-14 18:58   ` David Marchand
  1 sibling, 1 reply; 8+ messages in thread
From: David Marchand @ 2024-10-02 18:31 UTC (permalink / raw)
  To: Maxime Coquelin, Anatoly Burakov
  Cc: dev, Kevin Traynor, Luca Boccassi, Xueming(Steven) Li

On Tue, Sep 17, 2024 at 10:20 AM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
> On 9/16/24 14:30, David Marchand wrote:
> > At the moment, if VFIO is not available at DPDK init, it won't be
> > available unless a subsequent rte_vfio_enable() is done.
> >
> > Yet, even if rte_vfio_enable() is called again in primary and secondary
> > processes, a secondary process will never get to know that VFIO has been
> > enabled in the primary process as the MP requests handler is only
> > registered in EAL init.
> >
> > On the other hand, moving the MP requests handler registration earlier
> > in EAL init is ok, as secondary process are supposed to be waiting on
> > eal_mcfg_wait_complete() until the primary process calls
> > eal_mcfg_complete().
> >
> > Move vfio_mp_sync_setup() in rte_vfio_enable().
> >
> > Besides, rte_eal_vfio_setup() is useless and its name with a rte_ prefix
> > is ambiguous as it gives the impression it is an exported/public symbol.
> > Remove it and directly call rte_vfio_enable() where needed.
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > ---
> >   lib/eal/linux/eal.c      | 18 +-----------------
> >   lib/eal/linux/eal_vfio.c |  9 ++++++---
> >   2 files changed, 7 insertions(+), 20 deletions(-)
> >
>
> Should it be considered as a fix, and so a candidate for stable?
> Or do you think it is too risky to change the behaviour?

Cc: stable maintainers (fyi)

I don't think it is risky: existing applications which relied on multi
process had no choice but to make sure VFIO was properly setup before
DPDK init.
Yet, a change in behavior impact is always hard to estimate.

(same comment for the second patch of the series)


-- 
David Marchand


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

* Re: [PATCH 1/2] eal/linux: fix VFIO hotplug with multiprocess
  2024-10-02 18:31   ` David Marchand
@ 2024-10-03  8:25     ` Kevin Traynor
  0 siblings, 0 replies; 8+ messages in thread
From: Kevin Traynor @ 2024-10-03  8:25 UTC (permalink / raw)
  To: David Marchand, Maxime Coquelin, Anatoly Burakov
  Cc: dev, Luca Boccassi, Xueming(Steven) Li

On 02/10/2024 14:31, David Marchand wrote:
> On Tue, Sep 17, 2024 at 10:20 AM Maxime Coquelin
> <maxime.coquelin@redhat.com> wrote:
>> On 9/16/24 14:30, David Marchand wrote:
>>> At the moment, if VFIO is not available at DPDK init, it won't be
>>> available unless a subsequent rte_vfio_enable() is done.
>>>
>>> Yet, even if rte_vfio_enable() is called again in primary and secondary
>>> processes, a secondary process will never get to know that VFIO has been
>>> enabled in the primary process as the MP requests handler is only
>>> registered in EAL init.
>>>
>>> On the other hand, moving the MP requests handler registration earlier
>>> in EAL init is ok, as secondary process are supposed to be waiting on
>>> eal_mcfg_wait_complete() until the primary process calls
>>> eal_mcfg_complete().
>>>
>>> Move vfio_mp_sync_setup() in rte_vfio_enable().
>>>
>>> Besides, rte_eal_vfio_setup() is useless and its name with a rte_ prefix
>>> is ambiguous as it gives the impression it is an exported/public symbol.
>>> Remove it and directly call rte_vfio_enable() where needed.
>>>
>>> Signed-off-by: David Marchand <david.marchand@redhat.com>
>>> ---
>>>   lib/eal/linux/eal.c      | 18 +-----------------
>>>   lib/eal/linux/eal_vfio.c |  9 ++++++---
>>>   2 files changed, 7 insertions(+), 20 deletions(-)
>>>
>>
>> Should it be considered as a fix, and so a candidate for stable?
>> Or do you think it is too risky to change the behaviour?
> 
> Cc: stable maintainers (fyi)
> 
> I don't think it is risky: existing applications which relied on multi
> process had no choice but to make sure VFIO was properly setup before
> DPDK init.
> Yet, a change in behavior impact is always hard to estimate.
> 
> (same comment for the second patch of the series)
> 
> 

They seem borderline fixes for usability. I think it's a good candidate
for 22.11/23.11.

For 21.11, it is the last release and the behaviour has been like that
for 3 years, so would probably skip that one.


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

* Re: [PATCH 1/2] eal/linux: fix VFIO hotplug with multiprocess
  2024-09-16 12:30 [PATCH 1/2] eal/linux: fix VFIO hotplug with multiprocess David Marchand
  2024-09-16 12:30 ` [PATCH 2/2] bus/pci: enhance hotplug for VFIO bound devices David Marchand
  2024-09-17  8:20 ` [PATCH 1/2] eal/linux: fix VFIO hotplug with multiprocess Maxime Coquelin
@ 2024-10-04 14:34 ` David Marchand
  2 siblings, 0 replies; 8+ messages in thread
From: David Marchand @ 2024-10-04 14:34 UTC (permalink / raw)
  To: Anatoly Burakov; +Cc: dev, Mcnamara, John, Thomas Monjalon

Hello Anatoly,

On Mon, Sep 16, 2024 at 2:31 PM David Marchand
<david.marchand@redhat.com> wrote:
>
> At the moment, if VFIO is not available at DPDK init, it won't be
> available unless a subsequent rte_vfio_enable() is done.
>
> Yet, even if rte_vfio_enable() is called again in primary and secondary
> processes, a secondary process will never get to know that VFIO has been
> enabled in the primary process as the MP requests handler is only
> registered in EAL init.
>
> On the other hand, moving the MP requests handler registration earlier
> in EAL init is ok, as secondary process are supposed to be waiting on
> eal_mcfg_wait_complete() until the primary process calls
> eal_mcfg_complete().
>
> Move vfio_mp_sync_setup() in rte_vfio_enable().
>
> Besides, rte_eal_vfio_setup() is useless and its name with a rte_ prefix
> is ambiguous as it gives the impression it is an exported/public symbol.
> Remove it and directly call rte_vfio_enable() where needed.
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>

Can I get a review on this 2 patches series?
Thanks.


-- 
David Marchand


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

* Re: [PATCH 1/2] eal/linux: fix VFIO hotplug with multiprocess
  2024-09-17  8:20 ` [PATCH 1/2] eal/linux: fix VFIO hotplug with multiprocess Maxime Coquelin
  2024-10-02 18:31   ` David Marchand
@ 2024-10-14 18:58   ` David Marchand
  1 sibling, 0 replies; 8+ messages in thread
From: David Marchand @ 2024-10-14 18:58 UTC (permalink / raw)
  To: David Marchand
  Cc: dev, Anatoly Burakov, Kevin Traynor, Mcnamara, John,
	Thomas Monjalon, Maxime Coquelin

On Tue, Sep 17, 2024 at 10:20 AM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
> > At the moment, if VFIO is not available at DPDK init, it won't be
> > available unless a subsequent rte_vfio_enable() is done.
> >
> > Yet, even if rte_vfio_enable() is called again in primary and secondary
> > processes, a secondary process will never get to know that VFIO has been
> > enabled in the primary process as the MP requests handler is only
> > registered in EAL init.
> >
> > On the other hand, moving the MP requests handler registration earlier
> > in EAL init is ok, as secondary process are supposed to be waiting on
> > eal_mcfg_wait_complete() until the primary process calls
> > eal_mcfg_complete().
> >
> > Move vfio_mp_sync_setup() in rte_vfio_enable().
> >
> > Besides, rte_eal_vfio_setup() is useless and its name with a rte_ prefix
> > is ambiguous as it gives the impression it is an exported/public symbol.
> > Remove it and directly call rte_vfio_enable() where needed.
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Series applied for rc1.
Thanks for the review Maxime.


-- 
David Marchand


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

end of thread, other threads:[~2024-10-14 18:58 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-09-16 12:30 [PATCH 1/2] eal/linux: fix VFIO hotplug with multiprocess David Marchand
2024-09-16 12:30 ` [PATCH 2/2] bus/pci: enhance hotplug for VFIO bound devices David Marchand
2024-09-17  8:57   ` Maxime Coquelin
2024-09-17  8:20 ` [PATCH 1/2] eal/linux: fix VFIO hotplug with multiprocess Maxime Coquelin
2024-10-02 18:31   ` David Marchand
2024-10-03  8:25     ` Kevin Traynor
2024-10-14 18:58   ` David Marchand
2024-10-04 14:34 ` 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).