[RFC PATCH 0/7] Extend socket timestamping API

[RFC PATCH 0/7] Extend socket timestamping API

[Miroslav Lichvar]

This patchset adds new options to the timestamping API that will be
useful for NTP implementations and possibly other applications.

The first patch specifies a timestamp filter for NTP packets, which is
handled in the second patch in drivers that can timestamp all packets.
There is no attempt to add the support to the phyter driver.

The third patch adds a new option to get information about
HW-timestamped packets. The fourth patch adds support for this option to
the drivers (currently only igb and e1000e).

The fifth patch fixes the code to not make a false software TX timestamp
when HW timestamping is enabled. The sixth patch depends on this fix.

The sixth patch adds a new option to allow outgoing packets to be looped
multiple times to the error queue in order to allow simultaneous SW and
HW timestamping. The seventh patch updates drivers that assumed SW
timestamping cannot be used together with HW timestamping.

Miroslav Lichvar (7):
  net: define receive timestamp filter for NTP
  net: ethernet: update drivers to handle HWTSTAMP_FILTER_NTP_ALL
  net: add option to get information about timestamped packets
  net: ethernet: update drivers to provide timestamping packet info
  net: don't make false software transmit timestamps
  net: allow simultaneous SW and HW transmit timestamping
  net: ethernet: update drivers to make both SW and HW TX timestamps

 Documentation/networking/timestamping.txt          | 20 +++++++++-
 drivers/net/ethernet/amd/xgbe/xgbe-drv.c           |  4 +-
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c   |  1 +
 drivers/net/ethernet/cavium/liquidio/lio_main.c    |  1 +
 drivers/net/ethernet/cavium/liquidio/lio_vf_main.c |  1 +
 drivers/net/ethernet/cavium/octeon/octeon_mgmt.c   |  1 +
 drivers/net/ethernet/intel/e1000e/netdev.c         | 19 +++++-----
 drivers/net/ethernet/intel/i40e/i40e_ptp.c         |  1 +
 drivers/net/ethernet/intel/igb/igb.h               |  7 ++--
 drivers/net/ethernet/intel/igb/igb_main.c          | 22 +++++++++--
 drivers/net/ethernet/intel/igb/igb_ptp.c           | 43 +++++++++++-----------
 drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c       |  1 +
 drivers/net/ethernet/mellanox/mlx4/en_netdev.c     |  1 +
 drivers/net/ethernet/mellanox/mlx5/core/en_clock.c |  1 +
 drivers/net/ethernet/neterion/vxge/vxge-main.c     |  1 +
 drivers/net/ethernet/qlogic/qede/qede_ptp.c        |  1 +
 drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c    |  3 +-
 drivers/net/ethernet/sfc/ef10.c                    |  1 +
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c  |  7 ++--
 drivers/net/ethernet/ti/cpsw.c                     |  1 +
 drivers/net/ethernet/tile/tilegx.c                 |  1 +
 include/linux/skbuff.h                             | 25 ++++++++++++-
 include/uapi/asm-generic/socket.h                  |  2 +
 include/uapi/linux/errqueue.h                      |  8 ++++
 include/uapi/linux/net_tstamp.h                    |  7 +++-
 net/core/dev_ioctl.c                               |  1 +
 net/core/skbuff.c                                  | 16 ++++++--
 net/socket.c                                       | 20 +++++++++-
 28 files changed, 162 insertions(+), 55 deletions(-)

-- 
2.9.3

[RFC PATCH 1/7] net: define receive timestamp filter for NTP

[Miroslav Lichvar]

Add HWTSTAMP_FILTER_NTP_ALL to the hwtstamp_rx_filters enum for
timestamping of NTP packets. There is currently only one driver that
could support it directly, but it may change in the future.

CC: Richard Cochran <richardcochran@gmail.com>
CC: Willem de Bruijn <willemb@google.com>
Signed-off-by: Miroslav Lichvar <mlichvar@redhat.com>
---
 include/uapi/linux/net_tstamp.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/uapi/linux/net_tstamp.h b/include/uapi/linux/net_tstamp.h
index 464dcca..0749fb1 100644
--- a/include/uapi/linux/net_tstamp.h
+++ b/include/uapi/linux/net_tstamp.h
@@ -125,6 +125,9 @@ enum hwtstamp_rx_filters {
 	HWTSTAMP_FILTER_PTP_V2_SYNC,
 	/* PTP v2/802.AS1, any layer, Delay_req packet */
 	HWTSTAMP_FILTER_PTP_V2_DELAY_REQ,
+
+	/* NTP, UDP, all versions and packet modes */
+	HWTSTAMP_FILTER_NTP_ALL,
 };
 
 #endif /* _NET_TIMESTAMPING_H */
-- 
2.9.3

[RFC PATCH 2/7] net: ethernet: update drivers to handle HWTSTAMP_FILTER_NTP_ALL

[Miroslav Lichvar]

Update drivers which can timestamp all packets to handle also
HWTSTAMP_FILTER_NTP_ALL.

CC: Richard Cochran <richardcochran@gmail.com>
CC: Willem de Bruijn <willemb@google.com>
Signed-off-by: Miroslav Lichvar <mlichvar@redhat.com>
---
 drivers/net/ethernet/amd/xgbe/xgbe-drv.c           | 1 +
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c   | 1 +
 drivers/net/ethernet/cavium/liquidio/lio_main.c    | 1 +
 drivers/net/ethernet/cavium/liquidio/lio_vf_main.c | 1 +
 drivers/net/ethernet/cavium/octeon/octeon_mgmt.c   | 1 +
 drivers/net/ethernet/intel/e1000e/netdev.c         | 1 +
 drivers/net/ethernet/intel/i40e/i40e_ptp.c         | 1 +
 drivers/net/ethernet/intel/igb/igb_ptp.c           | 1 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c       | 1 +
 drivers/net/ethernet/mellanox/mlx4/en_netdev.c     | 1 +
 drivers/net/ethernet/mellanox/mlx5/core/en_clock.c | 1 +
 drivers/net/ethernet/neterion/vxge/vxge-main.c     | 1 +
 drivers/net/ethernet/qlogic/qede/qede_ptp.c        | 1 +
 drivers/net/ethernet/sfc/ef10.c                    | 1 +
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c  | 1 +
 drivers/net/ethernet/ti/cpsw.c                     | 1 +
 drivers/net/ethernet/tile/tilegx.c                 | 1 +
 net/core/dev_ioctl.c                               | 1 +
 18 files changed, 18 insertions(+)

diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-drv.c b/drivers/net/ethernet/amd/xgbe/xgbe-drv.c
index c772420..89b21d7 100644
--- a/drivers/net/ethernet/amd/xgbe/xgbe-drv.c
+++ b/drivers/net/ethernet/amd/xgbe/xgbe-drv.c
@@ -1268,6 +1268,7 @@ static int xgbe_set_hwtstamp_settings(struct xgbe_prv_data *pdata,
 	case HWTSTAMP_FILTER_NONE:
 		break;
 
+	case HWTSTAMP_FILTER_NTP_ALL:
 	case HWTSTAMP_FILTER_ALL:
 		XGMAC_SET_BITS(mac_tscr, MAC_TSCR, TSENALL, 1);
 		XGMAC_SET_BITS(mac_tscr, MAC_TSCR, TSENA, 1);
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
index a851f95..2f30b1a 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
@@ -15351,6 +15351,7 @@ int bnx2x_configure_ptp_filters(struct bnx2x *bp)
 		break;
 	case HWTSTAMP_FILTER_ALL:
 	case HWTSTAMP_FILTER_SOME:
+	case HWTSTAMP_FILTER_NTP_ALL:
 		bp->rx_filter = HWTSTAMP_FILTER_NONE;
 		break;
 	case HWTSTAMP_FILTER_PTP_V1_L4_EVENT:
diff --git a/drivers/net/ethernet/cavium/liquidio/lio_main.c b/drivers/net/ethernet/cavium/liquidio/lio_main.c
index 927617c..7a0ef5b 100644
--- a/drivers/net/ethernet/cavium/liquidio/lio_main.c
+++ b/drivers/net/ethernet/cavium/liquidio/lio_main.c
@@ -3020,6 +3020,7 @@ static int hwtstamp_ioctl(struct net_device *netdev, struct ifreq *ifr)
 	case HWTSTAMP_FILTER_PTP_V2_EVENT:
 	case HWTSTAMP_FILTER_PTP_V2_SYNC:
 	case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
+	case HWTSTAMP_FILTER_NTP_ALL:
 		conf.rx_filter = HWTSTAMP_FILTER_ALL;
 		break;
 	default:
diff --git a/drivers/net/ethernet/cavium/liquidio/lio_vf_main.c b/drivers/net/ethernet/cavium/liquidio/lio_vf_main.c
index 34c7782..15e21b5 100644
--- a/drivers/net/ethernet/cavium/liquidio/lio_vf_main.c
+++ b/drivers/net/ethernet/cavium/liquidio/lio_vf_main.c
@@ -2100,6 +2100,7 @@ static int hwtstamp_ioctl(struct net_device *netdev, struct ifreq *ifr)
 	case HWTSTAMP_FILTER_PTP_V2_EVENT:
 	case HWTSTAMP_FILTER_PTP_V2_SYNC:
 	case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
+	case HWTSTAMP_FILTER_NTP_ALL:
 		conf.rx_filter = HWTSTAMP_FILTER_ALL;
 		break;
 	default:
diff --git a/drivers/net/ethernet/cavium/octeon/octeon_mgmt.c b/drivers/net/ethernet/cavium/octeon/octeon_mgmt.c
index a213868..2887bca 100644
--- a/drivers/net/ethernet/cavium/octeon/octeon_mgmt.c
+++ b/drivers/net/ethernet/cavium/octeon/octeon_mgmt.c
@@ -755,6 +755,7 @@ static int octeon_mgmt_ioctl_hwtstamp(struct net_device *netdev,
 	case HWTSTAMP_FILTER_PTP_V2_EVENT:
 	case HWTSTAMP_FILTER_PTP_V2_SYNC:
 	case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
+	case HWTSTAMP_FILTER_NTP_ALL:
 		p->has_rx_tstamp = have_hw_timestamps;
 		config.rx_filter = HWTSTAMP_FILTER_ALL;
 		if (p->has_rx_tstamp) {
diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index e9af89a..3a77054 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -3662,6 +3662,7 @@ static int e1000e_config_hwtstamp(struct e1000_adapter *adapter,
 		 * Delay Request messages but not both so fall-through to
 		 * time stamp all packets.
 		 */
+	case HWTSTAMP_FILTER_NTP_ALL:
 	case HWTSTAMP_FILTER_ALL:
 		is_l2 = true;
 		is_l4 = true;
diff --git a/drivers/net/ethernet/intel/i40e/i40e_ptp.c b/drivers/net/ethernet/intel/i40e/i40e_ptp.c
index 2caee35..198b770 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_ptp.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_ptp.c
@@ -562,6 +562,7 @@ static int i40e_ptp_set_timestamp_mode(struct i40e_pf *pf,
 			config->rx_filter = HWTSTAMP_FILTER_PTP_V2_L2_EVENT;
 		}
 		break;
+	case HWTSTAMP_FILTER_NTP_ALL:
 	case HWTSTAMP_FILTER_ALL:
 	default:
 		return -ERANGE;
diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c b/drivers/net/ethernet/intel/igb/igb_ptp.c
index 7a3fd4d..d333d6d 100644
--- a/drivers/net/ethernet/intel/igb/igb_ptp.c
+++ b/drivers/net/ethernet/intel/igb/igb_ptp.c
@@ -941,6 +941,7 @@ static int igb_ptp_set_timestamp_mode(struct igb_adapter *adapter,
 		is_l4 = true;
 		break;
 	case HWTSTAMP_FILTER_PTP_V1_L4_EVENT:
+	case HWTSTAMP_FILTER_NTP_ALL:
 	case HWTSTAMP_FILTER_ALL:
 		/* 82576 cannot timestamp all packets, which it needs to do to
 		 * support both V1 Sync and Delay_Req messages
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c
index ef0635e..d44c728 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c
@@ -883,6 +883,7 @@ static int ixgbe_ptp_set_timestamp_mode(struct ixgbe_adapter *adapter,
 				   IXGBE_FLAG_RX_HWTSTAMP_IN_REGISTER);
 		break;
 	case HWTSTAMP_FILTER_PTP_V1_L4_EVENT:
+	case HWTSTAMP_FILTER_NTP_ALL:
 	case HWTSTAMP_FILTER_ALL:
 		/* The X550 controller is capable of timestamping all packets,
 		 * which allows it to accept any filter.
diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
index 94fab20..8243674 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
@@ -2375,6 +2375,7 @@ static int mlx4_en_hwtstamp_set(struct net_device *dev, struct ifreq *ifr)
 	case HWTSTAMP_FILTER_PTP_V2_EVENT:
 	case HWTSTAMP_FILTER_PTP_V2_SYNC:
 	case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
+	case HWTSTAMP_FILTER_NTP_ALL:
 		config.rx_filter = HWTSTAMP_FILTER_ALL;
 		break;
 	default:
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_clock.c b/drivers/net/ethernet/mellanox/mlx5/core/en_clock.c
index e706a87..e294944 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_clock.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_clock.c
@@ -128,6 +128,7 @@ int mlx5e_hwstamp_set(struct net_device *dev, struct ifreq *ifr)
 	case HWTSTAMP_FILTER_PTP_V2_EVENT:
 	case HWTSTAMP_FILTER_PTP_V2_SYNC:
 	case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
+	case HWTSTAMP_FILTER_NTP_ALL:
 		/* Disable CQE compression */
 		netdev_warn(dev, "Disabling cqe compression");
 		err = mlx5e_modify_rx_cqe_compression_locked(priv, false);
diff --git a/drivers/net/ethernet/neterion/vxge/vxge-main.c b/drivers/net/ethernet/neterion/vxge/vxge-main.c
index 6a4310a..50ea69d 100644
--- a/drivers/net/ethernet/neterion/vxge/vxge-main.c
+++ b/drivers/net/ethernet/neterion/vxge/vxge-main.c
@@ -3218,6 +3218,7 @@ static int vxge_hwtstamp_set(struct vxgedev *vdev, void __user *data)
 	case HWTSTAMP_FILTER_PTP_V2_EVENT:
 	case HWTSTAMP_FILTER_PTP_V2_SYNC:
 	case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
+	case HWTSTAMP_FILTER_NTP_ALL:
 		if (vdev->devh->config.hwts_en != VXGE_HW_HWTS_ENABLE)
 			return -EFAULT;
 
diff --git a/drivers/net/ethernet/qlogic/qede/qede_ptp.c b/drivers/net/ethernet/qlogic/qede/qede_ptp.c
index 2e62dec..e977745 100644
--- a/drivers/net/ethernet/qlogic/qede/qede_ptp.c
+++ b/drivers/net/ethernet/qlogic/qede/qede_ptp.c
@@ -250,6 +250,7 @@ static int qede_ptp_cfg_filters(struct qede_dev *edev)
 		break;
 	case HWTSTAMP_FILTER_ALL:
 	case HWTSTAMP_FILTER_SOME:
+	case HWTSTAMP_FILTER_NTP_ALL:
 		ptp->rx_filter = HWTSTAMP_FILTER_NONE;
 		break;
 	case HWTSTAMP_FILTER_PTP_V1_L4_EVENT:
diff --git a/drivers/net/ethernet/sfc/ef10.c b/drivers/net/ethernet/sfc/ef10.c
index 78efb28..ad9c4de 100644
--- a/drivers/net/ethernet/sfc/ef10.c
+++ b/drivers/net/ethernet/sfc/ef10.c
@@ -6068,6 +6068,7 @@ static int efx_ef10_ptp_set_ts_config(struct efx_nic *efx,
 	case HWTSTAMP_FILTER_PTP_V2_EVENT:
 	case HWTSTAMP_FILTER_PTP_V2_SYNC:
 	case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
+	case HWTSTAMP_FILTER_NTP_ALL:
 		init->rx_filter = HWTSTAMP_FILTER_ALL;
 		rc = efx_ptp_change_mode(efx, true, 0);
 		if (!rc)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 85f315e..a290c50 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -644,6 +644,7 @@ static int stmmac_hwtstamp_ioctl(struct net_device *dev, struct ifreq *ifr)
 			ptp_over_ethernet = PTP_TCR_TSIPENA;
 			break;
 
+		case HWTSTAMP_FILTER_NTP_ALL:
 		case HWTSTAMP_FILTER_ALL:
 			/* time stamp any incoming packet */
 			config.rx_filter = HWTSTAMP_FILTER_ALL;
diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index fa674a8..8da70fa 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -1718,6 +1718,7 @@ static int cpsw_hwtstamp_set(struct net_device *dev, struct ifreq *ifr)
 	case HWTSTAMP_FILTER_PTP_V1_L4_EVENT:
 	case HWTSTAMP_FILTER_PTP_V1_L4_SYNC:
 	case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ:
+	case HWTSTAMP_FILTER_NTP_ALL:
 		return -ERANGE;
 	case HWTSTAMP_FILTER_PTP_V2_L4_EVENT:
 	case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
diff --git a/drivers/net/ethernet/tile/tilegx.c b/drivers/net/ethernet/tile/tilegx.c
index 7c634bc..aec9538 100644
--- a/drivers/net/ethernet/tile/tilegx.c
+++ b/drivers/net/ethernet/tile/tilegx.c
@@ -512,6 +512,7 @@ static int tile_hwtstamp_set(struct net_device *dev, struct ifreq *rq)
 	case HWTSTAMP_FILTER_PTP_V2_EVENT:
 	case HWTSTAMP_FILTER_PTP_V2_SYNC:
 	case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
+	case HWTSTAMP_FILTER_NTP_ALL:
 		config.rx_filter = HWTSTAMP_FILTER_ALL;
 		break;
 	default:
diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
index b94b1d2..77f04e7 100644
--- a/net/core/dev_ioctl.c
+++ b/net/core/dev_ioctl.c
@@ -225,6 +225,7 @@ static int net_hwtstamp_validate(struct ifreq *ifr)
 	case HWTSTAMP_FILTER_PTP_V2_EVENT:
 	case HWTSTAMP_FILTER_PTP_V2_SYNC:
 	case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
+	case HWTSTAMP_FILTER_NTP_ALL:
 		rx_filter_valid = 1;
 		break;
 	}
-- 
2.9.3

[RFC PATCH 3/7] net: add option to get information about timestamped packets

[Miroslav Lichvar]

Extend the skb_shared_hwtstamps structure with the index of the
real interface which received or transmitted the packet and the length
of the packet at layer 2. Add a SOF_TIMESTAMPING_OPT_PKTINFO flag to
the SO_TIMESTAMPING option to allow applications to get this information
as struct scm_ts_pktinfo in SCM_TIMESTAMPING_PKTINFO control message.

The index is mainly useful with bonding, bridges and other virtual
interfaces, where IP_PKTINFO doesn't provide the index of the real
interface. Applications may need it to determine which PHC made the
timestamp. With the L2 length it is possible to transpose preamble
timestamps to trailer timestamps, which are used in the NTP protocol.

Instead of adding a new check to a common path that might slow down
processing of received packets without hardware timestamps, the new
fields are expected to be set by the drivers together with the hardware
timestamp using the skb_hw_timestamp() function.

CC: Richard Cochran <richardcochran@gmail.com>
CC: Willem de Bruijn <willemb@google.com>
Signed-off-by: Miroslav Lichvar <mlichvar@redhat.com>
---
 Documentation/networking/timestamping.txt |  8 ++++++++
 include/linux/skbuff.h                    | 22 ++++++++++++++++++++++
 include/uapi/asm-generic/socket.h         |  2 ++
 include/uapi/linux/errqueue.h             |  8 ++++++++
 include/uapi/linux/net_tstamp.h           |  3 ++-
 net/core/skbuff.c                         | 12 +++++++++---
 net/socket.c                              | 11 ++++++++++-
 7 files changed, 61 insertions(+), 5 deletions(-)

diff --git a/Documentation/networking/timestamping.txt b/Documentation/networking/timestamping.txt
index 96f5069..ed04aaa 100644
--- a/Documentation/networking/timestamping.txt
+++ b/Documentation/networking/timestamping.txt
@@ -193,6 +193,14 @@ SOF_TIMESTAMPING_OPT_STATS:
   the transmit timestamps, such as how long a certain block of
   data was limited by peer's receiver window.
 
+SOF_TIMESTAMPING_OPT_PKTINFO:
+
+  Optional information about timestamped packets. It includes the
+  index of the real interface which received or transmitted the
+  packet and its length at layer 2. If the device driver provides
+  this information, it will be attached in struct scm_ts_pktinfo as
+  a separate control message of type SCM_TIMESTAMPING_PKTINFO.
+
 New applications are encouraged to pass SOF_TIMESTAMPING_OPT_ID to
 disambiguate timestamps and SOF_TIMESTAMPING_OPT_TSONLY to operate
 regardless of the setting of sysctl net.core.tstamp_allow_data.
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 741d75c..e91685a 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -349,6 +349,8 @@ static inline void skb_frag_size_sub(skb_frag_t *frag, int delta)
  * struct skb_shared_hwtstamps - hardware time stamps
  * @hwtstamp:	hardware time stamp transformed into duration
  *		since arbitrary point in time
+ * @if_index:	index of the interface which timestamped the packet
+ * @pkt_length:	length of the packet
  *
  * Software time stamps generated by ktime_get_real() are stored in
  * skb->tstamp.
@@ -361,6 +363,8 @@ static inline void skb_frag_size_sub(skb_frag_t *frag, int delta)
  */
 struct skb_shared_hwtstamps {
 	ktime_t	hwtstamp;
+	int if_index;
+	int pkt_length;
 };
 
 /* Definitions for tx_flags in struct skb_shared_info */
@@ -3322,6 +3326,24 @@ static inline void skb_tx_timestamp(struct sk_buff *skb)
 }
 
 /**
+ * skb_hw_timestamp - set hardware timestamp with packet information
+ *
+ * @skb: A socket buffer.
+ * @hwtstamp: The hardware timestamp.
+ * @if_index: The index of the interface which timestamped the packet.
+ * @pkt_len: The length of the packet.
+ */
+static inline void skb_hw_timestamp(struct sk_buff *skb, ktime_t hwtstamp,
+				    int if_index, int pkt_length)
+{
+	struct skb_shared_hwtstamps *hwtstamps = skb_hwtstamps(skb);
+
+	hwtstamps->hwtstamp = hwtstamp;
+	hwtstamps->if_index = if_index;
+	hwtstamps->pkt_length = pkt_length;
+}
+
+/**
  * skb_complete_wifi_ack - deliver skb with wifi status
  *
  * @skb: the original outgoing packet
diff --git a/include/uapi/asm-generic/socket.h b/include/uapi/asm-generic/socket.h
index 2b48856..a5f6e81 100644
--- a/include/uapi/asm-generic/socket.h
+++ b/include/uapi/asm-generic/socket.h
@@ -100,4 +100,6 @@
 
 #define SO_COOKIE		57
 
+#define SCM_TIMESTAMPING_PKTINFO	58
+
 #endif /* __ASM_GENERIC_SOCKET_H */
diff --git a/include/uapi/linux/errqueue.h b/include/uapi/linux/errqueue.h
index 07bdce1..66d752f 100644
--- a/include/uapi/linux/errqueue.h
+++ b/include/uapi/linux/errqueue.h
@@ -43,4 +43,12 @@ enum {
 	SCM_TSTAMP_ACK,		/* data acknowledged by peer */
 };
 
+/**
+ *	struct scm_ts_pktinfo - information about HW-timestamped packets
+ */
+struct scm_ts_pktinfo {
+	int if_index;
+	int pkt_length;
+};
+
 #endif /* _UAPI_LINUX_ERRQUEUE_H */
diff --git a/include/uapi/linux/net_tstamp.h b/include/uapi/linux/net_tstamp.h
index 0749fb1..8397ecd 100644
--- a/include/uapi/linux/net_tstamp.h
+++ b/include/uapi/linux/net_tstamp.h
@@ -26,8 +26,9 @@ enum {
 	SOF_TIMESTAMPING_OPT_CMSG = (1<<10),
 	SOF_TIMESTAMPING_OPT_TSONLY = (1<<11),
 	SOF_TIMESTAMPING_OPT_STATS = (1<<12),
+	SOF_TIMESTAMPING_OPT_PKTINFO = (1<<13),
 
-	SOF_TIMESTAMPING_LAST = SOF_TIMESTAMPING_OPT_STATS,
+	SOF_TIMESTAMPING_LAST = SOF_TIMESTAMPING_OPT_PKTINFO,
 	SOF_TIMESTAMPING_MASK = (SOF_TIMESTAMPING_LAST - 1) |
 				 SOF_TIMESTAMPING_LAST
 };
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 9f78109..7ca251f 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3888,10 +3888,16 @@ void __skb_tstamp_tx(struct sk_buff *orig_skb,
 		skb_shinfo(skb)->tskey = skb_shinfo(orig_skb)->tskey;
 	}
 
-	if (hwtstamps)
-		*skb_hwtstamps(skb) = *hwtstamps;
-	else
+	if (hwtstamps) {
+		skb_hwtstamps(skb)->hwtstamp = hwtstamps->hwtstamp;
+		if (sk->sk_tsflags & SOF_TIMESTAMPING_OPT_PKTINFO) {
+			skb_hwtstamps(skb)->if_index = orig_skb->dev->ifindex;
+			skb_hwtstamps(skb)->pkt_length = orig_skb->mac_len +
+							 orig_skb->len;
+		}
+	} else {
 		skb->tstamp = ktime_get_real();
+	}
 
 	__skb_complete_tx_timestamp(skb, sk, tstype, opt_stats);
 }
diff --git a/net/socket.c b/net/socket.c
index eea9970..f272019 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -670,6 +670,7 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk,
 {
 	int need_software_tstamp = sock_flag(sk, SOCK_RCVTSTAMP);
 	struct scm_timestamping tss;
+	struct scm_ts_pktinfo ts_pktinfo;
 	int empty = 1;
 	struct skb_shared_hwtstamps *shhwtstamps =
 		skb_hwtstamps(skb);
@@ -699,8 +700,16 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk,
 		empty = 0;
 	if (shhwtstamps &&
 	    (sk->sk_tsflags & SOF_TIMESTAMPING_RAW_HARDWARE) &&
-	    ktime_to_timespec_cond(shhwtstamps->hwtstamp, tss.ts + 2))
+	    ktime_to_timespec_cond(shhwtstamps->hwtstamp, tss.ts + 2)) {
+		if (sk->sk_tsflags & SOF_TIMESTAMPING_OPT_PKTINFO &&
+		    shhwtstamps->if_index) {
+			ts_pktinfo.if_index = shhwtstamps->if_index;
+			ts_pktinfo.pkt_length = shhwtstamps->pkt_length;
+			put_cmsg(msg, SOL_SOCKET, SCM_TIMESTAMPING_PKTINFO,
+				 sizeof(ts_pktinfo), &ts_pktinfo);
+		}
 		empty = 0;
+	}
 	if (!empty) {
 		put_cmsg(msg, SOL_SOCKET,
 			 SCM_TIMESTAMPING, sizeof(tss), &tss);
-- 
2.9.3

[RFC PATCH 4/7] net: ethernet: update drivers to provide timestamping packet info

[Miroslav Lichvar]

Update drivers that support hardware timestamping to provide the
interface index and packet length for the SOF_TIMESTAMPING_OPT_PKTINFO
option.

TODO: update other drivers (not just e1000e and igb)

CC: Richard Cochran <richardcochran@gmail.com>
CC: Willem de Bruijn <willemb@google.com>
CC: Jacob Keller <jacob.e.keller@intel.com>
Signed-off-by: Miroslav Lichvar <mlichvar@redhat.com>
---
 drivers/net/ethernet/intel/e1000e/netdev.c | 14 +++++-----
 drivers/net/ethernet/intel/igb/igb.h       |  7 ++---
 drivers/net/ethernet/intel/igb/igb_main.c  | 21 ++++++++++++---
 drivers/net/ethernet/intel/igb/igb_ptp.c   | 42 +++++++++++++++---------------
 4 files changed, 49 insertions(+), 35 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 3a77054..097b1ec2 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -508,9 +508,8 @@ static int e1000_desc_unused(struct e1000_ring *ring)
  * value involves reading two 32 bit registers. The first read latches the
  * value.
  **/
-static void e1000e_systim_to_hwtstamp(struct e1000_adapter *adapter,
-				      struct skb_shared_hwtstamps *hwtstamps,
-				      u64 systim)
+static ktime_t e1000e_systim_to_hwtstamp(struct e1000_adapter *adapter,
+					 u64 systim)
 {
 	u64 ns;
 	unsigned long flags;
@@ -519,8 +518,7 @@ static void e1000e_systim_to_hwtstamp(struct e1000_adapter *adapter,
 	ns = timecounter_cyc2time(&adapter->tc, systim);
 	spin_unlock_irqrestore(&adapter->systim_lock, flags);
 
-	memset(hwtstamps, 0, sizeof(*hwtstamps));
-	hwtstamps->hwtstamp = ns_to_ktime(ns);
+	return ns_to_ktime(ns);
 }
 
 /**
@@ -553,7 +551,8 @@ static void e1000e_rx_hwtstamp(struct e1000_adapter *adapter, u32 status,
 	 */
 	rxstmp = (u64)er32(RXSTMPL);
 	rxstmp |= (u64)er32(RXSTMPH) << 32;
-	e1000e_systim_to_hwtstamp(adapter, skb_hwtstamps(skb), rxstmp);
+	skb_hw_timestamp(skb, e1000e_systim_to_hwtstamp(adapter, rxstmp),
+			 adapter->netdev->ifindex, skb->len);
 
 	adapter->flags2 &= ~FLAG2_CHECK_RX_HWTSTAMP;
 }
@@ -1188,7 +1187,8 @@ static void e1000e_tx_hwtstamp_work(struct work_struct *work)
 		txstmp = er32(TXSTMPL);
 		txstmp |= (u64)er32(TXSTMPH) << 32;
 
-		e1000e_systim_to_hwtstamp(adapter, &shhwtstamps, txstmp);
+		shhwtstamps.hwtstamp =
+			e1000e_systim_to_hwtstamp(adapter, txstmp);
 
 		skb_tstamp_tx(adapter->tx_hwtstamp_skb, &shhwtstamps);
 		dev_kfree_skb_any(adapter->tx_hwtstamp_skb);
diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
index dc6e298..6ceccba 100644
--- a/drivers/net/ethernet/intel/igb/igb.h
+++ b/drivers/net/ethernet/intel/igb/igb.h
@@ -653,9 +653,10 @@ void igb_ptp_stop(struct igb_adapter *adapter);
 void igb_ptp_reset(struct igb_adapter *adapter);
 void igb_ptp_suspend(struct igb_adapter *adapter);
 void igb_ptp_rx_hang(struct igb_adapter *adapter);
-void igb_ptp_rx_rgtstamp(struct igb_q_vector *q_vector, struct sk_buff *skb);
-void igb_ptp_rx_pktstamp(struct igb_q_vector *q_vector, void *va,
-			 struct sk_buff *skb);
+ktime_t igb_ptp_rx_rgtstamp(struct igb_q_vector *q_vector,
+			    struct sk_buff *skb);
+ktime_t igb_ptp_rx_pktstamp(struct igb_q_vector *q_vector, void *va,
+			    struct sk_buff *skb);
 int igb_ptp_set_ts_config(struct net_device *netdev, struct ifreq *ifr);
 int igb_ptp_get_ts_config(struct net_device *netdev, struct ifreq *ifr);
 void igb_set_flag_queue_pairs(struct igb_adapter *, const u32);
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 26a821f..20014d92 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -6990,9 +6990,14 @@ static struct sk_buff *igb_construct_skb(struct igb_ring *rx_ring,
 		return NULL;
 
 	if (unlikely(igb_test_staterr(rx_desc, E1000_RXDADV_STAT_TSIP))) {
-		igb_ptp_rx_pktstamp(rx_ring->q_vector, va, skb);
+		ktime_t hwtstamp;
+
+		hwtstamp = igb_ptp_rx_pktstamp(rx_ring->q_vector, va, skb);
 		va += IGB_TS_HDR_LEN;
 		size -= IGB_TS_HDR_LEN;
+		/* FIXME: is size the L2 size of the packet? */
+		skb_hw_timestamp(skb, hwtstamp, rx_ring->netdev->ifindex,
+				 size);
 	}
 
 	/* Determine available headroom for copy */
@@ -7052,8 +7057,12 @@ static struct sk_buff *igb_build_skb(struct igb_ring *rx_ring,
 
 	/* pull timestamp out of packet data */
 	if (igb_test_staterr(rx_desc, E1000_RXDADV_STAT_TSIP)) {
-		igb_ptp_rx_pktstamp(rx_ring->q_vector, skb->data, skb);
+		ktime_t hwtstamp;
+
+		hwtstamp = igb_ptp_rx_pktstamp(rx_ring->q_vector, va, skb);
 		__skb_pull(skb, IGB_TS_HDR_LEN);
+		skb_hw_timestamp(skb, hwtstamp, rx_ring->netdev->ifindex,
+				 skb->len);
 	}
 
 	/* update buffer offset */
@@ -7199,8 +7208,12 @@ static void igb_process_skb_fields(struct igb_ring *rx_ring,
 	igb_rx_checksum(rx_ring, rx_desc, skb);
 
 	if (igb_test_staterr(rx_desc, E1000_RXDADV_STAT_TS) &&
-	    !igb_test_staterr(rx_desc, E1000_RXDADV_STAT_TSIP))
-		igb_ptp_rx_rgtstamp(rx_ring->q_vector, skb);
+	    !igb_test_staterr(rx_desc, E1000_RXDADV_STAT_TSIP)) {
+		ktime_t hwtstamp;
+
+		hwtstamp = igb_ptp_rx_rgtstamp(rx_ring->q_vector, skb);
+		skb_hw_timestamp(skb, hwtstamp, dev->ifindex, skb->len);
+	}
 
 	if ((dev->features & NETIF_F_HW_VLAN_CTAG_RX) &&
 	    igb_test_staterr(rx_desc, E1000_RXD_STAT_VP)) {
diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c b/drivers/net/ethernet/intel/igb/igb_ptp.c
index d333d6d..e903173 100644
--- a/drivers/net/ethernet/intel/igb/igb_ptp.c
+++ b/drivers/net/ethernet/intel/igb/igb_ptp.c
@@ -163,11 +163,11 @@ static void igb_ptp_write_i210(struct igb_adapter *adapter,
  * In addition, here have extended the system time with an overflow
  * counter in software.
  **/
-static void igb_ptp_systim_to_hwtstamp(struct igb_adapter *adapter,
-				       struct skb_shared_hwtstamps *hwtstamps,
-				       u64 systim)
+static ktime_t igb_ptp_systim_to_hwtstamp(struct igb_adapter *adapter,
+					  u64 systim)
 {
 	unsigned long flags;
+	ktime_t hwtstamp = 0;
 	u64 ns;
 
 	switch (adapter->hw.mac.type) {
@@ -181,19 +181,18 @@ static void igb_ptp_systim_to_hwtstamp(struct igb_adapter *adapter,
 
 		spin_unlock_irqrestore(&adapter->tmreg_lock, flags);
 
-		memset(hwtstamps, 0, sizeof(*hwtstamps));
-		hwtstamps->hwtstamp = ns_to_ktime(ns);
+		hwtstamp = ns_to_ktime(ns);
 		break;
 	case e1000_i210:
 	case e1000_i211:
-		memset(hwtstamps, 0, sizeof(*hwtstamps));
 		/* Upper 32 bits contain s, lower 32 bits contain ns. */
-		hwtstamps->hwtstamp = ktime_set(systim >> 32,
-						systim & 0xFFFFFFFF);
+		hwtstamp = ktime_set(systim >> 32, systim & 0xFFFFFFFF);
 		break;
 	default:
 		break;
 	}
+
+	return hwtstamp;
 }
 
 /* PTP clock operations */
@@ -729,7 +728,7 @@ static void igb_ptp_tx_hwtstamp(struct igb_adapter *adapter)
 	regval = rd32(E1000_TXSTMPL);
 	regval |= (u64)rd32(E1000_TXSTMPH) << 32;
 
-	igb_ptp_systim_to_hwtstamp(adapter, &shhwtstamps, regval);
+	shhwtstamps.hwtstamp = igb_ptp_systim_to_hwtstamp(adapter, regval);
 	/* adjust timestamp for the TX latency based on link speed */
 	if (adapter->hw.mac.type == e1000_i210) {
 		switch (adapter->link_speed) {
@@ -764,19 +763,19 @@ static void igb_ptp_tx_hwtstamp(struct igb_adapter *adapter)
  * incoming frame.  The value is stored in little endian format starting on
  * byte 8.
  **/
-void igb_ptp_rx_pktstamp(struct igb_q_vector *q_vector, void *va,
-			 struct sk_buff *skb)
+ktime_t igb_ptp_rx_pktstamp(struct igb_q_vector *q_vector, void *va,
+			    struct sk_buff *skb)
 {
 	__le64 *regval = (__le64 *)va;
 	struct igb_adapter *adapter = q_vector->adapter;
+	ktime_t hwtstamp;
 	int adjust = 0;
 
 	/* The timestamp is recorded in little endian format.
 	 * DWORD: 0        1        2        3
 	 * Field: Reserved Reserved SYSTIML  SYSTIMH
 	 */
-	igb_ptp_systim_to_hwtstamp(adapter, skb_hwtstamps(skb),
-				   le64_to_cpu(regval[1]));
+	hwtstamp = igb_ptp_systim_to_hwtstamp(adapter, le64_to_cpu(regval[1]));
 
 	/* adjust timestamp for the RX latency based on link speed */
 	if (adapter->hw.mac.type == e1000_i210) {
@@ -792,8 +791,8 @@ void igb_ptp_rx_pktstamp(struct igb_q_vector *q_vector, void *va,
 			break;
 		}
 	}
-	skb_hwtstamps(skb)->hwtstamp =
-		ktime_sub_ns(skb_hwtstamps(skb)->hwtstamp, adjust);
+
+	return ktime_sub_ns(hwtstamp, adjust);
 }
 
 /**
@@ -804,11 +803,12 @@ void igb_ptp_rx_pktstamp(struct igb_q_vector *q_vector, void *va,
  * This function is meant to retrieve a timestamp from the internal registers
  * of the adapter and store it in the skb.
  **/
-void igb_ptp_rx_rgtstamp(struct igb_q_vector *q_vector,
-			 struct sk_buff *skb)
+ktime_t igb_ptp_rx_rgtstamp(struct igb_q_vector *q_vector,
+			    struct sk_buff *skb)
 {
 	struct igb_adapter *adapter = q_vector->adapter;
 	struct e1000_hw *hw = &adapter->hw;
+	ktime_t hwtstamp;
 	u64 regval;
 	int adjust = 0;
 
@@ -823,12 +823,12 @@ void igb_ptp_rx_rgtstamp(struct igb_q_vector *q_vector,
 	 * can turn into a skb_shared_hwtstamps.
 	 */
 	if (!(rd32(E1000_TSYNCRXCTL) & E1000_TSYNCRXCTL_VALID))
-		return;
+		return 0;
 
 	regval = rd32(E1000_RXSTMPL);
 	regval |= (u64)rd32(E1000_RXSTMPH) << 32;
 
-	igb_ptp_systim_to_hwtstamp(adapter, skb_hwtstamps(skb), regval);
+	hwtstamp = igb_ptp_systim_to_hwtstamp(adapter, regval);
 
 	/* adjust timestamp for the RX latency based on link speed */
 	if (adapter->hw.mac.type == e1000_i210) {
@@ -844,13 +844,13 @@ void igb_ptp_rx_rgtstamp(struct igb_q_vector *q_vector,
 			break;
 		}
 	}
-	skb_hwtstamps(skb)->hwtstamp =
-		ktime_sub_ns(skb_hwtstamps(skb)->hwtstamp, adjust);
 
 	/* Update the last_rx_timestamp timer in order to enable watchdog check
 	 * for error case of latched timestamp on a dropped packet.
 	 */
 	adapter->last_rx_timestamp = jiffies;
+
+	return ktime_sub_ns(hwtstamp, adjust);
 }
 
 /**
-- 
2.9.3

[RFC PATCH 5/7] net: don't make false software transmit timestamps

[Miroslav Lichvar]

If software timestamping is enabled by the SO_TIMESTAMP(NS) option
when a message without timestamp is already waiting in the queue, the
__sock_recv_timestamp() function will read the current time to make a
timestamp in order to always have something for the application.

However, this applies also to outgoing packets looped back to the error
queue when hardware timestamping is enabled by the SO_TIMESTAMPING
option. A software transmit timestamp made after the actual transmission
is added to messages with hardware timestamps.

Modify the function to save the current time as a software timestamp
only if it's for a received packet (i.e. it's not in the error queue).

CC: Richard Cochran <richardcochran@gmail.com>
CC: Willem de Bruijn <willemb@google.com>
Signed-off-by: Miroslav Lichvar <mlichvar@redhat.com>
---
 net/socket.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/socket.c b/net/socket.c
index f272019..32e78de 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -668,7 +668,8 @@ static bool skb_is_err_queue(const struct sk_buff *skb)
 void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk,
 	struct sk_buff *skb)
 {
-	int need_software_tstamp = sock_flag(sk, SOCK_RCVTSTAMP);
+	int need_software_tstamp = sock_flag(sk, SOCK_RCVTSTAMP) &&
+				   !skb_is_err_queue(skb);
 	struct scm_timestamping tss;
 	struct scm_ts_pktinfo ts_pktinfo;
 	int empty = 1;
-- 
2.9.3

[RFC PATCH 6/7] net: allow simultaneous SW and HW transmit timestamping

[Miroslav Lichvar]

Add SOF_TIMESTAMPING_OPT_MULTIMSG option to allow looping the outgoing
packet to the socket's error queue with a software timestamp even when
a hardware transmit timestamp is expected to be provided by the driver.

Applications using this option will receive two separate messages from
the error queue, one with a software timestamp and the other with a
hardware timestamp. As the hardware timestamp is saved to the shared skb
info, which may happen before the first message with software timestamp
is received by the application, the hardware timestamp is copied to the
SCM_TIMESTAMPING control message only when the skb has no software
timestamp.

CC: Richard Cochran <richardcochran@gmail.com>
CC: Willem de Bruijn <willemb@google.com>
Signed-off-by: Miroslav Lichvar <mlichvar@redhat.com>
---
 Documentation/networking/timestamping.txt | 12 ++++++++++--
 include/linux/skbuff.h                    |  3 +--
 include/uapi/linux/net_tstamp.h           |  3 ++-
 net/core/skbuff.c                         |  4 ++++
 net/socket.c                              |  6 ++++++
 5 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/Documentation/networking/timestamping.txt b/Documentation/networking/timestamping.txt
index ed04aaa..8f30385 100644
--- a/Documentation/networking/timestamping.txt
+++ b/Documentation/networking/timestamping.txt
@@ -201,6 +201,12 @@ SOF_TIMESTAMPING_OPT_PKTINFO:
   this information, it will be attached in struct scm_ts_pktinfo as
   a separate control message of type SCM_TIMESTAMPING_PKTINFO.
 
+SOF_TIMESTAMPING_OPT_MULTIMSG:
+
+  Allow outgoing packets to be looped multiple times to the socket's
+  error queue in order to receive both software and hardware transmit
+  timestamps.
+
 New applications are encouraged to pass SOF_TIMESTAMPING_OPT_ID to
 disambiguate timestamps and SOF_TIMESTAMPING_OPT_TSONLY to operate
 regardless of the setting of sysctl net.core.tstamp_allow_data.
@@ -320,8 +326,10 @@ struct scm_timestamping {
 };
 
 The structure can return up to three timestamps. This is a legacy
-feature. Only one field is non-zero at any time. Most timestamps
-are passed in ts[0]. Hardware timestamps are passed in ts[2].
+feature. Most timestamps are passed in ts[0]. Hardware timestamps
+are passed in ts[2]. Incoming packets may have timestamps in both
+ts[0] and ts[2], but for outgoing packets only one field is non-zero
+at any time.
 
 ts[1] used to hold hardware timestamps converted to system time.
 Instead, expose the hardware clock device on the NIC directly as
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index e91685a..0387c4b 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -3302,8 +3302,7 @@ void skb_tstamp_tx(struct sk_buff *orig_skb,
 
 static inline void sw_tx_timestamp(struct sk_buff *skb)
 {
-	if (skb_shinfo(skb)->tx_flags & SKBTX_SW_TSTAMP &&
-	    !(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS))
+	if (skb_shinfo(skb)->tx_flags & SKBTX_SW_TSTAMP)
 		skb_tstamp_tx(skb, NULL);
 }
 
diff --git a/include/uapi/linux/net_tstamp.h b/include/uapi/linux/net_tstamp.h
index 8397ecd..887e3ff 100644
--- a/include/uapi/linux/net_tstamp.h
+++ b/include/uapi/linux/net_tstamp.h
@@ -27,8 +27,9 @@ enum {
 	SOF_TIMESTAMPING_OPT_TSONLY = (1<<11),
 	SOF_TIMESTAMPING_OPT_STATS = (1<<12),
 	SOF_TIMESTAMPING_OPT_PKTINFO = (1<<13),
+	SOF_TIMESTAMPING_OPT_MULTIMSG = (1<<14),
 
-	SOF_TIMESTAMPING_LAST = SOF_TIMESTAMPING_OPT_PKTINFO,
+	SOF_TIMESTAMPING_LAST = SOF_TIMESTAMPING_OPT_MULTIMSG,
 	SOF_TIMESTAMPING_MASK = (SOF_TIMESTAMPING_LAST - 1) |
 				 SOF_TIMESTAMPING_LAST
 };
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 7ca251f..d3df8ff 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3863,6 +3863,10 @@ void __skb_tstamp_tx(struct sk_buff *orig_skb,
 	if (!sk)
 		return;
 
+	if (!hwtstamps && !(sk->sk_tsflags & SOF_TIMESTAMPING_OPT_MULTIMSG) &&
+	    skb_shinfo(orig_skb)->tx_flags & SKBTX_IN_PROGRESS)
+		return;
+
 	tsonly = sk->sk_tsflags & SOF_TIMESTAMPING_OPT_TSONLY;
 	if (!skb_may_tx_timestamp(sk, tsonly))
 		return;
diff --git a/net/socket.c b/net/socket.c
index 32e78de..5c9f2eb 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -695,12 +695,18 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk,
 		}
 	}
 
+	/* Received packets may have both SW and HW timestamps in one control
+	 * message.  Transmitted packets may have only one timestamp in the
+	 * control message, but there may be two separate messages in the error
+	 * queue if the SOF_TIMESTAMPING_OPT_MULTIMSG option is enabled.
+	 */
 	memset(&tss, 0, sizeof(tss));
 	if ((sk->sk_tsflags & SOF_TIMESTAMPING_SOFTWARE) &&
 	    ktime_to_timespec_cond(skb->tstamp, tss.ts + 0))
 		empty = 0;
 	if (shhwtstamps &&
 	    (sk->sk_tsflags & SOF_TIMESTAMPING_RAW_HARDWARE) &&
+	    (empty || !skb_is_err_queue(skb)) &&
 	    ktime_to_timespec_cond(shhwtstamps->hwtstamp, tss.ts + 2)) {
 		if (sk->sk_tsflags & SOF_TIMESTAMPING_OPT_PKTINFO &&
 		    shhwtstamps->if_index) {
-- 
2.9.3

[RFC PATCH 7/7] net: ethernet: update drivers to make both SW and HW TX timestamps

[Miroslav Lichvar]

Some drivers were calling the skb_tx_timestamp() function only when
hardware timestamp was not requested. Now that applications have an
option to receive both timestamps in separate messages, the drivers need
to be modified to always call skb_tx_timestamp().

CC: Richard Cochran <richardcochran@gmail.com>
CC: Willem de Bruijn <willemb@google.com>
Signed-off-by: Miroslav Lichvar <mlichvar@redhat.com>
---
 drivers/net/ethernet/amd/xgbe/xgbe-drv.c          | 3 +--
 drivers/net/ethernet/intel/e1000e/netdev.c        | 4 ++--
 drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c   | 3 +--
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 6 ++----
 4 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-drv.c b/drivers/net/ethernet/amd/xgbe/xgbe-drv.c
index 89b21d7..5a2ad9c 100644
--- a/drivers/net/ethernet/amd/xgbe/xgbe-drv.c
+++ b/drivers/net/ethernet/amd/xgbe/xgbe-drv.c
@@ -1391,8 +1391,7 @@ static void xgbe_prep_tx_tstamp(struct xgbe_prv_data *pdata,
 		spin_unlock_irqrestore(&pdata->tstamp_lock, flags);
 	}
 
-	if (!XGMAC_GET_BITS(packet->attributes, TX_PACKET_ATTRIBUTES, PTP))
-		skb_tx_timestamp(skb);
+	skb_tx_timestamp(skb);
 }
 
 static void xgbe_prep_vlan(struct sk_buff *skb, struct xgbe_packet_data *packet)
diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 097b1ec2..eee047b 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -5850,10 +5850,10 @@ static netdev_tx_t e1000_xmit_frame(struct sk_buff *skb,
 			adapter->tx_hwtstamp_skb = skb_get(skb);
 			adapter->tx_hwtstamp_start = jiffies;
 			schedule_work(&adapter->tx_hwtstamp_work);
-		} else {
-			skb_tx_timestamp(skb);
 		}
 
+		skb_tx_timestamp(skb);
+
 		netdev_sent_queue(netdev, skb->len);
 		e1000_tx_queue(tx_ring, tx_flags, count);
 		/* Make sure there is space in the ring for the next send. */
diff --git a/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c b/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c
index d54490d..50c182c 100644
--- a/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c
+++ b/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c
@@ -1418,8 +1418,7 @@ static netdev_tx_t sxgbe_xmit(struct sk_buff *skb, struct net_device *dev)
 		priv->hw->desc->tx_enable_tstamp(first_desc);
 	}
 
-	if (!tqueue->hwts_tx_en)
-		skb_tx_timestamp(skb);
+	skb_tx_timestamp(skb);
 
 	priv->hw->dma->enable_dma_transmission(priv->ioaddr, txq_index);
 
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index a290c50..b866985 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -2880,8 +2880,7 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
 		priv->xstats.tx_set_ic_bit++;
 	}
 
-	if (!priv->hwts_tx_en)
-		skb_tx_timestamp(skb);
+	skb_tx_timestamp(skb);
 
 	if (unlikely((skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&
 		     priv->hwts_tx_en)) {
@@ -3083,8 +3082,7 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
 		priv->xstats.tx_set_ic_bit++;
 	}
 
-	if (!priv->hwts_tx_en)
-		skb_tx_timestamp(skb);
+	skb_tx_timestamp(skb);
 
 	/* Ready to fill the first descriptor and set the OWN bit w/o any
 	 * problems because all the descriptors are actually ready to be
-- 
2.9.3

Re: [RFC PATCH 2/7] net: ethernet: update drivers to handle HWTSTAMP_FILTER_NTP_ALL

[Richard Cochran]

On Wed, Apr 12, 2017 at 04:17:32PM +0200, Miroslav Lichvar wrote:
> Update drivers which can timestamp all packets to handle also
> HWTSTAMP_FILTER_NTP_ALL.

The $subject and message are bit confusing, as some of those do _not_
handle ALL ...

> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> index a851f95..2f30b1a 100644
> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> @@ -15351,6 +15351,7 @@ int bnx2x_configure_ptp_filters(struct bnx2x *bp)
>  		break;
>  	case HWTSTAMP_FILTER_ALL:
>  	case HWTSTAMP_FILTER_SOME:
> +	case HWTSTAMP_FILTER_NTP_ALL:
>  		bp->rx_filter = HWTSTAMP_FILTER_NONE;

... like here, for example.

Thanks,
Richard

RE: [RFC PATCH 2/7] net: ethernet: update drivers to handle HWTSTAMP_FILTER_NTP_ALL

[Keller, Jacob E]

> -----Original Message-----
> From: Miroslav Lichvar [mailto:mlichvar@redhat.com]
> Sent: Wednesday, April 12, 2017 7:18 AM
> To: netdev@vger.kernel.org
> Cc: Richard Cochran <richardcochran@gmail.com>; Willem de Bruijn
> <willemb@google.com>; Soheil Hassas Yeganeh <soheil@google.com>; Keller,
> Jacob E <jacob.e.keller@intel.com>; Denny Page <dennypage@me.com>; Jiri
> Benc <jbenc@redhat.com>
> Subject: [RFC PATCH 2/7] net: ethernet: update drivers to handle
> HWTSTAMP_FILTER_NTP_ALL
> 
> Update drivers which can timestamp all packets to handle also
> HWTSTAMP_FILTER_NTP_ALL.
> 

The code looks ok to me, though I agree with Richard that the description should be more clear that not all locations are about supporting the new feature (since many locations don't actually support ALL but merely error out)

Thanks,
Jake

RE: [RFC PATCH 4/7] net: ethernet: update drivers to provide timestamping packet info

[Keller, Jacob E]

> -----Original Message-----
> From: Miroslav Lichvar [mailto:mlichvar@redhat.com]
> Sent: Wednesday, April 12, 2017 7:18 AM
> To: netdev@vger.kernel.org
> Cc: Richard Cochran <richardcochran@gmail.com>; Willem de Bruijn
> <willemb@google.com>; Soheil Hassas Yeganeh <soheil@google.com>; Keller,
> Jacob E <jacob.e.keller@intel.com>; Denny Page <dennypage@me.com>; Jiri
> Benc <jbenc@redhat.com>
> Subject: [RFC PATCH 4/7] net: ethernet: update drivers to provide timestamping
> packet info
> 
> Update drivers that support hardware timestamping to provide the
> interface index and packet length for the SOF_TIMESTAMPING_OPT_PKTINFO
> option.
> 
> TODO: update other drivers (not just e1000e and igb)
> 
> CC: Richard Cochran <richardcochran@gmail.com>
> CC: Willem de Bruijn <willemb@google.com>
> CC: Jacob Keller <jacob.e.keller@intel.com>
> Signed-off-by: Miroslav Lichvar <mlichvar@redhat.com>
> ---

Driver changes seem alright to me. I might have gone a different route without changing the return value of functions but it doesn't make a huge difference, and i guess this route is easier to follow the code.

Thanks,
Jake

RE: [RFC PATCH 0/7] Extend socket timestamping API

[Keller, Jacob E]

> -----Original Message-----
> From: Miroslav Lichvar [mailto:mlichvar@redhat.com]
> Sent: Wednesday, April 12, 2017 7:18 AM
> To: netdev@vger.kernel.org
> Cc: Richard Cochran <richardcochran@gmail.com>; Willem de Bruijn
> <willemb@google.com>; Soheil Hassas Yeganeh <soheil@google.com>; Keller,
> Jacob E <jacob.e.keller@intel.com>; Denny Page <dennypage@me.com>; Jiri
> Benc <jbenc@redhat.com>
> Subject: [RFC PATCH 0/7] Extend socket timestamping API
> 
> This patchset adds new options to the timestamping API that will be
> useful for NTP implementations and possibly other applications.
> 
> The first patch specifies a timestamp filter for NTP packets, which is
> handled in the second patch in drivers that can timestamp all packets.
> There is no attempt to add the support to the phyter driver.
> 
> The third patch adds a new option to get information about
> HW-timestamped packets. The fourth patch adds support for this option to
> the drivers (currently only igb and e1000e).
> 
> The fifth patch fixes the code to not make a false software TX timestamp
> when HW timestamping is enabled. The sixth patch depends on this fix.
> 
> The sixth patch adds a new option to allow outgoing packets to be looped
> multiple times to the error queue in order to allow simultaneous SW and
> HW timestamping. The seventh patch updates drivers that assumed SW
> timestamping cannot be used together with HW timestamping.
> 

I think this looks pretty good, straight forward and useful. I agree with Richard about the one commit message, but the rest looks good modulo missing updates for all the other drivers.

I didn't see any code to update the ethtool get_ts_info data for _NTP_ALL either.

Thanks,
Jake

Re: [RFC PATCH 0/7] Extend socket timestamping API

[Miroslav Lichvar]

On Thu, Apr 13, 2017 at 09:08:08AM +0000, Keller, Jacob E wrote:
> > -----Original Message-----
> > This patchset adds new options to the timestamping API that will be
> > useful for NTP implementations and possibly other applications.

> I think this looks pretty good, straight forward and useful. I agree with Richard about the one commit message, but the rest looks good modulo missing updates for all the other drivers.

Thanks for the review.

> I didn't see any code to update the ethtool get_ts_info data for _NTP_ALL either.

My understanding was that ethtool should list only filters that are
actually supported by the HW and are not handled just by switching to
a more general filter. I think that's what drivers I've looked were
doing. The phyter driver would be the only one that could list the
filter as supported. Is that correct?

-- 
Miroslav Lichvar

RE: [RFC PATCH 0/7] Extend socket timestamping API

[Keller, Jacob E]

> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
> On Behalf Of Miroslav Lichvar
> Sent: Thursday, April 13, 2017 2:54 AM
> To: Keller, Jacob E <jacob.e.keller@intel.com>
> Cc: netdev@vger.kernel.org; Richard Cochran <richardcochran@gmail.com>;
> Willem de Bruijn <willemb@google.com>; Soheil Hassas Yeganeh
> <soheil@google.com>; Denny Page <dennypage@me.com>; Jiri Benc
> <jbenc@redhat.com>
> Subject: Re: [RFC PATCH 0/7] Extend socket timestamping API
> 
> On Thu, Apr 13, 2017 at 09:08:08AM +0000, Keller, Jacob E wrote:
> > > -----Original Message-----
> > > This patchset adds new options to the timestamping API that will be
> > > useful for NTP implementations and possibly other applications.
> 
> > I think this looks pretty good, straight forward and useful. I agree with Richard
> about the one commit message, but the rest looks good modulo missing updates
> for all the other drivers.
> 
> Thanks for the review.
> 
> > I didn't see any code to update the ethtool get_ts_info data for _NTP_ALL
> either.
> 
> My understanding was that ethtool should list only filters that are
> actually supported by the HW and are not handled just by switching to
> a more general filter. I think that's what drivers I've looked were
> doing. The phyter driver would be the only one that could list the
> filter as supported. Is that correct?
> 
> --
> Miroslav Lichvar

I'm not sure whether that's how all drivers are implemented, or whether that makes sense. Suppose an application wants to know if a particular filter is supported, they might ask using get_ts_info, but now the application must encode the rules for which filters are more general. I suppose this logic isn't all that complicated and is straight forward deductions based on the name and generally we prefer if driver or hardware supports timestamping all frames anyways.

In this regard I guess you'd be right.

Thanks,
Jake
 

Re: [RFC PATCH 6/7] net: allow simultaneous SW and HW transmit timestamping

[Willem de Bruijn]

On Wed, Apr 12, 2017 at 10:17 AM, Miroslav Lichvar <mlichvar@redhat.com> wrote:
> Add SOF_TIMESTAMPING_OPT_MULTIMSG option to allow looping the outgoing
> packet to the socket's error queue with a software timestamp even when
> a hardware transmit timestamp is expected to be provided by the driver.
>
> Applications using this option will receive two separate messages from
> the error queue, one with a software timestamp and the other with a
> hardware timestamp. As the hardware timestamp is saved to the shared skb
> info, which may happen before the first message with software timestamp
> is received by the application, the hardware timestamp is copied to the
> SCM_TIMESTAMPING control message only when the skb has no software
> timestamp.
>
> CC: Richard Cochran <richardcochran@gmail.com>
> CC: Willem de Bruijn <willemb@google.com>
> Signed-off-by: Miroslav Lichvar <mlichvar@redhat.com>
> ---
>  Documentation/networking/timestamping.txt | 12 ++++++++++--
>  include/linux/skbuff.h                    |  3 +--
>  include/uapi/linux/net_tstamp.h           |  3 ++-
>  net/core/skbuff.c                         |  4 ++++
>  net/socket.c                              |  6 ++++++
>  5 files changed, 23 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/networking/timestamping.txt b/Documentation/networking/timestamping.txt
> index ed04aaa..8f30385 100644
> --- a/Documentation/networking/timestamping.txt
> +++ b/Documentation/networking/timestamping.txt
> @@ -201,6 +201,12 @@ SOF_TIMESTAMPING_OPT_PKTINFO:
>    this information, it will be attached in struct scm_ts_pktinfo as
>    a separate control message of type SCM_TIMESTAMPING_PKTINFO.
>
> +SOF_TIMESTAMPING_OPT_MULTIMSG:
> +
> +  Allow outgoing packets to be looped multiple times to the socket's
> +  error queue in order to receive both software and hardware transmit
> +  timestamps.
> +

It is already possible to receive multiple copies of the same message.
Timestamps of type SCM_TSTAMP_SCHED will be passed for every
device, virtual or physical, such as bonding. With TCP retransmits,
multiple SCM_TSTAMP_SND can be observed.

The name for this option is therefore not very descriptive. Perhaps
SOF_TIMESTAMPING_OPT_BOTH_SW_HW.

>  New applications are encouraged to pass SOF_TIMESTAMPING_OPT_ID to
>  disambiguate timestamps and SOF_TIMESTAMPING_OPT_TSONLY to operate
>  regardless of the setting of sysctl net.core.tstamp_allow_data.
> @@ -320,8 +326,10 @@ struct scm_timestamping {
>  };
>
>  The structure can return up to three timestamps. This is a legacy
> -feature. Only one field is non-zero at any time. Most timestamps
> -are passed in ts[0]. Hardware timestamps are passed in ts[2].
> +feature. Most timestamps are passed in ts[0]. Hardware timestamps
> +are passed in ts[2]. Incoming packets may have timestamps in both
> +ts[0] and ts[2], but for outgoing packets only one field is non-zero
> +at any time.
>
>  ts[1] used to hold hardware timestamps converted to system time.
>  Instead, expose the hardware clock device on the NIC directly as
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index e91685a..0387c4b 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -3302,8 +3302,7 @@ void skb_tstamp_tx(struct sk_buff *orig_skb,
>
>  static inline void sw_tx_timestamp(struct sk_buff *skb)
>  {
> -       if (skb_shinfo(skb)->tx_flags & SKBTX_SW_TSTAMP &&
> -           !(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS))
> +       if (skb_shinfo(skb)->tx_flags & SKBTX_SW_TSTAMP)
>                 skb_tstamp_tx(skb, NULL);
>  }
>
> diff --git a/include/uapi/linux/net_tstamp.h b/include/uapi/linux/net_tstamp.h
> index 8397ecd..887e3ff 100644
> --- a/include/uapi/linux/net_tstamp.h
> +++ b/include/uapi/linux/net_tstamp.h
> @@ -27,8 +27,9 @@ enum {
>         SOF_TIMESTAMPING_OPT_TSONLY = (1<<11),
>         SOF_TIMESTAMPING_OPT_STATS = (1<<12),
>         SOF_TIMESTAMPING_OPT_PKTINFO = (1<<13),
> +       SOF_TIMESTAMPING_OPT_MULTIMSG = (1<<14),
>
> -       SOF_TIMESTAMPING_LAST = SOF_TIMESTAMPING_OPT_PKTINFO,
> +       SOF_TIMESTAMPING_LAST = SOF_TIMESTAMPING_OPT_MULTIMSG,
>         SOF_TIMESTAMPING_MASK = (SOF_TIMESTAMPING_LAST - 1) |
>                                  SOF_TIMESTAMPING_LAST
>  };
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 7ca251f..d3df8ff 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -3863,6 +3863,10 @@ void __skb_tstamp_tx(struct sk_buff *orig_skb,
>         if (!sk)
>                 return;
>
> +       if (!hwtstamps && !(sk->sk_tsflags & SOF_TIMESTAMPING_OPT_MULTIMSG) &&
> +           skb_shinfo(orig_skb)->tx_flags & SKBTX_IN_PROGRESS)
> +               return;
> +
>         tsonly = sk->sk_tsflags & SOF_TIMESTAMPING_OPT_TSONLY;
>         if (!skb_may_tx_timestamp(sk, tsonly))
>                 return;
> diff --git a/net/socket.c b/net/socket.c
> index 32e78de..5c9f2eb 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -695,12 +695,18 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk,
>                 }
>         }
>
> +       /* Received packets may have both SW and HW timestamps in one control
> +        * message.  Transmitted packets may have only one timestamp in the
> +        * control message, but there may be two separate messages in the error
> +        * queue if the SOF_TIMESTAMPING_OPT_MULTIMSG option is enabled.
> +        */

Unnecessary comment, and inexact given SCM_TSTAMP_SCHED and others.

>         memset(&tss, 0, sizeof(tss));
>         if ((sk->sk_tsflags & SOF_TIMESTAMPING_SOFTWARE) &&
>             ktime_to_timespec_cond(skb->tstamp, tss.ts + 0))
>                 empty = 0;
>         if (shhwtstamps &&
>             (sk->sk_tsflags & SOF_TIMESTAMPING_RAW_HARDWARE) &&
> +           (empty || !skb_is_err_queue(skb)) &&
>             ktime_to_timespec_cond(shhwtstamps->hwtstamp, tss.ts + 2)) {
>                 if (sk->sk_tsflags & SOF_TIMESTAMPING_OPT_PKTINFO &&
>                     shhwtstamps->if_index) {
> --
> 2.9.3
>

Re: [RFC PATCH 3/7] net: add option to get information about timestamped packets

[Willem de Bruijn]

On Wed, Apr 12, 2017 at 10:17 AM, Miroslav Lichvar <mlichvar@redhat.com> wrote:
> Extend the skb_shared_hwtstamps structure with the index of the
> real interface which received or transmitted the packet and the length
> of the packet at layer 2.

The original packet is received along with the timestamp. Why is this L2
length needed?

> Add a SOF_TIMESTAMPING_OPT_PKTINFO flag to
> the SO_TIMESTAMPING option to allow applications to get this information
> as struct scm_ts_pktinfo in SCM_TIMESTAMPING_PKTINFO control message.

This patch saves skb->dev->ifindex, which is the same as existing
SOF_TIMESTAMPING_OPT_CMSG. See also the bug fix for that
feature I sent yesterday: http://patchwork.ozlabs.org/patch/750197/

If the intent is to return a different ifindex, I would still suggest using
the existing pktinfo infrastructure, but changing the ifindex that is
recorded.

Re: [RFC PATCH 6/7] net: allow simultaneous SW and HW transmit timestamping

[Miroslav Lichvar]

On Thu, Apr 13, 2017 at 10:30:32AM -0400, Willem de Bruijn wrote:
> On Wed, Apr 12, 2017 at 10:17 AM, Miroslav Lichvar <mlichvar@redhat.com> wrote:
> > +SOF_TIMESTAMPING_OPT_MULTIMSG:
> > +
> > +  Allow outgoing packets to be looped multiple times to the socket's
> > +  error queue in order to receive both software and hardware transmit
> > +  timestamps.
> > +
> 
> It is already possible to receive multiple copies of the same message.
> Timestamps of type SCM_TSTAMP_SCHED will be passed for every
> device, virtual or physical, such as bonding. With TCP retransmits,
> multiple SCM_TSTAMP_SND can be observed.

Oh, I see. I was struggling to find a good name for this option.

> The name for this option is therefore not very descriptive. Perhaps
> SOF_TIMESTAMPING_OPT_BOTH_SW_HW.

Simultaneous SW/HW timestamping was already possible for incoming
packets. Maybe _OPT_TX_SWHW would be better?

-- 
Miroslav Lichvar

Re: [RFC PATCH 3/7] net: add option to get information about timestamped packets

[Miroslav Lichvar]

On Thu, Apr 13, 2017 at 10:37:07AM -0400, Willem de Bruijn wrote:
> On Wed, Apr 12, 2017 at 10:17 AM, Miroslav Lichvar <mlichvar@redhat.com> wrote:
> > Extend the skb_shared_hwtstamps structure with the index of the
> > real interface which received or transmitted the packet and the length
> > of the packet at layer 2.
> 
> The original packet is received along with the timestamp.

But only outgoing packets, right?

> Why is this L2 length needed?

It's needed for incoming packets to allow converting of preamble
timestamps to trailer timestamps.

> > Add a SOF_TIMESTAMPING_OPT_PKTINFO flag to
> > the SO_TIMESTAMPING option to allow applications to get this information
> > as struct scm_ts_pktinfo in SCM_TIMESTAMPING_PKTINFO control message.
> 
> This patch saves skb->dev->ifindex, which is the same as existing
> SOF_TIMESTAMPING_OPT_CMSG. See also the bug fix for that
> feature I sent yesterday: http://patchwork.ozlabs.org/patch/750197/

The main point is that it provides the index of the device which
received the packet. It does duplicate the functionality of OPT_CMSG +
IP_PKTINFO for outgoing packets, but I thought it might be useful with
the TSONLY option.

BTW, the original ifindex used to be in skb->skb_iif, but that changed
in b6858177.

> If the intent is to return a different ifindex, I would still suggest using
> the existing pktinfo infrastructure, but changing the ifindex that is
> recorded.

How would the application get the l2 length? If this (l2 length,
if_index) tuple is specific to timestamping, I think it would make
sense to keep it out of the IP layer.

-- 
Miroslav Lichvar

RE: [RFC PATCH 6/7] net: allow simultaneous SW and HW transmit timestamping

[Keller, Jacob E]

> -----Original Message-----
> From: Miroslav Lichvar [mailto:mlichvar@redhat.com]
> Sent: Thursday, April 13, 2017 8:00 AM
>
> Oh, I see. I was struggling to find a good name for this option.
> 
> > The name for this option is therefore not very descriptive. Perhaps
> > SOF_TIMESTAMPING_OPT_BOTH_SW_HW.
> 
> Simultaneous SW/HW timestamping was already possible for incoming
> packets. Maybe _OPT_TX_SWHW would be better?
>

This sounds more accurate to me.

Thanks,
Jake
 
> --
> Miroslav Lichvar

Re: [RFC PATCH 3/7] net: add option to get information about timestamped packets

[Willem de Bruijn]

On Thu, Apr 13, 2017 at 11:18 AM, Miroslav Lichvar <mlichvar@redhat.com> wrote:
> On Thu, Apr 13, 2017 at 10:37:07AM -0400, Willem de Bruijn wrote:
>> On Wed, Apr 12, 2017 at 10:17 AM, Miroslav Lichvar <mlichvar@redhat.com> wrote:
>> > Extend the skb_shared_hwtstamps structure with the index of the
>> > real interface which received or transmitted the packet and the length
>> > of the packet at layer 2.
>>
>> The original packet is received along with the timestamp.
>
> But only outgoing packets, right?

Timestamps for incoming packets are also passed alongside the original packet.

>> Why is this L2 length needed?
>
> It's needed for incoming packets to allow converting of preamble
> timestamps to trailer timestamps.

Receiving the mac length of a packet sounds like a feature independent
from timestamping. Either an ioctl similar to SIOCGIFMTU or, if it may
vary due to existince of vlan headers, a new independent cmsg at the
SOL_SOCKET layer.

>> > Add a SOF_TIMESTAMPING_OPT_PKTINFO flag to
>> > the SO_TIMESTAMPING option to allow applications to get this information
>> > as struct scm_ts_pktinfo in SCM_TIMESTAMPING_PKTINFO control message.
>>
>> This patch saves skb->dev->ifindex, which is the same as existing
>> SOF_TIMESTAMPING_OPT_CMSG. See also the bug fix for that
>> feature I sent yesterday: http://patchwork.ozlabs.org/patch/750197/
>
> The main point is that it provides the index of the device which
> received the packet. It does duplicate the functionality of OPT_CMSG +
> IP_PKTINFO for outgoing packets, but I thought it might be useful with
> the TSONLY option.

Agreed. I'd prefer to reuse the existing option for that and just extend it
to work together with TSONLY.

We will have to set serr->header.h4.iif from something other than skb->dev
if the skb was allocated fresh in __skb_tstamp_tx without the device
association.

Re: [RFC PATCH 6/7] net: allow simultaneous SW and HW transmit timestamping

[Willem de Bruijn]

On Thu, Apr 13, 2017 at 11:24 AM, Keller, Jacob E
<jacob.e.keller@intel.com> wrote:
>
>
>> -----Original Message-----
>> From: Miroslav Lichvar [mailto:mlichvar@redhat.com]
>> Sent: Thursday, April 13, 2017 8:00 AM
>>
>> Oh, I see. I was struggling to find a good name for this option.
>>
>> > The name for this option is therefore not very descriptive. Perhaps
>> > SOF_TIMESTAMPING_OPT_BOTH_SW_HW.
>>
>> Simultaneous SW/HW timestamping was already possible for incoming
>> packets. Maybe _OPT_TX_SWHW would be better?
>>
>
> This sounds more accurate to me.

Agreed.

Re: [RFC PATCH 3/7] net: add option to get information about timestamped packets

[Miroslav Lichvar]

On Thu, Apr 13, 2017 at 12:16:09PM -0400, Willem de Bruijn wrote:
> On Thu, Apr 13, 2017 at 11:18 AM, Miroslav Lichvar <mlichvar@redhat.com> wrote:
> > On Thu, Apr 13, 2017 at 10:37:07AM -0400, Willem de Bruijn wrote:
> >> Why is this L2 length needed?
> >
> > It's needed for incoming packets to allow converting of preamble
> > timestamps to trailer timestamps.
> 
> Receiving the mac length of a packet sounds like a feature independent
> from timestamping.

I agree, but so far nobody suggested another use for this information.
Do you have any suggestions?

The idea was that if it is useful only with HW timestamping, it would
be better to save it only with the timestamp, so there is no
performance impact in the more common case when HW timestamping is
disabled. Am I overly cautious here?

> Either an ioctl similar to SIOCGIFMTU or, if it may
> vary due to existince of vlan headers, a new independent cmsg at the
> SOL_SOCKET layer.

It's not just the VLAN headers. The length of the IP header may vary
with IP options, so the offset of the UDP data in the packet cannot be
assumed to be constant.

Now I'm wondering if it's actually necessary to save the original
value of skb->mac_len + skb->len. Would "skb->data - skb->head -
skb->mac_header + skb->len" always work as the L2 length for received
packets at the time when the cmsg is prepared?

As for the original ifindex, it seems to me it does need to be saved
to a new field since __netif_receive_skb_core() intentionally
overwrites skb->skb_iif. What would be the best place for it, sk_buff
or skb_shared_info?

And would it really be acceptable to save it for all packets in
__netif_receive_skb_core(), even when HW timestamping is disabled?
Seeing how the code and the data structures were optimized over time,
I have a feeling it would not be accepted.

-- 
Miroslav Lichvar

Re: [RFC PATCH 3/7] net: add option to get information about timestamped packets

[Willem de Bruijn]

On Mon, Apr 24, 2017 at 5:00 AM, Miroslav Lichvar <mlichvar@redhat.com> wrote:
> On Thu, Apr 13, 2017 at 12:16:09PM -0400, Willem de Bruijn wrote:
>> On Thu, Apr 13, 2017 at 11:18 AM, Miroslav Lichvar <mlichvar@redhat.com> wrote:
>> > On Thu, Apr 13, 2017 at 10:37:07AM -0400, Willem de Bruijn wrote:
>> >> Why is this L2 length needed?
>> >
>> > It's needed for incoming packets to allow converting of preamble
>> > timestamps to trailer timestamps.
>>
>> Receiving the mac length of a packet sounds like a feature independent
>> from timestamping.
>
> I agree, but so far nobody suggested another use for this information.
> Do you have any suggestions?
>
> The idea was that if it is useful only with HW timestamping, it would
> be better to save it only with the timestamp, so there is no
> performance impact in the more common case when HW timestamping is
> disabled. Am I overly cautious here?

The additional cost of a cmsg is zero for sockets that have no cmsg
enabled, due to

        if (inet->cmsg_flags)
                ip_cmsg_recv_offset(msg, sk, skb, sizeof(struct udphdr), off);

But you might be right that there are no uses outside the specific
timestamp requirement you have, so if you prefer to use a timestamp
option, I won't object further.

>> Either an ioctl similar to SIOCGIFMTU or, if it may
>> vary due to existince of vlan headers, a new independent cmsg at the
>> SOL_SOCKET layer.

The latter would require adding the SOL_SOCKET level cmsg processing
infra. It is simpler to just add it at the INET/INET6 levels.

> It's not just the VLAN headers. The length of the IP header may vary
> with IP options, so the offset of the UDP data in the packet cannot be
> assumed to be constant.

As well as tunnels.

> Now I'm wondering if it's actually necessary to save the original
> value of skb->mac_len + skb->len.

Computing it on recv if needed is definitely preferable to computing
on enqueue and storing in an intermediate variable.

> Would "skb->data - skb->head -
> skb->mac_header + skb->len" always work as the L2 length for received
> packets at the time when the cmsg is prepared?

(skb->data - skb->head) - skb->mac_header computes the length
of data before the mac, such as reserve? Do you mean skb->data -
skb->mac_header (or - skb_mac_offset(skb))?

> As for the original ifindex, it seems to me it does need to be saved
> to a new field since __netif_receive_skb_core() intentionally
> overwrites skb->skb_iif. What would be the best place for it, sk_buff
> or skb_shared_info?

Finding storage space on the receive path will not be easy.

One shortcut to avoid storing this information explicitly is to look up
the device from skb->napi_id.

> And would it really be acceptable to save it for all packets in
> __netif_receive_skb_core(), even when HW timestamping is disabled?
> Seeing how the code and the data structures were optimized over time,
> I have a feeling it would not be accepted.

Incurring this cost on all packets for such a rare edge case does sound
like a non-starter.

It can be called only if the netstamp_needed static key is enabled (false),
in __net_timestamp, though.

Re: [RFC PATCH 3/7] net: add option to get information about timestamped packets

[Miroslav Lichvar]

On Mon, Apr 24, 2017 at 11:18:13AM -0400, Willem de Bruijn wrote:
> On Mon, Apr 24, 2017 at 5:00 AM, Miroslav Lichvar <mlichvar@redhat.com> wrote:
> > Would "skb->data - skb->head -
> > skb->mac_header + skb->len" always work as the L2 length for received
> > packets at the time when the cmsg is prepared?
> 
> (skb->data - skb->head) - skb->mac_header computes the length
> of data before the mac, such as reserve?

data - head includes the reserve, but mac_header does too, so I think
it should be just the length of MAC header and everything up to the
data.

> Do you mean skb->data -
> skb->mac_header (or - skb_mac_offset(skb))?

That would give me a pointer? If I used skb_mac_offset(), the total
length would be just skb->len - skb_mac_offset()?

> > As for the original ifindex, it seems to me it does need to be saved
> > to a new field since __netif_receive_skb_core() intentionally
> > overwrites skb->skb_iif. What would be the best place for it, sk_buff
> > or skb_shared_info?
> 
> Finding storage space on the receive path will not be easy.
> 
> One shortcut to avoid storing this information explicitly is to look up
> the device from skb->napi_id.

Thanks. This looks promising. It will depend on CONFIG_NET_RX_BUSY_POLL,
but I guess that's ok. It nicely isolates all costs to the timestamping
option.

-- 
Miroslav Lichvar

Re: [RFC PATCH 3/7] net: add option to get information about timestamped packets

[Willem de Bruijn]

On Tue, Apr 25, 2017 at 9:56 AM, Miroslav Lichvar <mlichvar@redhat.com> wrote:
> On Mon, Apr 24, 2017 at 11:18:13AM -0400, Willem de Bruijn wrote:
>> On Mon, Apr 24, 2017 at 5:00 AM, Miroslav Lichvar <mlichvar@redhat.com> wrote:
>> > Would "skb->data - skb->head -
>> > skb->mac_header + skb->len" always work as the L2 length for received
>> > packets at the time when the cmsg is prepared?
>>
>> (skb->data - skb->head) - skb->mac_header computes the length
>> of data before the mac, such as reserve?
>
> data - head includes the reserve, but mac_header does too, so I think
> it should be just the length of MAC header and everything up to the
> data.
>
>> Do you mean skb->data -
>> skb->mac_header (or - skb_mac_offset(skb))?
>
> That would give me a pointer? If I used skb_mac_offset(), the total
> length would be just skb->len - skb_mac_offset()?

It appears so. The only existing caller first checks
skb_mac_header_was_set(skb).