[dpdk-dev] [PATCH] net/octeontx2: register dynamic mbuf timestamp field

[dpdk-dev] [PATCH] net/octeontx2: register dynamic mbuf timestamp field

[Harman Kalra]

A crash is observed if dynamic mbuf timestamp field is
registered in dev_start, as in most of the applications
rte_eth_timesync_enable is called after dev_start due
to which timestamp field did not get registered.

Signed-off-by: Harman Kalra <hkalra@marvell.com>
---
 drivers/net/octeontx2/otx2_ethdev.c | 10 ----------
 drivers/net/octeontx2/otx2_ptp.c    |  9 +++++++++
 2 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/net/octeontx2/otx2_ethdev.c b/drivers/net/octeontx2/otx2_ethdev.c
index f6962be9b..cfb733a4b 100644
--- a/drivers/net/octeontx2/otx2_ethdev.c
+++ b/drivers/net/octeontx2/otx2_ethdev.c
@@ -2219,16 +2219,6 @@ otx2_nix_dev_start(struct rte_eth_dev *eth_dev)
 	else
 		otx2_nix_timesync_disable(eth_dev);
 
-	if (dev->rx_offload_flags & NIX_RX_OFFLOAD_TSTAMP_F) {
-		rc = rte_mbuf_dyn_rx_timestamp_register(
-				&dev->tstamp.tstamp_dynfield_offset,
-				&dev->tstamp.rx_tstamp_dynflag);
-		if (rc != 0) {
-			otx2_err("Failed to register Rx timestamp field/flag");
-			return -rte_errno;
-		}
-	}
-
 	/* Update VF about data off shifted by 8 bytes if PTP already
 	 * enabled in PF owning this VF
 	 */
diff --git a/drivers/net/octeontx2/otx2_ptp.c b/drivers/net/octeontx2/otx2_ptp.c
index ae5a2b7cd..4aa68f578 100644
--- a/drivers/net/octeontx2/otx2_ptp.c
+++ b/drivers/net/octeontx2/otx2_ptp.c
@@ -256,6 +256,15 @@ otx2_nix_timesync_enable(struct rte_eth_dev *eth_dev)
 		/* Setting up the function pointers as per new offload flags */
 		otx2_eth_set_rx_function(eth_dev);
 		otx2_eth_set_tx_function(eth_dev);
+
+		/* Registering dynamic mbuf timestamp field and flag */
+		rc = rte_mbuf_dyn_rx_timestamp_register(
+				&dev->tstamp.tstamp_dynfield_offset,
+				&dev->tstamp.rx_tstamp_dynflag);
+		if (rc != 0) {
+			otx2_err("Failed to register Rx timestamp field/flag");
+			return -rte_errno;
+		}
 	}
 
 	rc = otx2_nix_recalc_mtu(eth_dev);
-- 
2.17.1

Re: [dpdk-dev] [PATCH] net/octeontx2: register dynamic mbuf timestamp field

[Thomas Monjalon]

03/11/2020 15:16, Harman Kalra:
> A crash is observed if dynamic mbuf timestamp field is
> registered in dev_start, as in most of the applications
> rte_eth_timesync_enable is called after dev_start due
> to which timestamp field did not get registered.

So you are not reading your emails?

I was waiting for you, so I looked at the ugly code of octeontx2
with Olivier and David, and we fixed it already.

Not reading emails is wasting time of everybody.
On the contrary, being available on IRC can speed up work.


> Signed-off-by: Harman Kalra <hkalra@marvell.com>
> --- a/drivers/net/octeontx2/otx2_ethdev.c
> +++ b/drivers/net/octeontx2/otx2_ethdev.c
> @@ -2219,16 +2219,6 @@ otx2_nix_dev_start(struct rte_eth_dev *eth_dev)
>  	else
>  		otx2_nix_timesync_disable(eth_dev);
>  
> -	if (dev->rx_offload_flags & NIX_RX_OFFLOAD_TSTAMP_F) {
> -		rc = rte_mbuf_dyn_rx_timestamp_register(
> -				&dev->tstamp.tstamp_dynfield_offset,
> -				&dev->tstamp.rx_tstamp_dynflag);
> -		if (rc != 0) {
> -			otx2_err("Failed to register Rx timestamp field/flag");
> -			return -rte_errno;
> -		}
> -	}
> -

This is wrong, you still need to register for the case
of DEV_RX_OFFLOAD_TIMESTAMP without timesync.

In my v5, it is moved below after VF special config.

>  	/* Update VF about data off shifted by 8 bytes if PTP already
>  	 * enabled in PF owning this VF
>  	 */

Re: [dpdk-dev] [PATCH] net/octeontx2: register dynamic mbuf timestamp field

