[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 at 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
> > >  	 */
> > 
> 
> 
> 
> 
>