DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] net/pcap: fix registration of timestamp dynamic field
@ 2020-11-13 10:39 Olivier Matz
  2020-11-13 13:13 ` Olivier Matz
  0 siblings, 1 reply; 5+ messages in thread
From: Olivier Matz @ 2020-11-13 10:39 UTC (permalink / raw)
  To: dev; +Cc: Ferruh Yigit, David Marchand, Thomas Monjalon

In pcap pmd, the timestamp mbuf dynamic field is mandatory. When the
pcap pmd is created in a secondary process (this is the case for pdump),
it cannot be registered because this is not allowed from a secondary
process.

To ensure that the field is properly registered, do it from probe()
instead of configure(). Indeed, probe() is invoked on the primary
process when a device is created in a secondary.

Bugzilla ID: 571
Fixes: d23d73d088c1 ("net/pcap: switch Rx timestamp to dynamic mbuf field")

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 drivers/net/pcap/rte_eth_pcap.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
index 4e6d49370e..4930d7d382 100644
--- a/drivers/net/pcap/rte_eth_pcap.c
+++ b/drivers/net/pcap/rte_eth_pcap.c
@@ -661,15 +661,6 @@ eth_dev_stop(struct rte_eth_dev *dev)
 static int
 eth_dev_configure(struct rte_eth_dev *dev __rte_unused)
 {
-	int ret;
-
-	ret = rte_mbuf_dyn_rx_timestamp_register(&timestamp_dynfield_offset,
-			&timestamp_rx_dynflag);
-	if (ret != 0) {
-		PMD_LOG(ERR, "Failed to register Rx timestamp field/flag");
-		return -rte_errno;
-	}
-
 	return 0;
 }
 
@@ -1387,6 +1378,13 @@ pmd_pcap_probe(struct rte_vdev_device *dev)
 	start_cycles = rte_get_timer_cycles();
 	hz = rte_get_timer_hz();
 
+	ret = rte_mbuf_dyn_rx_timestamp_register(&timestamp_dynfield_offset,
+			&timestamp_rx_dynflag);
+	if (ret != 0) {
+		PMD_LOG(ERR, "Failed to register Rx timestamp field/flag");
+		return -1;
+	}
+
 	if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
 		eth_dev = rte_eth_dev_attach_secondary(name);
 		if (!eth_dev) {
-- 
2.25.1


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

* Re: [dpdk-dev] [PATCH] net/pcap: fix registration of timestamp dynamic field
  2020-11-13 10:39 [dpdk-dev] [PATCH] net/pcap: fix registration of timestamp dynamic field Olivier Matz
@ 2020-11-13 13:13 ` Olivier Matz
  2020-11-13 14:02   ` Ferruh Yigit
  0 siblings, 1 reply; 5+ messages in thread
From: Olivier Matz @ 2020-11-13 13:13 UTC (permalink / raw)
  To: dev; +Cc: Ferruh Yigit, David Marchand, Thomas Monjalon

On Fri, Nov 13, 2020 at 11:39:57AM +0100, Olivier Matz wrote:
> In pcap pmd, the timestamp mbuf dynamic field is mandatory. When the
> pcap pmd is created in a secondary process (this is the case for pdump),
> it cannot be registered because this is not allowed from a secondary
> process.
> 
> To ensure that the field is properly registered, do it from probe()
> instead of configure(). Indeed, probe() is invoked on the primary
> process when a device is created in a secondary.
> 
> Bugzilla ID: 571
> Fixes: d23d73d088c1 ("net/pcap: switch Rx timestamp to dynamic mbuf field")
> 
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> ---

One additional comment about this patch.

While it solves the issue described in the bug report, there may still
be a gap when it is needed to register a dynamic field/flag from a
secondary process. This happens when registering and configuring devices
from a secondary process (is it supported?). It happens if the secondary
process initializes a library which is not initialized in primary, and
which requires a dynamic field.

From afar, it does not look too difficult to implement dynamic field
registration from secondary processes. The only thing missing is a way
to allocate the shared memory in the primary process at initialization.
Currently, there is no init callback that is invoked when eal init is
done.

This is the exact same problem than for this issue:
http://inbox.dpdk.org/dev/20200504074227.GA6327@platinum/#t


>  drivers/net/pcap/rte_eth_pcap.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
> index 4e6d49370e..4930d7d382 100644
> --- a/drivers/net/pcap/rte_eth_pcap.c
> +++ b/drivers/net/pcap/rte_eth_pcap.c
> @@ -661,15 +661,6 @@ eth_dev_stop(struct rte_eth_dev *dev)
>  static int
>  eth_dev_configure(struct rte_eth_dev *dev __rte_unused)
>  {
> -	int ret;
> -
> -	ret = rte_mbuf_dyn_rx_timestamp_register(&timestamp_dynfield_offset,
> -			&timestamp_rx_dynflag);
> -	if (ret != 0) {
> -		PMD_LOG(ERR, "Failed to register Rx timestamp field/flag");
> -		return -rte_errno;
> -	}
> -
>  	return 0;
>  }
>  
> @@ -1387,6 +1378,13 @@ pmd_pcap_probe(struct rte_vdev_device *dev)
>  	start_cycles = rte_get_timer_cycles();
>  	hz = rte_get_timer_hz();
>  
> +	ret = rte_mbuf_dyn_rx_timestamp_register(&timestamp_dynfield_offset,
> +			&timestamp_rx_dynflag);
> +	if (ret != 0) {
> +		PMD_LOG(ERR, "Failed to register Rx timestamp field/flag");
> +		return -1;
> +	}
> +
>  	if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
>  		eth_dev = rte_eth_dev_attach_secondary(name);
>  		if (!eth_dev) {
> -- 
> 2.25.1
> 

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

* Re: [dpdk-dev] [PATCH] net/pcap: fix registration of timestamp dynamic field
  2020-11-13 13:13 ` Olivier Matz
