DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] net/octeontx2: register dynamic mbuf timestamp field
@ 2020-11-03 14:16 Harman Kalra
  2020-11-03 14:24 ` Thomas Monjalon
  0 siblings, 1 reply; 4+ messages in thread
From: Harman Kalra @ 2020-11-03 14:16 UTC (permalink / raw)
  To: thomas, Jerin Jacob, Nithin Dabilpuram, Kiran Kumar K; +Cc: dev, 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


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

* Re: [dpdk-dev] [PATCH] net/octeontx2: register dynamic mbuf timestamp field
  2020-11-03 14:16 [dpdk-dev] [PATCH] net/octeontx2: register dynamic mbuf timestamp field Harman Kalra
@ 2020-11-03 14:24 ` Thomas Monjalon
  2020-11-03 14:26   ` Thomas Monjalon
  0 siblings, 1 reply; 4+ messages in thread
From: Thomas Monjalon @ 2020-11-03 14:24 UTC (permalink / raw)
  To: Harman Kalra; +Cc: Jerin Jacob, Nithin Dabilpuram, Kiran Kumar K, dev

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




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

* Re: [dpdk-dev] [PATCH] net/octeontx2: register dynamic mbuf timestamp field
  2020-11-03 14:24 ` Thomas Monjalon
@ 2020-11-03 14:26   ` Thomas Monjalon
  2020-11-03 14:48     ` [dpdk-dev] [EXT] " Harman Kalra
  0 siblings, 1 reply; 4+ messages in thread
From: Thomas Monjalon @ 2020-11-03 14:26 UTC (permalink / raw)
  To: Harman Kalra
  Cc: Jerin Jacob, Nithin Dabilpuram, Kiran Kumar K, dev, olivier.matz,
	david.marchand

+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
> >  	 */
> 






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

* Re: [dpdk-dev] [EXT] Re: [PATCH] net/octeontx2: register dynamic mbuf timestamp field
  2020-11-03 14:26   ` Thomas Monjalon
@ 2020-11-03 14:48     ` Harman Kalra
  0 siblings, 0 replies; 4+ messages in thread
From: Harman Kalra @ 2020-11-03 14:48 UTC (permalink / raw)
  To: Thomas Monjalon, avPRASAD#123
  Cc: Jerin Jacob, Nithin Dabilpuram, Kiran Kumar K, dev, olivier.matz,
	david.marchand

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

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

end of thread, other threads:[~2020-11-03 14:48 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-03 14:16 [dpdk-dev] [PATCH] net/octeontx2: register dynamic mbuf timestamp field Harman Kalra
2020-11-03 14:24 ` Thomas Monjalon
2020-11-03 14:26   ` Thomas Monjalon
2020-11-03 14:48     ` [dpdk-dev] [EXT] " Harman Kalra

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git