[PATCH net-next] net: add a helper to avoid issues with HW TX timestamping and SO_TXTIME

[PATCH net-next] net: add a helper to avoid issues with HW TX timestamping and SO_TXTIME

[Vladimir Oltean]

As explained in commit 29d98f54a4fe ("net: enetc: allow hardware
timestamping on TX queues with tc-etf enabled"), hardware TX
timestamping requires an skb with skb->tstamp = 0. When a packet is sent
with SO_TXTIME, the skb->skb_mstamp_ns corrupts the value of skb->tstamp,
so the drivers need to explicitly reset skb->tstamp to zero after
consuming the TX time.

Create a helper named skb_txtime_consumed() which does just that. All
drivers which offload TC_SETUP_QDISC_ETF should implement it, and it
would make it easier to assess during review whether they do the right
thing in order to be compatible with hardware timestamping or not.

Suggested-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/freescale/enetc/enetc.c | 8 ++------
 drivers/net/ethernet/intel/igb/igb_main.c    | 2 +-
 drivers/net/ethernet/intel/igc/igc_main.c    | 2 +-
 include/net/pkt_sched.h                      | 9 +++++++++
 4 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
index e85dfccb9ed1..5a54976e6a28 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
@@ -5,6 +5,7 @@
 #include <linux/tcp.h>
 #include <linux/udp.h>
 #include <linux/vmalloc.h>
+#include <net/pkt_sched.h>
 
 /* ENETC overhead: optional extension BD + 1 BD gap */
 #define ENETC_TXBDS_NEEDED(val)	((val) + 2)
@@ -293,12 +294,7 @@ static void enetc_tstamp_tx(struct sk_buff *skb, u64 tstamp)
 	if (skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS) {
 		memset(&shhwtstamps, 0, sizeof(shhwtstamps));
 		shhwtstamps.hwtstamp = ns_to_ktime(tstamp);
-		/* Ensure skb_mstamp_ns, which might have been populated with
-		 * the txtime, is not mistaken for a software timestamp,
-		 * because this will prevent the dispatch of our hardware
-		 * timestamp to the socket.
-		 */
-		skb->tstamp = ktime_set(0, 0);
+		skb_txtime_consumed(skb);
 		skb_tstamp_tx(skb, &shhwtstamps);
 	}
 }
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 878b31d534ec..369533feb4f2 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -5856,7 +5856,7 @@ static void igb_tx_ctxtdesc(struct igb_ring *tx_ring,
 	 */
 	if (tx_ring->launchtime_enable) {
 		ts = ktime_to_timespec64(first->skb->tstamp);
-		first->skb->tstamp = ktime_set(0, 0);
+		skb_txtime_consumed(first->skb);
 		context_desc->seqnum_seed = cpu_to_le32(ts.tv_nsec / 32);
 	} else {
 		context_desc->seqnum_seed = 0;
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 7ac9597ddb84..059ffcfb0bda 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -941,7 +941,7 @@ static void igc_tx_ctxtdesc(struct igc_ring *tx_ring,
 		struct igc_adapter *adapter = netdev_priv(tx_ring->netdev);
 		ktime_t txtime = first->skb->tstamp;
 
-		first->skb->tstamp = ktime_set(0, 0);
+		skb_txtime_consumed(first->skb);
 		context_desc->launch_time = igc_tx_launchtime(adapter,
 							      txtime);
 	} else {
diff --git a/include/net/pkt_sched.h b/include/net/pkt_sched.h
index 15b1b30f454e..f5c1bee0cd6a 100644
--- a/include/net/pkt_sched.h
+++ b/include/net/pkt_sched.h
@@ -188,4 +188,13 @@ struct tc_taprio_qopt_offload *taprio_offload_get(struct tc_taprio_qopt_offload
 						  *offload);
 void taprio_offload_free(struct tc_taprio_qopt_offload *offload);
 
+/* Ensure skb_mstamp_ns, which might have been populated with the txtime, is
+ * not mistaken for a software timestamp, because this will otherwise prevent
+ * the dispatch of hardware timestamps to the socket.
+ */
+static inline void skb_txtime_consumed(struct sk_buff *skb)
+{
+	skb->tstamp = ktime_set(0, 0);
+}
+
 #endif
-- 
2.25.1

Re: [PATCH net-next] net: add a helper to avoid issues with HW TX timestamping and SO_TXTIME

[Richard Cochran]

On Wed, Mar 10, 2021 at 04:50:44PM +0200, Vladimir Oltean wrote:
> As explained in commit 29d98f54a4fe ("net: enetc: allow hardware
> timestamping on TX queues with tc-etf enabled"), hardware TX
> timestamping requires an skb with skb->tstamp = 0. When a packet is sent
> with SO_TXTIME, the skb->skb_mstamp_ns corrupts the value of skb->tstamp,
> so the drivers need to explicitly reset skb->tstamp to zero after
> consuming the TX time.
> 
> Create a helper named skb_txtime_consumed() which does just that. All

Bikeshedding about the name: "consumed" suggests much more to me than
what is going on.

How about this?   skb_reset_txtime();

Thanks,
Richard

Re: [PATCH net-next] net: add a helper to avoid issues with HW TX timestamping and SO_TXTIME

[Vladimir Oltean]

On Wed, Mar 10, 2021 at 07:16:26AM -0800, Richard Cochran wrote:
> On Wed, Mar 10, 2021 at 04:50:44PM +0200, Vladimir Oltean wrote:
> > As explained in commit 29d98f54a4fe ("net: enetc: allow hardware
> > timestamping on TX queues with tc-etf enabled"), hardware TX
> > timestamping requires an skb with skb->tstamp = 0. When a packet is sent
> > with SO_TXTIME, the skb->skb_mstamp_ns corrupts the value of skb->tstamp,
> > so the drivers need to explicitly reset skb->tstamp to zero after
> > consuming the TX time.
> >
> > Create a helper named skb_txtime_consumed() which does just that. All
>
> Bikeshedding about the name: "consumed" suggests much more to me than
> what is going on.
>
> How about this?   skb_reset_txtime();

Not really a native speaker, but what more does it suggest? As far as
the Ethernet driver is concerned, it needs to consume the TX time (by
putting it into the TX buffer descriptor or whatever) and say it did
that. From the perspective of a driver writer I think it is intuitive to
have it called that way.

Re: [PATCH net-next] net: add a helper to avoid issues with HW TX timestamping and SO_TXTIME

[Vinicius Costa Gomes]

Vladimir Oltean <olteanv@gmail.com> writes:

> As explained in commit 29d98f54a4fe ("net: enetc: allow hardware
> timestamping on TX queues with tc-etf enabled"), hardware TX
> timestamping requires an skb with skb->tstamp = 0. When a packet is sent
> with SO_TXTIME, the skb->skb_mstamp_ns corrupts the value of skb->tstamp,
> so the drivers need to explicitly reset skb->tstamp to zero after
> consuming the TX time.
>
> Create a helper named skb_txtime_consumed() which does just that. All
> drivers which offload TC_SETUP_QDISC_ETF should implement it, and it

nitpick: to my ears, it seems that you meant "call"/"use" instead of
"implement".

> would make it easier to assess during review whether they do the right
> thing in order to be compatible with hardware timestamping or not.
>
> Suggested-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---

Acked-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>


Cheers,
-- 
Vinicius

Re: [PATCH net-next] net: add a helper to avoid issues with HW TX timestamping and SO_TXTIME

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (refs/heads/master):

On Wed, 10 Mar 2021 16:50:44 +0200 you wrote:
> As explained in commit 29d98f54a4fe ("net: enetc: allow hardware
> timestamping on TX queues with tc-etf enabled"), hardware TX
> timestamping requires an skb with skb->tstamp = 0. When a packet is sent
> with SO_TXTIME, the skb->skb_mstamp_ns corrupts the value of skb->tstamp,
> so the drivers need to explicitly reset skb->tstamp to zero after
> consuming the TX time.
> 
> [...]

Here is the summary with links:
  - [net-next] net: add a helper to avoid issues with HW TX timestamping and SO_TXTIME
    https://git.kernel.org/netdev/net-next/c/847cbfc014ad

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next] net: add a helper to avoid issues with HW TX timestamping and SO_TXTIME

[Vladimir Oltean]

Hi Po,

On Thu, Mar 11, 2021 at 02:30:12AM +0000, Po Liu wrote:
> Hi
>
> Can it just move
>
>  skb->tstamp = ktime_set(0, 0);
>
> into
>
> skb_tstamp_tx(skb, &shhwtstamps);
>
> if it always need to clear for HW tstamp setting.

I don't know if that works under all circumstances. Also, to keep the
driver interface consistent, we would also need to plug that into
skb_complete_tx_timestamp, for the case when the driver is working with
a clone directly.
It just seemed simpler to me to modify the few drivers which use SO_TXTIME.

RE: [PATCH net-next] net: add a helper to avoid issues with HW TX timestamping and SO_TXTIME

[Po Liu]

Hi

Can it just move

 skb->tstamp = ktime_set(0, 0);  

into 

skb_tstamp_tx(skb, &shhwtstamps);

if it always need to clear for HW tstamp setting.


> -----Original Message-----
> From: Vladimir Oltean <olteanv@gmail.com>
> Sent: 2021年3月10日 22:51
> To: Jakub Kicinski <kuba@kernel.org>; netdev@vger.kernel.org; Po Liu
> <po.liu@nxp.com>; Claudiu Manoil <claudiu.manoil@nxp.com>
> Cc: Vinicius Costa Gomes <vinicius.gomes@intel.com>; Richard Cochran
> <richardcochran@gmail.com>
> Subject: [PATCH net-next] net: add a helper to avoid issues with HW TX
> timestamping and SO_TXTIME
> 
> Caution: EXT Email
> 
> As explained in commit 29d98f54a4fe ("net: enetc: allow hardware
> timestamping on TX queues with tc-etf enabled"), hardware TX timestamping
> requires an skb with skb->tstamp = 0. When a packet is sent with SO_TXTIME,
> the skb->skb_mstamp_ns corrupts the value of skb->tstamp, so the drivers need
> to explicitly reset skb->tstamp to zero after consuming the TX time.
> 
> Create a helper named skb_txtime_consumed() which does just that. All drivers
> which offload TC_SETUP_QDISC_ETF should implement it, and it would make it
> easier to assess during review whether they do the right thing in order to be
> compatible with hardware timestamping or not.
> 
> Suggested-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  drivers/net/ethernet/freescale/enetc/enetc.c | 8 ++------
>  drivers/net/ethernet/intel/igb/igb_main.c    | 2 +-
>  drivers/net/ethernet/intel/igc/igc_main.c    | 2 +-
>  include/net/pkt_sched.h                      | 9 +++++++++
>  4 files changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c
> b/drivers/net/ethernet/freescale/enetc/enetc.c
> index e85dfccb9ed1..5a54976e6a28 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> @@ -5,6 +5,7 @@
>  #include <linux/tcp.h>
>  #include <linux/udp.h>
>  #include <linux/vmalloc.h>
> +#include <net/pkt_sched.h>
> 
>  /* ENETC overhead: optional extension BD + 1 BD gap */
>  #define ENETC_TXBDS_NEEDED(val)        ((val) + 2)
> @@ -293,12 +294,7 @@ static void enetc_tstamp_tx(struct sk_buff *skb, u64
> tstamp)
>         if (skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS) {
>                 memset(&shhwtstamps, 0, sizeof(shhwtstamps));
>                 shhwtstamps.hwtstamp = ns_to_ktime(tstamp);
> -               /* Ensure skb_mstamp_ns, which might have been populated with
> -                * the txtime, is not mistaken for a software timestamp,
> -                * because this will prevent the dispatch of our hardware
> -                * timestamp to the socket.
> -                */
> -               skb->tstamp = ktime_set(0, 0);
> +               skb_txtime_consumed(skb);
>                 skb_tstamp_tx(skb, &shhwtstamps);
>         }
>  }
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c
> b/drivers/net/ethernet/intel/igb/igb_main.c
> index 878b31d534ec..369533feb4f2 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -5856,7 +5856,7 @@ static void igb_tx_ctxtdesc(struct igb_ring *tx_ring,
>          */
>         if (tx_ring->launchtime_enable) {
>                 ts = ktime_to_timespec64(first->skb->tstamp);
> -               first->skb->tstamp = ktime_set(0, 0);
> +               skb_txtime_consumed(first->skb);
>                 context_desc->seqnum_seed = cpu_to_le32(ts.tv_nsec / 32);
>         } else {
>                 context_desc->seqnum_seed = 0; diff --git
> a/drivers/net/ethernet/intel/igc/igc_main.c
> b/drivers/net/ethernet/intel/igc/igc_main.c
> index 7ac9597ddb84..059ffcfb0bda 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -941,7 +941,7 @@ static void igc_tx_ctxtdesc(struct igc_ring *tx_ring,
>                 struct igc_adapter *adapter = netdev_priv(tx_ring->netdev);
>                 ktime_t txtime = first->skb->tstamp;
> 
> -               first->skb->tstamp = ktime_set(0, 0);
> +               skb_txtime_consumed(first->skb);
>                 context_desc->launch_time = igc_tx_launchtime(adapter,
>                                                               txtime);
>         } else {
> diff --git a/include/net/pkt_sched.h b/include/net/pkt_sched.h index
> 15b1b30f454e..f5c1bee0cd6a 100644
> --- a/include/net/pkt_sched.h
> +++ b/include/net/pkt_sched.h
> @@ -188,4 +188,13 @@ struct tc_taprio_qopt_offload
> *taprio_offload_get(struct tc_taprio_qopt_offload
>                                                   *offload);  void taprio_offload_free(struct
> tc_taprio_qopt_offload *offload);
> 
> +/* Ensure skb_mstamp_ns, which might have been populated with the
> +txtime, is
> + * not mistaken for a software timestamp, because this will otherwise
> +prevent
> + * the dispatch of hardware timestamps to the socket.
> + */
> +static inline void skb_txtime_consumed(struct sk_buff *skb) {
> +       skb->tstamp = ktime_set(0, 0);
> +}
> +
>  #endif
> --
> 2.25.1



Br,
Po Liu