@ 2020-11-13 14:02   ` Ferruh Yigit
  2020-11-13 17:23     ` Ferruh Yigit
  2020-11-13 20:35     ` Olivier Matz
  0 siblings, 2 replies; 5+ messages in thread
From: Ferruh Yigit @ 2020-11-13 14:02 UTC (permalink / raw)
  To: Olivier Matz, dev; +Cc: David Marchand, Thomas Monjalon

On 11/13/2020 1:13 PM, Olivier Matz wrote:
> On Fri, Nov 13, 2020 at 11:39:57AM +0100, Olivier Matz wrote:
>> In pcap pmd, the timestamp mbuf dynamic field is mandatory. When the
>> pcap pmd is created in a secondary process (this is the case for pdump),
>> it cannot be registered because this is not allowed from a secondary
>> process.
>>
>> To ensure that the field is properly registered, do it from probe()
>> instead of configure(). Indeed, probe() is invoked on the primary
>> process when a device is created in a secondary.
>>

probe() invoked first in the primary, later in the secondary, both process calls 
the driver probe(). But for this case probe(), and dynfield register, being 
called first in primary seems solving the problem.
Would you be OK to change last sentences as:
"Indeed, probe() is first invoked on the primary process when a device is 
created in a secondary, this enables registering dynfield from secondary process."

>> Bugzilla ID: 571
>> Fixes: d23d73d088c1 ("net/pcap: switch Rx timestamp to dynamic mbuf field")
>>
>> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

>> ---
> 
> One additional comment about this patch.
> 
> While it solves the issue described in the bug report, there may still
> be a gap when it is needed to register a dynamic field/flag from a
> secondary process. This happens when registering and configuring devices
> from a secondary process (is it supported?). It happens if the secondary
> process initializes a library which is not initialized in primary, and
> which requires a dynamic field.
> 
>  From afar, it does not look too difficult to implement dynamic field
> registration from secondary processes. The only thing missing is a way
> to allocate the shared memory in the primary process at initialization.
> Currently, there is no init callback that is invoked when eal init is
> done.
> 

I was checking this, it seems what prevents to register dnyfield from the 
secondary process is 'init_shared_mem()', so if primary process registers any 
dynfield first, secondary process can register dynfields too.
Do you think should this limitation documented?

> This is the exact same problem than for this issue:
> http://inbox.dpdk.org/dev/20200504074227.GA6327@platinum/#t
> 

<...>


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

* Re: [dpdk-dev] [PATCH] net/pcap: fix registration of timestamp dynamic field
  2020-11-13 14:02   ` Ferruh Yigit
@ 2020-11-13 17:23     ` Ferruh Yigit
  2020-11-13 20:35     ` Olivier Matz
  1 sibling, 0 replies; 5+ messages in thread
From: Ferruh Yigit @ 2020-11-13 17:23 UTC (permalink / raw)
  To: Olivier Matz, dev; +Cc: David Marchand, Thomas Monjalon