[Thomas Monjalon]

+Cc David and Olivier to make them laugh or cry.

03/11/2020 15:24, Thomas Monjalon:
> 03/11/2020 15:16, Harman Kalra:
> > A crash is observed if dynamic mbuf timestamp field is
> > registered in dev_start, as in most of the applications
> > rte_eth_timesync_enable is called after dev_start due
> > to which timestamp field did not get registered.
> 
> So you are not reading your emails?
> 
> I was waiting for you, so I looked at the ugly code of octeontx2
> with Olivier and David, and we fixed it already.
> 
> Not reading emails is wasting time of everybody.
> On the contrary, being available on IRC can speed up work.
> 
> 
> > Signed-off-by: Harman Kalra <hkalra@marvell.com>
> > --- a/drivers/net/octeontx2/otx2_ethdev.c
> > +++ b/drivers/net/octeontx2/otx2_ethdev.c
> > @@ -2219,16 +2219,6 @@ otx2_nix_dev_start(struct rte_eth_dev *eth_dev)
> >  	else
> >  		otx2_nix_timesync_disable(eth_dev);
> >  
> > -	if (dev->rx_offload_flags & NIX_RX_OFFLOAD_TSTAMP_F) {
> > -		rc = rte_mbuf_dyn_rx_timestamp_register(
> > -				&dev->tstamp.tstamp_dynfield_offset,
> > -				&dev->tstamp.rx_tstamp_dynflag);
> > -		if (rc != 0) {
> > -			otx2_err("Failed to register Rx timestamp field/flag");
> > -			return -rte_errno;
> > -		}
> > -	}
> > -
> 
> This is wrong, you still need to register for the case
> of DEV_RX_OFFLOAD_TIMESTAMP without timesync.
> 
> In my v5, it is moved below after VF special config.
> 
> >  	/* Update VF about data off shifted by 8 bytes if PTP already
> >  	 * enabled in PF owning this VF
> >  	 */
> 

Re: [dpdk-dev] [EXT] Re: [PATCH] net/octeontx2: register dynamic mbuf timestamp field

[Harman Kalra]

On Tue, Nov 03, 2020 at 03:26:17PM +0100, Thomas Monjalon wrote:
> External Email
> 
> ----------------------------------------------------------------------
> +Cc David and Olivier to make them laugh or cry.
> 
> 03/11/2020 15:24, Thomas Monjalon:
> > 03/11/2020 15:16, Harman Kalra:
> > > A crash is observed if dynamic mbuf timestamp field is
> > > registered in dev_start, as in most of the applications
> > > rte_eth_timesync_enable is called after dev_start due
> > > to which timestamp field did not get registered.
> > 
> > So you are not reading your emails?
> > 
> > I was waiting for you, so I looked at the ugly code of octeontx2
> > with Olivier and David, and we fixed it already.
> > 
> > Not reading emails is wasting time of everybody.
> > On the contrary, being available on IRC can speed up work.
> > 
My focus was to fix the issue ASAP to meet the deadline as well as well
making sure that our performance nos are also not hampered with the
change.
By no means its a ugly code, its properly designed to cover many use cases.

While the changes done as part of v5 are partially correct becasue if
our kernel changes wrt PTP fails then its a unecessary registration.
https://patches.dpdk.org/patch/83600/

Thats why my patch does the registration only if kernel changes are
successful.


> > 
> > > Signed-off-by: Harman Kalra <hkalra@marvell.com>
> > > --- a/drivers/net/octeontx2/otx2_ethdev.c
> > > +++ b/drivers/net/octeontx2/otx2_ethdev.c
> > > @@ -2219,16 +2219,6 @@ otx2_nix_dev_start(struct rte_eth_dev *eth_dev)
> > >  	else
> > >  		otx2_nix_timesync_disable(eth_dev);
> > >  
> > > -	if (dev->rx_offload_flags & NIX_RX_OFFLOAD_TSTAMP_F) {
> > > -		rc = rte_mbuf_dyn_rx_timestamp_register(
> > > -				&dev->tstamp.tstamp_dynfield_offset,
> > > -				&dev->tstamp.rx_tstamp_dynflag);
> > > -		if (rc != 0) {
> > > -			otx2_err("Failed to register Rx timestamp field/flag");
> > > -			return -rte_errno;
> > > -		}
> > > -	}
> > > -
> > 
> > This is wrong, you still need to register for the case
> > of DEV_RX_OFFLOAD_TIMESTAMP without timesync.
> > 
> > In my v5, it is moved below after VF special config.
> > 
> > >  	/* Update VF about data off shifted by 8 bytes if PTP already
> > >  	 * enabled in PF owning this VF
> > >  	 */
> > 
> 
> 
> 
> 
>