On 11/13/2020 2:02 PM, Ferruh Yigit wrote:
> On 11/13/2020 1:13 PM, Olivier Matz wrote:
>> On Fri, Nov 13, 2020 at 11:39:57AM +0100, Olivier Matz wrote:
>>> In pcap pmd, the timestamp mbuf dynamic field is mandatory. When the
>>> pcap pmd is created in a secondary process (this is the case for pdump),
>>> it cannot be registered because this is not allowed from a secondary
>>> process.
>>>
>>> To ensure that the field is properly registered, do it from probe()
>>> instead of configure(). Indeed, probe() is invoked on the primary
>>> process when a device is created in a secondary.
>>>
> 
> probe() invoked first in the primary, later in the secondary, both process calls 
> the driver probe(). But for this case probe(), and dynfield register, being 
> called first in primary seems solving the problem.
> Would you be OK to change last sentences as:
> "Indeed, probe() is first invoked on the primary process when a device is 
> created in a secondary, this enables registering dynfield from secondary process."
> 
>>> Bugzilla ID: 571
>>> Fixes: d23d73d088c1 ("net/pcap: switch Rx timestamp to dynamic mbuf field")
>>>
>>> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> 
> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
>
Applied to dpdk-next-net/main, thanks.

With above suggested change in the commit log.

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

* Re: [dpdk-dev] [PATCH] net/pcap: fix registration of timestamp dynamic field
  2020-11-13 14:02   ` Ferruh Yigit
  2020-11-13 17:23     ` Ferruh Yigit
@ 2020-11-13 20:35     ` Olivier Matz
  1 sibling, 0 replies; 5+ messages in thread
From: Olivier Matz @ 2020-11-13 20:35 UTC (permalink / raw)
  To: Ferruh Yigit; +Cc: dev, David Marchand, Thomas Monjalon

On Fri, Nov 13, 2020 at 02:02:38PM +0000, Ferruh Yigit wrote:
> On 11/13/2020 1:13 PM, Olivier Matz wrote:
> > On Fri, Nov 13, 2020 at 11:39:57AM +0100, Olivier Matz wrote:
> > > In pcap pmd, the timestamp mbuf dynamic field is mandatory. When the
> > > pcap pmd is created in a secondary process (this is the case for pdump),
> > > it cannot be registered because this is not allowed from a secondary
> > > process.
> > > 
> > > To ensure that the field is properly registered, do it from probe()
> > > instead of configure(). Indeed, probe() is invoked on the primary
> > > process when a device is created in a secondary.
> > > 
> 
> probe() invoked first in the primary, later in the secondary, both process
> calls the driver probe(). But for this case probe(), and dynfield register,
> being called first in primary seems solving the problem.
> Would you be OK to change last sentences as:
> "Indeed, probe() is first invoked on the primary process when a device is
> created in a secondary, this enables registering dynfield from secondary
> process."

Yes, it is more precise, thanks

> > > Bugzilla ID: 571
> > > Fixes: d23d73d088c1 ("net/pcap: switch Rx timestamp to dynamic mbuf field")
> > > 
> > > Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> 
> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
> 
> > > ---
> > 
> > One additional comment about this patch.
> > 
> > While it solves the issue described in the bug report, there may still
> > be a gap when it is needed to register a dynamic field/flag from a
> > secondary process. This happens when registering and configuring devices
> > from a secondary process (is it supported?). It happens if the secondary
> > process initializes a library which is not initialized in primary, and
> > which requires a dynamic field.
> > 
> >  From afar, it does not look too difficult to implement dynamic field
> > registration from secondary processes. The only thing missing is a way
> > to allocate the shared memory in the primary process at initialization.
> > Currently, there is no init callback that is invoked when eal init is
> > done.
> > 
> 
> I was checking this, it seems what prevents to register dnyfield from the
> secondary process is 'init_shared_mem()', so if primary process registers
> any dynfield first, secondary process can register dynfields too.
> Do you think should this limitation documented?

Currently, registration from secondary is explicitly denied, with a code
like this:

	if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
		rte_errno = EPERM;
		return -1;
	}

> > This is the exact same problem than for this issue:
> > http://inbox.dpdk.org/dev/20200504074227.GA6327@platinum/#t
> > 
> 
> <...>
> 

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

end of thread, other threads:[~2020-11-13 20:35 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-13 10:39 [dpdk-dev] [PATCH] net/pcap: fix registration of timestamp dynamic field Olivier Matz
2020-11-13 13:13 ` Olivier Matz
2020-11-13 14:02   ` Ferruh Yigit
2020-11-13 17:23     ` Ferruh Yigit
2020-11-13 20:35     ` Olivier Matz

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