[next-queue RFC 0/4] ethtool: Add support for frame preemption

[next-queue RFC 0/4] ethtool: Add support for frame preemption

[Vinicius Costa Gomes]

Hi,

This series adds support for configuring frame preemption, as defined
by IEEE 802.1Q-2018 (previously IEEE 802.1Qbu) and IEEE 802.3br.

Frame preemption allows a packet from a higher priority queue marked
as "express" to preempt a packet from lower priority queue marked as
"preemptible". The idea is that this can help reduce the latency for
higher priority traffic.

Previously, the proposed interface for configuring these features was
using the qdisc layer. But as this is very hardware dependent and all
that qdisc did was pass the information to the driver, it makes sense
to have this in ethtool.

One example, for retrieving and setting the configuration:

$ ethtool $ sudo ./ethtool --show-frame-preemption enp3s0
Frame preemption settings for enp3s0:
	support: supported
	active: active
	supported queues: 0xf
	supported queues: 0xe
	minimum fragment size: 68


$ ethtool --set-frame-preemption enp3s0 fp on min-frag-size 68 preemptible-queues-mask 0xe

This is a RFC because I wanted to have feedback on some points:

  - The parameters added are enough for the hardware I have, is it
    enough in general?

  - even with the ethtool via netlink effort, I chose to keep the
    ioctl() way, in case someone wants to backport this to an older
    kernel, is there a problem with this?

  - Some space for bikeshedding the names and location (for example,
    does it make sense for these settings to be per-queue?), as I am
    not quite happy with them, one example, is the use of preemptible
    vs. preemptable;


About the patches, should be quite straightforward:

Patch 1, adds the ETHTOOL_GFP and ETHOOL_SFP commands and the
associated data structures;

Patch 2, adds the ETHTOOL_MSG_PREEMPT_GET and ETHTOOL_MSG_PREEMPT_SET
netlink messages and the associated attributes;

Patch 3, is the example implementation for the igc driver, the catch
here is that frame preemption in igc is dependent on the TSN "mode"
being enabled;

Patch 4, adds some registers that helped during implementation.

Another thing is that because igc is still under development, to avoid
conflicts, I think it might be easier for the PATCH version of this
series to go through Jeff Kirsher's tree.

Vinicius Costa Gomes (4):
  ethtool: Add support for configuring frame preemption
  ethtool: Add support for configuring frame preemption via netlink
  igc: Add support for configuring frame preemption
  igc: Add support for exposing frame preemption stats registers

 drivers/net/ethernet/intel/igc/igc.h         |   3 +
 drivers/net/ethernet/intel/igc/igc_defines.h |   6 +
 drivers/net/ethernet/intel/igc/igc_ethtool.c |  77 ++++++++
 drivers/net/ethernet/intel/igc/igc_regs.h    |  10 +
 drivers/net/ethernet/intel/igc/igc_tsn.c     |  46 ++++-
 include/linux/ethtool.h                      |   6 +
 include/uapi/linux/ethtool.h                 |  25 +++
 include/uapi/linux/ethtool_netlink.h         |  19 ++
 net/ethtool/Makefile                         |   3 +-
 net/ethtool/ioctl.c                          |  36 ++++
 net/ethtool/netlink.c                        |  15 ++
 net/ethtool/netlink.h                        |   2 +
 net/ethtool/preempt.c                        | 181 +++++++++++++++++++
 13 files changed, 423 insertions(+), 6 deletions(-)
 create mode 100644 net/ethtool/preempt.c

-- 
2.26.2

[Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Vinicius Costa Gomes]

Hi,

This series adds support for configuring frame preemption, as defined
by IEEE 802.1Q-2018 (previously IEEE 802.1Qbu) and IEEE 802.3br.

Frame preemption allows a packet from a higher priority queue marked
as "express" to preempt a packet from lower priority queue marked as
"preemptible". The idea is that this can help reduce the latency for
higher priority traffic.

Previously, the proposed interface for configuring these features was
using the qdisc layer. But as this is very hardware dependent and all
that qdisc did was pass the information to the driver, it makes sense
to have this in ethtool.

One example, for retrieving and setting the configuration:

$ ethtool $ sudo ./ethtool --show-frame-preemption enp3s0
Frame preemption settings for enp3s0:
	support: supported
	active: active
	supported queues: 0xf
	supported queues: 0xe
	minimum fragment size: 68


$ ethtool --set-frame-preemption enp3s0 fp on min-frag-size 68 preemptible-queues-mask 0xe

This is a RFC because I wanted to have feedback on some points:

  - The parameters added are enough for the hardware I have, is it
    enough in general?

  - even with the ethtool via netlink effort, I chose to keep the
    ioctl() way, in case someone wants to backport this to an older
    kernel, is there a problem with this?

  - Some space for bikeshedding the names and location (for example,
    does it make sense for these settings to be per-queue?), as I am
    not quite happy with them, one example, is the use of preemptible
    vs. preemptable;


About the patches, should be quite straightforward:

Patch 1, adds the ETHTOOL_GFP and ETHOOL_SFP commands and the
associated data structures;

Patch 2, adds the ETHTOOL_MSG_PREEMPT_GET and ETHTOOL_MSG_PREEMPT_SET
netlink messages and the associated attributes;

Patch 3, is the example implementation for the igc driver, the catch
here is that frame preemption in igc is dependent on the TSN "mode"
being enabled;

Patch 4, adds some registers that helped during implementation.

Another thing is that because igc is still under development, to avoid
conflicts, I think it might be easier for the PATCH version of this
series to go through Jeff Kirsher's tree.

Vinicius Costa Gomes (4):
  ethtool: Add support for configuring frame preemption
  ethtool: Add support for configuring frame preemption via netlink
  igc: Add support for configuring frame preemption
  igc: Add support for exposing frame preemption stats registers

 drivers/net/ethernet/intel/igc/igc.h         |   3 +
 drivers/net/ethernet/intel/igc/igc_defines.h |   6 +
 drivers/net/ethernet/intel/igc/igc_ethtool.c |  77 ++++++++
 drivers/net/ethernet/intel/igc/igc_regs.h    |  10 +
 drivers/net/ethernet/intel/igc/igc_tsn.c     |  46 ++++-
 include/linux/ethtool.h                      |   6 +
 include/uapi/linux/ethtool.h                 |  25 +++
 include/uapi/linux/ethtool_netlink.h         |  19 ++
 net/ethtool/Makefile                         |   3 +-
 net/ethtool/ioctl.c                          |  36 ++++
 net/ethtool/netlink.c                        |  15 ++
 net/ethtool/netlink.h                        |   2 +
 net/ethtool/preempt.c                        | 181 +++++++++++++++++++
 13 files changed, 423 insertions(+), 6 deletions(-)
 create mode 100644 net/ethtool/preempt.c

-- 
2.26.2

[next-queue RFC 1/4] ethtool: Add support for configuring frame preemption

[Vinicius Costa Gomes]

Frame preemption (described in IEEE 802.3br-2016) defines the concept
of preemptible and express queues. It allows traffic from express
queues to "interrupt" traffic from preemptible queues, which are
"resumed" after the express traffic has finished transmitting.

Frame preemption can only be used when both the local device and the
link partner support it.

A new ethtool command was added to support the configuration
parameters.

Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
 include/linux/ethtool.h      |  6 ++++++
 include/uapi/linux/ethtool.h | 25 +++++++++++++++++++++++++
 net/ethtool/ioctl.c          | 36 ++++++++++++++++++++++++++++++++++++
 3 files changed, 67 insertions(+)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index a23b26e..e4a6710 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -360,6 +360,8 @@ bool ethtool_convert_link_mode_to_legacy_u32(u32 *legacy_u32,
  * @get_ethtool_phy_stats: Return extended statistics about the PHY device.
  *	This is only useful if the device maintains PHY statistics and
  *	cannot use the standard PHY library helpers.
+ * @get_preempt: Get the network device Frame Preemption parameters.
+ * @set_preempt: Set the network device Frame Preemption parameters.
  *
  * All operations are optional (i.e. the function pointer may be set
  * to %NULL) and callers must take this into account.  Callers must
@@ -454,6 +456,10 @@ struct ethtool_ops {
 				      struct ethtool_fecparam *);
 	int	(*set_fecparam)(struct net_device *,
 				      struct ethtool_fecparam *);
+	int	(*get_preempt)(struct net_device *,
+			       struct ethtool_fp *);
+	int	(*set_preempt)(struct net_device *,
+			       struct ethtool_fp *);
 	void	(*get_ethtool_phy_stats)(struct net_device *,
 					 struct ethtool_stats *, u64 *);
 };
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index f4662b3..d63f9f8 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -369,6 +369,28 @@ struct ethtool_eee {
 	__u32	reserved[2];
 };
 
+/**
+ * struct ethtool_fp - Frame Preemption information
+ * @cmd: ETHTOOL_{G,S}FP
+ * @fp_supported: If frame preemption is supported.
+ * @fp_enabled: If frame preemption should be advertised to the link partner
+ *	as enabled.
+ * @supported_queues_mask: Bitmask indicating which queues support being
+ *	configured as preemptible (bit 0 -> queue 0, bit N -> queue N).
+ * @preemptible_queues_mask: Bitmask indicating which queues are
+ *	configured as preemptible (bit 0 -> queue 0, bit N -> queue N).
+ * @min_frag_size: Minimum size for all non-final fragment size.
+ */
+struct ethtool_fp {
+	__u32	cmd;
+	__u8	fp_supported;
+	__u8	fp_enabled;
+	__u32	supported_queues_mask;
+	__u32	preemptible_queues_mask;
+	__u32	min_frag_size;
+	__u32	reserved[2];
+};
+
 /**
  * struct ethtool_modinfo - plugin module eeprom information
  * @cmd: %ETHTOOL_GMODULEINFO
@@ -1441,6 +1463,9 @@ enum ethtool_fec_config_bits {
 #define ETHTOOL_GFECPARAM	0x00000050 /* Get FEC settings */
 #define ETHTOOL_SFECPARAM	0x00000051 /* Set FEC settings */
 
+#define ETHTOOL_GFP		0x00000052 /* Get Frame Preemption settings */
+#define ETHTOOL_SFP		0x00000053 /* Set Frame Preemption settings */
+
 /* compatibility with older code */
 #define SPARC_ETH_GSET		ETHTOOL_GSET
 #define SPARC_ETH_SSET		ETHTOOL_SSET
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 52102ab..e15ad5c 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -2531,6 +2531,36 @@ static int ethtool_set_fecparam(struct net_device *dev, void __user *useraddr)
 	return dev->ethtool_ops->set_fecparam(dev, &fecparam);
 }
 
+static int ethtool_get_preempt(struct net_device *dev, void __user *useraddr)
+{
+	struct ethtool_fp fpparam = { .cmd = ETHTOOL_GFP };
+	int rc;
+
+	if (!dev->ethtool_ops->get_preempt)
+		return -EOPNOTSUPP;
+
+	rc = dev->ethtool_ops->get_preempt(dev, &fpparam);
+	if (rc)
+		return rc;
+
+	if (copy_to_user(useraddr, &fpparam, sizeof(fpparam)))
+		return -EFAULT;
+	return 0;
+}
+
+static int ethtool_set_preempt(struct net_device *dev, void __user *useraddr)
+{
+	struct ethtool_fp fpparam;
+
+	if (!dev->ethtool_ops->set_preempt)
+		return -EOPNOTSUPP;
+
+	if (copy_from_user(&fpparam, useraddr, sizeof(fpparam)))
+		return -EFAULT;
+
+	return dev->ethtool_ops->set_preempt(dev, &fpparam);
+}
+
 /* The main entry point in this file.  Called from net/core/dev_ioctl.c */
 
 int dev_ethtool(struct net *net, struct ifreq *ifr)
@@ -2810,6 +2840,12 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 	case ETHTOOL_SFECPARAM:
 		rc = ethtool_set_fecparam(dev, useraddr);
 		break;
+	case ETHTOOL_GFP:
+		rc = ethtool_get_preempt(dev, useraddr);
+		break;
+	case ETHTOOL_SFP:
+		rc = ethtool_set_preempt(dev, useraddr);
+		break;
 	default:
 		rc = -EOPNOTSUPP;
 	}
-- 
2.26.2

[Intel-wired-lan] [next-queue RFC 1/4] ethtool: Add support for configuring frame preemption

[Vinicius Costa Gomes]

Frame preemption (described in IEEE 802.3br-2016) defines the concept
of preemptible and express queues. It allows traffic from express
queues to "interrupt" traffic from preemptible queues, which are
"resumed" after the express traffic has finished transmitting.

Frame preemption can only be used when both the local device and the
link partner support it.

A new ethtool command was added to support the configuration
parameters.

Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
 include/linux/ethtool.h      |  6 ++++++
 include/uapi/linux/ethtool.h | 25 +++++++++++++++++++++++++
 net/ethtool/ioctl.c          | 36 ++++++++++++++++++++++++++++++++++++
 3 files changed, 67 insertions(+)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index a23b26e..e4a6710 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -360,6 +360,8 @@ bool ethtool_convert_link_mode_to_legacy_u32(u32 *legacy_u32,
  * @get_ethtool_phy_stats: Return extended statistics about the PHY device.
  *	This is only useful if the device maintains PHY statistics and
  *	cannot use the standard PHY library helpers.
+ * @get_preempt: Get the network device Frame Preemption parameters.
+ * @set_preempt: Set the network device Frame Preemption parameters.
  *
  * All operations are optional (i.e. the function pointer may be set
  * to %NULL) and callers must take this into account.  Callers must
@@ -454,6 +456,10 @@ struct ethtool_ops {
 				      struct ethtool_fecparam *);
 	int	(*set_fecparam)(struct net_device *,
 				      struct ethtool_fecparam *);
+	int	(*get_preempt)(struct net_device *,
+			       struct ethtool_fp *);
+	int	(*set_preempt)(struct net_device *,
+			       struct ethtool_fp *);
 	void	(*get_ethtool_phy_stats)(struct net_device *,
 					 struct ethtool_stats *, u64 *);
 };
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index f4662b3..d63f9f8 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -369,6 +369,28 @@ struct ethtool_eee {
 	__u32	reserved[2];
 };
 
+/**
+ * struct ethtool_fp - Frame Preemption information
+ * @cmd: ETHTOOL_{G,S}FP
+ * @fp_supported: If frame preemption is supported.
+ * @fp_enabled: If frame preemption should be advertised to the link partner
+ *	as enabled.
+ * @supported_queues_mask: Bitmask indicating which queues support being
+ *	configured as preemptible (bit 0 -> queue 0, bit N -> queue N).
+ * @preemptible_queues_mask: Bitmask indicating which queues are
+ *	configured as preemptible (bit 0 -> queue 0, bit N -> queue N).
+ * @min_frag_size: Minimum size for all non-final fragment size.
+ */
+struct ethtool_fp {
+	__u32	cmd;
+	__u8	fp_supported;
+	__u8	fp_enabled;
+	__u32	supported_queues_mask;
+	__u32	preemptible_queues_mask;
+	__u32	min_frag_size;
+	__u32	reserved[2];
+};
+
 /**
  * struct ethtool_modinfo - plugin module eeprom information
  * @cmd: %ETHTOOL_GMODULEINFO
@@ -1441,6 +1463,9 @@ enum ethtool_fec_config_bits {
 #define ETHTOOL_GFECPARAM	0x00000050 /* Get FEC settings */
 #define ETHTOOL_SFECPARAM	0x00000051 /* Set FEC settings */
 
+#define ETHTOOL_GFP		0x00000052 /* Get Frame Preemption settings */
+#define ETHTOOL_SFP		0x00000053 /* Set Frame Preemption settings */
+
 /* compatibility with older code */
 #define SPARC_ETH_GSET		ETHTOOL_GSET
 #define SPARC_ETH_SSET		ETHTOOL_SSET
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 52102ab..e15ad5c 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -2531,6 +2531,36 @@ static int ethtool_set_fecparam(struct net_device *dev, void __user *useraddr)
 	return dev->ethtool_ops->set_fecparam(dev, &fecparam);
 }
 
+static int ethtool_get_preempt(struct net_device *dev, void __user *useraddr)
+{
+	struct ethtool_fp fpparam = { .cmd = ETHTOOL_GFP };
+	int rc;
+
+	if (!dev->ethtool_ops->get_preempt)
+		return -EOPNOTSUPP;
+
+	rc = dev->ethtool_ops->get_preempt(dev, &fpparam);
+	if (rc)
+		return rc;
+
+	if (copy_to_user(useraddr, &fpparam, sizeof(fpparam)))
+		return -EFAULT;
+	return 0;
+}
+
+static int ethtool_set_preempt(struct net_device *dev, void __user *useraddr)
+{
+	struct ethtool_fp fpparam;
+
+	if (!dev->ethtool_ops->set_preempt)
+		return -EOPNOTSUPP;
+
+	if (copy_from_user(&fpparam, useraddr, sizeof(fpparam)))
+		return -EFAULT;
+
+	return dev->ethtool_ops->set_preempt(dev, &fpparam);
+}
+
 /* The main entry point in this file.  Called from net/core/dev_ioctl.c */
 
 int dev_ethtool(struct net *net, struct ifreq *ifr)
@@ -2810,6 +2840,12 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 	case ETHTOOL_SFECPARAM:
 		rc = ethtool_set_fecparam(dev, useraddr);
 		break;
+	case ETHTOOL_GFP:
+		rc = ethtool_get_preempt(dev, useraddr);
+		break;
+	case ETHTOOL_SFP:
+		rc = ethtool_set_preempt(dev, useraddr);
+		break;
 	default:
 		rc = -EOPNOTSUPP;
 	}
-- 
2.26.2

[next-queue RFC 2/4] ethtool: Add support for configuring frame preemption via netlink

[Vinicius Costa Gomes]

ethtool is gaining support for using netlink as transport for its
messages, being an alternative to ioctl() calls.

Frame preemption, being new, makes a good target for being added to
the list of features that are also supported via the netlink
transport.

Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
 include/uapi/linux/ethtool_netlink.h |  19 +++
 net/ethtool/Makefile                 |   3 +-
 net/ethtool/netlink.c                |  15 +++
 net/ethtool/netlink.h                |   2 +
 net/ethtool/preempt.c                | 181 +++++++++++++++++++++++++++
 5 files changed, 219 insertions(+), 1 deletion(-)
 create mode 100644 net/ethtool/preempt.c

diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index 2881af4..21afba1 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -40,6 +40,8 @@ enum {
 	ETHTOOL_MSG_EEE_SET,
 	ETHTOOL_MSG_TSINFO_GET,
 	ETHTOOL_MSG_CABLE_TEST_ACT,
+	ETHTOOL_MSG_PREEMPT_GET,
+	ETHTOOL_MSG_PREEMPT_SET,
 
 	/* add new constants above here */
 	__ETHTOOL_MSG_USER_CNT,
@@ -76,6 +78,8 @@ enum {
 	ETHTOOL_MSG_EEE_NTF,
 	ETHTOOL_MSG_TSINFO_GET_REPLY,
 	ETHTOOL_MSG_CABLE_TEST_NTF,
+	ETHTOOL_MSG_PREEMPT_GET_REPLY,
+	ETHTOOL_MSG_PREEMPT_NTF,
 
 	/* add new constants above here */
 	__ETHTOOL_MSG_KERNEL_CNT,
@@ -476,6 +480,21 @@ enum {
 	ETHTOOL_A_CABLE_TEST_NTF_MAX = (__ETHTOOL_A_CABLE_TEST_NTF_CNT - 1)
 };
 
+/* FRAME PREEMPTION */
+enum {
+	ETHTOOL_A_PREEMPT_UNSPEC,
+	ETHTOOL_A_PREEMPT_HEADER,			/* nest - _A_HEADER_* */
+	ETHTOOL_A_PREEMPT_SUPPORTED,			/* u8 */
+	ETHTOOL_A_PREEMPT_ACTIVE,			/* u8 */
+	ETHTOOL_A_PREEMPT_MIN_FRAG_SIZE,		/* u32 */
+	ETHTOOL_A_PREEMPT_QUEUES_SUPPORTED,		/* u32 */
+	ETHTOOL_A_PREEMPT_QUEUES_PREEMPTIBLE,		/* u32 */
+
+	/* add new constants above here */
+	__ETHTOOL_A_PREEMPT_CNT,
+	ETHTOOL_A_PREEMPT_MAX = (__ETHTOOL_A_PREEMPT_CNT - 1)
+};
+
 /* generic netlink info */
 #define ETHTOOL_GENL_NAME "ethtool"
 #define ETHTOOL_GENL_VERSION 1
diff --git a/net/ethtool/Makefile b/net/ethtool/Makefile
index 0c2b94f..11661d1 100644
--- a/net/ethtool/Makefile
+++ b/net/ethtool/Makefile
@@ -6,4 +6,5 @@ obj-$(CONFIG_ETHTOOL_NETLINK)	+= ethtool_nl.o
 
 ethtool_nl-y	:= netlink.o bitset.o strset.o linkinfo.o linkmodes.o \
 		   linkstate.o debug.o wol.o features.o privflags.o rings.o \
-		   channels.o coalesce.o pause.o eee.o tsinfo.o cabletest.o
+		   channels.o coalesce.o pause.o eee.o tsinfo.o cabletest.o \
+		   preempt.o
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index 87bc02d..308653f 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -231,6 +231,7 @@ ethnl_default_requests[__ETHTOOL_MSG_USER_CNT] = {
 	[ETHTOOL_MSG_PAUSE_GET]		= &ethnl_pause_request_ops,
 	[ETHTOOL_MSG_EEE_GET]		= &ethnl_eee_request_ops,
 	[ETHTOOL_MSG_TSINFO_GET]	= &ethnl_tsinfo_request_ops,
+	[ETHTOOL_MSG_PREEMPT_GET]	= &ethnl_preempt_request_ops,
 };
 
 static struct ethnl_dump_ctx *ethnl_dump_context(struct netlink_callback *cb)
@@ -550,6 +551,7 @@ ethnl_default_notify_ops[ETHTOOL_MSG_KERNEL_MAX + 1] = {
 	[ETHTOOL_MSG_COALESCE_NTF]	= &ethnl_coalesce_request_ops,
 	[ETHTOOL_MSG_PAUSE_NTF]		= &ethnl_pause_request_ops,
 	[ETHTOOL_MSG_EEE_NTF]		= &ethnl_eee_request_ops,
+	[ETHTOOL_MSG_PREEMPT_NTF]	= &ethnl_preempt_request_ops,
 };
 
 /* default notification handler */
@@ -642,6 +644,7 @@ static const ethnl_notify_handler_t ethnl_notify_handlers[] = {
 	[ETHTOOL_MSG_COALESCE_NTF]	= ethnl_default_notify,
 	[ETHTOOL_MSG_PAUSE_NTF]		= ethnl_default_notify,
 	[ETHTOOL_MSG_EEE_NTF]		= ethnl_default_notify,
+	[ETHTOOL_MSG_PREEMPT_NTF]	= ethnl_default_notify,
 };
 
 void ethtool_notify(struct net_device *dev, unsigned int cmd, const void *data)
@@ -844,6 +847,18 @@ static const struct genl_ops ethtool_genl_ops[] = {
 		.flags	= GENL_UNS_ADMIN_PERM,
 		.doit	= ethnl_act_cable_test,
 	},
+	{
+		.cmd	= ETHTOOL_MSG_PREEMPT_GET,
+		.doit	= ethnl_default_doit,
+		.start	= ethnl_default_start,
+		.dumpit	= ethnl_default_dumpit,
+		.done	= ethnl_default_done,
+	},
+	{
+		.cmd	= ETHTOOL_MSG_PREEMPT_SET,
+		.flags	= GENL_UNS_ADMIN_PERM,
+		.doit	= ethnl_set_preempt,
+	},
 };
 
 static const struct genl_multicast_group ethtool_nl_mcgrps[] = {
diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index b0eb5d9..d22dac6 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -347,6 +347,7 @@ extern const struct ethnl_request_ops ethnl_coalesce_request_ops;
 extern const struct ethnl_request_ops ethnl_pause_request_ops;
 extern const struct ethnl_request_ops ethnl_eee_request_ops;
 extern const struct ethnl_request_ops ethnl_tsinfo_request_ops;
+extern const struct ethnl_request_ops ethnl_preempt_request_ops;
 
 int ethnl_set_linkinfo(struct sk_buff *skb, struct genl_info *info);
 int ethnl_set_linkmodes(struct sk_buff *skb, struct genl_info *info);
@@ -360,5 +361,6 @@ int ethnl_set_coalesce(struct sk_buff *skb, struct genl_info *info);
 int ethnl_set_pause(struct sk_buff *skb, struct genl_info *info);
 int ethnl_set_eee(struct sk_buff *skb, struct genl_info *info);
 int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info);
+int ethnl_set_preempt(struct sk_buff *skb, struct genl_info *info);
 
 #endif /* _NET_ETHTOOL_NETLINK_H */
diff --git a/net/ethtool/preempt.c b/net/ethtool/preempt.c
new file mode 100644
index 0000000..9c8df6f
--- /dev/null
+++ b/net/ethtool/preempt.c
@@ -0,0 +1,181 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include "netlink.h"
+#include "common.h"
+
+struct preempt_req_info {
+	struct ethnl_req_info		base;
+};
+
+struct preempt_reply_data {
+	struct ethnl_reply_data		base;
+	struct ethtool_fp		fp;
+};
+
+#define PREEMPT_REPDATA(__reply_base) \
+	container_of(__reply_base, struct preempt_reply_data, base)
+
+static const struct nla_policy
+preempt_get_policy[ETHTOOL_A_PREEMPT_MAX + 1] = {
+	[ETHTOOL_A_PREEMPT_UNSPEC]		= { .type = NLA_REJECT },
+	[ETHTOOL_A_PREEMPT_HEADER]		= { .type = NLA_NESTED },
+	[ETHTOOL_A_PREEMPT_SUPPORTED]		= { .type = NLA_REJECT },
+	[ETHTOOL_A_PREEMPT_ACTIVE]		= { .type = NLA_REJECT },
+	[ETHTOOL_A_PREEMPT_MIN_FRAG_SIZE]	= { .type = NLA_REJECT },
+	[ETHTOOL_A_PREEMPT_QUEUES_SUPPORTED]	= { .type = NLA_REJECT },
+	[ETHTOOL_A_PREEMPT_QUEUES_PREEMPTIBLE]	= { .type = NLA_REJECT },
+};
+
+static int preempt_prepare_data(const struct ethnl_req_info *req_base,
+				struct ethnl_reply_data *reply_base,
+				struct genl_info *info)
+{
+	struct preempt_reply_data *data = PREEMPT_REPDATA(reply_base);
+	struct net_device *dev = reply_base->dev;
+	int ret;
+
+	if (!dev->ethtool_ops->get_preempt)
+		return -EOPNOTSUPP;
+
+	ret = ethnl_ops_begin(dev);
+	if (ret < 0)
+		return ret;
+
+	ret = dev->ethtool_ops->get_preempt(dev, &data->fp);
+	ethnl_ops_complete(dev);
+
+	return ret;
+}
+
+static int preempt_reply_size(const struct ethnl_req_info *req_base,
+			      const struct ethnl_reply_data *reply_base)
+{
+	int len = 0;
+
+	len += nla_total_size(sizeof(u8)); /* _PREEMPT_SUPPORTED */
+	len += nla_total_size(sizeof(u8)); /* _PREEMPT_ACTIVE */
+	len += nla_total_size(sizeof(u32)); /* _PREEMPT_QUEUES_SUPPORTED */
+	len += nla_total_size(sizeof(u32)); /* _PREEMPT_QUEUES_PREEMPTIBLE */
+	len += nla_total_size(sizeof(u32)); /* _PREEMPT_MIN_FRAG_SIZE */
+
+	return len;
+}
+
+static int preempt_fill_reply(struct sk_buff *skb,
+			      const struct ethnl_req_info *req_base,
+			      const struct ethnl_reply_data *reply_base)
+{
+	const struct preempt_reply_data *data = PREEMPT_REPDATA(reply_base);
+	const struct ethtool_fp *preempt = &data->fp;
+
+	if (nla_put_u32(skb, ETHTOOL_A_PREEMPT_QUEUES_SUPPORTED,
+			  preempt->supported_queues_mask))
+		return -EMSGSIZE;
+
+	if (nla_put_u32(skb, ETHTOOL_A_PREEMPT_QUEUES_PREEMPTIBLE,
+			  preempt->preemptible_queues_mask))
+		return -EMSGSIZE;
+
+	if (nla_put_u8(skb, ETHTOOL_A_PREEMPT_ACTIVE, preempt->fp_enabled))
+		return -EMSGSIZE;
+
+	if (nla_put_u8(skb, ETHTOOL_A_PREEMPT_SUPPORTED,
+		       preempt->fp_supported))
+		return -EMSGSIZE;
+
+	if (nla_put_u32(skb, ETHTOOL_A_PREEMPT_MIN_FRAG_SIZE,
+			preempt->min_frag_size))
+		return -EMSGSIZE;
+
+	return 0;
+}
+
+const struct ethnl_request_ops ethnl_preempt_request_ops = {
+	.request_cmd		= ETHTOOL_MSG_PREEMPT_GET,
+	.reply_cmd		= ETHTOOL_MSG_PREEMPT_GET_REPLY,
+	.hdr_attr		= ETHTOOL_A_PREEMPT_HEADER,
+	.max_attr		= ETHTOOL_A_PREEMPT_MAX,
+	.req_info_size		= sizeof(struct preempt_req_info),
+	.reply_data_size	= sizeof(struct preempt_reply_data),
+	.request_policy		= preempt_get_policy,
+
+	.prepare_data		= preempt_prepare_data,
+	.reply_size		= preempt_reply_size,
+	.fill_reply		= preempt_fill_reply,
+};
+
+static const struct nla_policy
+preempt_set_policy[ETHTOOL_A_PREEMPT_MAX + 1] = {
+	[ETHTOOL_A_PREEMPT_UNSPEC]			= { .type = NLA_REJECT },
+	[ETHTOOL_A_PREEMPT_HEADER]			= { .type = NLA_NESTED },
+	[ETHTOOL_A_PREEMPT_SUPPORTED]			= { .type = NLA_REJECT },
+	[ETHTOOL_A_PREEMPT_ACTIVE]			= { .type = NLA_U8 },
+	[ETHTOOL_A_PREEMPT_MIN_FRAG_SIZE]		= { .type = NLA_U32 },
+	[ETHTOOL_A_PREEMPT_QUEUES_SUPPORTED]		= { .type = NLA_REJECT },
+	[ETHTOOL_A_PREEMPT_QUEUES_PREEMPTIBLE]		= { .type = NLA_U32 },
+};
+
+int ethnl_set_preempt(struct sk_buff *skb, struct genl_info *info)
+{
+	struct nlattr *tb[ETHTOOL_A_LINKINFO_MAX + 1];
+	struct ethtool_fp preempt = {};
+	struct ethnl_req_info req_info = {};
+	struct net_device *dev;
+	bool mod = false;
+	int ret;
+
+	ret = nlmsg_parse(info->nlhdr, GENL_HDRLEN, tb,
+			  ETHTOOL_A_PREEMPT_MAX, preempt_set_policy,
+			  info->extack);
+	if (ret < 0)
+		return ret;
+
+	ret = ethnl_parse_header_dev_get(&req_info,
+					 tb[ETHTOOL_A_PREEMPT_HEADER],
+					 genl_info_net(info), info->extack,
+					 true);
+	if (ret < 0)
+		return ret;
+	dev = req_info.dev;
+	ret = -EOPNOTSUPP;
+	if (!dev->ethtool_ops->get_preempt ||
+	    !dev->ethtool_ops->set_preempt)
+		goto out_dev;
+
+	rtnl_lock();
+	ret = ethnl_ops_begin(dev);
+	if (ret < 0)
+		goto out_rtnl;
+
+	ret = dev->ethtool_ops->get_preempt(dev, &preempt);
+	if (ret < 0) {
+		if (info)
+			GENL_SET_ERR_MSG(info, "failed to retrieve frame preemption settings");
+		goto out_ops;
+	}
+
+	ethnl_update_u8(&preempt.fp_enabled,
+			tb[ETHTOOL_A_PREEMPT_ACTIVE], &mod);
+	ethnl_update_u32(&preempt.min_frag_size,
+			 tb[ETHTOOL_A_PREEMPT_MIN_FRAG_SIZE], &mod);
+	ethnl_update_u32(&preempt.preemptible_queues_mask,
+			 tb[ETHTOOL_A_PREEMPT_QUEUES_PREEMPTIBLE], &mod);
+
+	ret = 0;
+	if (!mod)
+		goto out_ops;
+
+	ret = dev->ethtool_ops->set_preempt(dev, &preempt);
+	if (ret < 0)
+		GENL_SET_ERR_MSG(info, "frame preemption settings update failed");
+	else
+		ethtool_notify(dev, ETHTOOL_MSG_PREEMPT_NTF, NULL);
+
+out_ops:
+	ethnl_ops_complete(dev);
+out_rtnl:
+	rtnl_unlock();
+out_dev:
+	dev_put(dev);
+	return ret;
+}
-- 
2.26.2

[Intel-wired-lan] [next-queue RFC 2/4] ethtool: Add support for configuring frame preemption via netlink

[Vinicius Costa Gomes]

ethtool is gaining support for using netlink as transport for its
messages, being an alternative to ioctl() calls.

Frame preemption, being new, makes a good target for being added to
the list of features that are also supported via the netlink
transport.

Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
 include/uapi/linux/ethtool_netlink.h |  19 +++
 net/ethtool/Makefile                 |   3 +-
 net/ethtool/netlink.c                |  15 +++
 net/ethtool/netlink.h                |   2 +
 net/ethtool/preempt.c                | 181 +++++++++++++++++++++++++++
 5 files changed, 219 insertions(+), 1 deletion(-)
 create mode 100644 net/ethtool/preempt.c

diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index 2881af4..21afba1 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -40,6 +40,8 @@ enum {
 	ETHTOOL_MSG_EEE_SET,
 	ETHTOOL_MSG_TSINFO_GET,
 	ETHTOOL_MSG_CABLE_TEST_ACT,
+	ETHTOOL_MSG_PREEMPT_GET,
+	ETHTOOL_MSG_PREEMPT_SET,
 
 	/* add new constants above here */
 	__ETHTOOL_MSG_USER_CNT,
@@ -76,6 +78,8 @@ enum {
 	ETHTOOL_MSG_EEE_NTF,
 	ETHTOOL_MSG_TSINFO_GET_REPLY,
 	ETHTOOL_MSG_CABLE_TEST_NTF,
+	ETHTOOL_MSG_PREEMPT_GET_REPLY,
+	ETHTOOL_MSG_PREEMPT_NTF,
 
 	/* add new constants above here */
 	__ETHTOOL_MSG_KERNEL_CNT,
@@ -476,6 +480,21 @@ enum {
 	ETHTOOL_A_CABLE_TEST_NTF_MAX = (__ETHTOOL_A_CABLE_TEST_NTF_CNT - 1)
 };
 
+/* FRAME PREEMPTION */
+enum {
+	ETHTOOL_A_PREEMPT_UNSPEC,
+	ETHTOOL_A_PREEMPT_HEADER,			/* nest - _A_HEADER_* */
+	ETHTOOL_A_PREEMPT_SUPPORTED,			/* u8 */
+	ETHTOOL_A_PREEMPT_ACTIVE,			/* u8 */
+	ETHTOOL_A_PREEMPT_MIN_FRAG_SIZE,		/* u32 */
+	ETHTOOL_A_PREEMPT_QUEUES_SUPPORTED,		/* u32 */
+	ETHTOOL_A_PREEMPT_QUEUES_PREEMPTIBLE,		/* u32 */
+
+	/* add new constants above here */
+	__ETHTOOL_A_PREEMPT_CNT,
+	ETHTOOL_A_PREEMPT_MAX = (__ETHTOOL_A_PREEMPT_CNT - 1)
+};
+
 /* generic netlink info */
 #define ETHTOOL_GENL_NAME "ethtool"
 #define ETHTOOL_GENL_VERSION 1
diff --git a/net/ethtool/Makefile b/net/ethtool/Makefile
index 0c2b94f..11661d1 100644
--- a/net/ethtool/Makefile
+++ b/net/ethtool/Makefile
@@ -6,4 +6,5 @@ obj-$(CONFIG_ETHTOOL_NETLINK)	+= ethtool_nl.o
 
 ethtool_nl-y	:= netlink.o bitset.o strset.o linkinfo.o linkmodes.o \
 		   linkstate.o debug.o wol.o features.o privflags.o rings.o \
-		   channels.o coalesce.o pause.o eee.o tsinfo.o cabletest.o
+		   channels.o coalesce.o pause.o eee.o tsinfo.o cabletest.o \
+		   preempt.o
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index 87bc02d..308653f 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -231,6 +231,7 @@ ethnl_default_requests[__ETHTOOL_MSG_USER_CNT] = {
 	[ETHTOOL_MSG_PAUSE_GET]		= &ethnl_pause_request_ops,
 	[ETHTOOL_MSG_EEE_GET]		= &ethnl_eee_request_ops,
 	[ETHTOOL_MSG_TSINFO_GET]	= &ethnl_tsinfo_request_ops,
+	[ETHTOOL_MSG_PREEMPT_GET]	= &ethnl_preempt_request_ops,
 };
 
 static struct ethnl_dump_ctx *ethnl_dump_context(struct netlink_callback *cb)
@@ -550,6 +551,7 @@ ethnl_default_notify_ops[ETHTOOL_MSG_KERNEL_MAX + 1] = {
 	[ETHTOOL_MSG_COALESCE_NTF]	= &ethnl_coalesce_request_ops,
 	[ETHTOOL_MSG_PAUSE_NTF]		= &ethnl_pause_request_ops,
 	[ETHTOOL_MSG_EEE_NTF]		= &ethnl_eee_request_ops,
+	[ETHTOOL_MSG_PREEMPT_NTF]	= &ethnl_preempt_request_ops,
 };
 
 /* default notification handler */
@@ -642,6 +644,7 @@ static const ethnl_notify_handler_t ethnl_notify_handlers[] = {
 	[ETHTOOL_MSG_COALESCE_NTF]	= ethnl_default_notify,
 	[ETHTOOL_MSG_PAUSE_NTF]		= ethnl_default_notify,
 	[ETHTOOL_MSG_EEE_NTF]		= ethnl_default_notify,
+	[ETHTOOL_MSG_PREEMPT_NTF]	= ethnl_default_notify,
 };
 
 void ethtool_notify(struct net_device *dev, unsigned int cmd, const void *data)
@@ -844,6 +847,18 @@ static const struct genl_ops ethtool_genl_ops[] = {
 		.flags	= GENL_UNS_ADMIN_PERM,
 		.doit	= ethnl_act_cable_test,
 	},
+	{
+		.cmd	= ETHTOOL_MSG_PREEMPT_GET,
+		.doit	= ethnl_default_doit,
+		.start	= ethnl_default_start,
+		.dumpit	= ethnl_default_dumpit,
+		.done	= ethnl_default_done,
+	},
+	{
+		.cmd	= ETHTOOL_MSG_PREEMPT_SET,
+		.flags	= GENL_UNS_ADMIN_PERM,
+		.doit	= ethnl_set_preempt,
+	},
 };
 
 static const struct genl_multicast_group ethtool_nl_mcgrps[] = {
diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index b0eb5d9..d22dac6 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -347,6 +347,7 @@ extern const struct ethnl_request_ops ethnl_coalesce_request_ops;
 extern const struct ethnl_request_ops ethnl_pause_request_ops;
 extern const struct ethnl_request_ops ethnl_eee_request_ops;
 extern const struct ethnl_request_ops ethnl_tsinfo_request_ops;
+extern const struct ethnl_request_ops ethnl_preempt_request_ops;
 
 int ethnl_set_linkinfo(struct sk_buff *skb, struct genl_info *info);
 int ethnl_set_linkmodes(struct sk_buff *skb, struct genl_info *info);
@@ -360,5 +361,6 @@ int ethnl_set_coalesce(struct sk_buff *skb, struct genl_info *info);
 int ethnl_set_pause(struct sk_buff *skb, struct genl_info *info);
 int ethnl_set_eee(struct sk_buff *skb, struct genl_info *info);
 int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info);
+int ethnl_set_preempt(struct sk_buff *skb, struct genl_info *info);
 
 #endif /* _NET_ETHTOOL_NETLINK_H */
diff --git a/net/ethtool/preempt.c b/net/ethtool/preempt.c
new file mode 100644
index 0000000..9c8df6f
--- /dev/null
+++ b/net/ethtool/preempt.c
@@ -0,0 +1,181 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include "netlink.h"
+#include "common.h"
+
+struct preempt_req_info {
+	struct ethnl_req_info		base;
+};
+
+struct preempt_reply_data {
+	struct ethnl_reply_data		base;
+	struct ethtool_fp		fp;
+};
+
+#define PREEMPT_REPDATA(__reply_base) \
+	container_of(__reply_base, struct preempt_reply_data, base)
+
+static const struct nla_policy
+preempt_get_policy[ETHTOOL_A_PREEMPT_MAX + 1] = {
+	[ETHTOOL_A_PREEMPT_UNSPEC]		= { .type = NLA_REJECT },
+	[ETHTOOL_A_PREEMPT_HEADER]		= { .type = NLA_NESTED },
+	[ETHTOOL_A_PREEMPT_SUPPORTED]		= { .type = NLA_REJECT },
+	[ETHTOOL_A_PREEMPT_ACTIVE]		= { .type = NLA_REJECT },
+	[ETHTOOL_A_PREEMPT_MIN_FRAG_SIZE]	= { .type = NLA_REJECT },
+	[ETHTOOL_A_PREEMPT_QUEUES_SUPPORTED]	= { .type = NLA_REJECT },
+	[ETHTOOL_A_PREEMPT_QUEUES_PREEMPTIBLE]	= { .type = NLA_REJECT },
+};
+
+static int preempt_prepare_data(const struct ethnl_req_info *req_base,
+				struct ethnl_reply_data *reply_base,
+				struct genl_info *info)
+{
+	struct preempt_reply_data *data = PREEMPT_REPDATA(reply_base);
+	struct net_device *dev = reply_base->dev;
+	int ret;
+
+	if (!dev->ethtool_ops->get_preempt)
+		return -EOPNOTSUPP;
+
+	ret = ethnl_ops_begin(dev);
+	if (ret < 0)
+		return ret;
+
+	ret = dev->ethtool_ops->get_preempt(dev, &data->fp);
+	ethnl_ops_complete(dev);
+
+	return ret;
+}
+
+static int preempt_reply_size(const struct ethnl_req_info *req_base,
+			      const struct ethnl_reply_data *reply_base)
+{
+	int len = 0;
+
+	len += nla_total_size(sizeof(u8)); /* _PREEMPT_SUPPORTED */
+	len += nla_total_size(sizeof(u8)); /* _PREEMPT_ACTIVE */
+	len += nla_total_size(sizeof(u32)); /* _PREEMPT_QUEUES_SUPPORTED */
+	len += nla_total_size(sizeof(u32)); /* _PREEMPT_QUEUES_PREEMPTIBLE */
+	len += nla_total_size(sizeof(u32)); /* _PREEMPT_MIN_FRAG_SIZE */
+
+	return len;
+}
+
+static int preempt_fill_reply(struct sk_buff *skb,
+			      const struct ethnl_req_info *req_base,
+			      const struct ethnl_reply_data *reply_base)
+{
+	const struct preempt_reply_data *data = PREEMPT_REPDATA(reply_base);
+	const struct ethtool_fp *preempt = &data->fp;
+
+	if (nla_put_u32(skb, ETHTOOL_A_PREEMPT_QUEUES_SUPPORTED,
+			  preempt->supported_queues_mask))
+		return -EMSGSIZE;
+
+	if (nla_put_u32(skb, ETHTOOL_A_PREEMPT_QUEUES_PREEMPTIBLE,
+			  preempt->preemptible_queues_mask))
+		return -EMSGSIZE;
+
+	if (nla_put_u8(skb, ETHTOOL_A_PREEMPT_ACTIVE, preempt->fp_enabled))
+		return -EMSGSIZE;
+
+	if (nla_put_u8(skb, ETHTOOL_A_PREEMPT_SUPPORTED,
+		       preempt->fp_supported))
+		return -EMSGSIZE;
+
+	if (nla_put_u32(skb, ETHTOOL_A_PREEMPT_MIN_FRAG_SIZE,
+			preempt->min_frag_size))
+		return -EMSGSIZE;
+
+	return 0;
+}
+
+const struct ethnl_request_ops ethnl_preempt_request_ops = {
+	.request_cmd		= ETHTOOL_MSG_PREEMPT_GET,
+	.reply_cmd		= ETHTOOL_MSG_PREEMPT_GET_REPLY,
+	.hdr_attr		= ETHTOOL_A_PREEMPT_HEADER,
+	.max_attr		= ETHTOOL_A_PREEMPT_MAX,
+	.req_info_size		= sizeof(struct preempt_req_info),
+	.reply_data_size	= sizeof(struct preempt_reply_data),
+	.request_policy		= preempt_get_policy,
+
+	.prepare_data		= preempt_prepare_data,
+	.reply_size		= preempt_reply_size,
+	.fill_reply		= preempt_fill_reply,
+};
+
+static const struct nla_policy
+preempt_set_policy[ETHTOOL_A_PREEMPT_MAX + 1] = {
+	[ETHTOOL_A_PREEMPT_UNSPEC]			= { .type = NLA_REJECT },
+	[ETHTOOL_A_PREEMPT_HEADER]			= { .type = NLA_NESTED },
+	[ETHTOOL_A_PREEMPT_SUPPORTED]			= { .type = NLA_REJECT },
+	[ETHTOOL_A_PREEMPT_ACTIVE]			= { .type = NLA_U8 },
+	[ETHTOOL_A_PREEMPT_MIN_FRAG_SIZE]		= { .type = NLA_U32 },
+	[ETHTOOL_A_PREEMPT_QUEUES_SUPPORTED]		= { .type = NLA_REJECT },
+	[ETHTOOL_A_PREEMPT_QUEUES_PREEMPTIBLE]		= { .type = NLA_U32 },
+};
+
+int ethnl_set_preempt(struct sk_buff *skb, struct genl_info *info)
+{
+	struct nlattr *tb[ETHTOOL_A_LINKINFO_MAX + 1];
+	struct ethtool_fp preempt = {};
+	struct ethnl_req_info req_info = {};
+	struct net_device *dev;
+	bool mod = false;
+	int ret;
+
+	ret = nlmsg_parse(info->nlhdr, GENL_HDRLEN, tb,
+			  ETHTOOL_A_PREEMPT_MAX, preempt_set_policy,
+			  info->extack);
+	if (ret < 0)
+		return ret;
+
+	ret = ethnl_parse_header_dev_get(&req_info,
+					 tb[ETHTOOL_A_PREEMPT_HEADER],
+					 genl_info_net(info), info->extack,
+					 true);
+	if (ret < 0)
+		return ret;
+	dev = req_info.dev;
+	ret = -EOPNOTSUPP;
+	if (!dev->ethtool_ops->get_preempt ||
+	    !dev->ethtool_ops->set_preempt)
+		goto out_dev;
+
+	rtnl_lock();
+	ret = ethnl_ops_begin(dev);
+	if (ret < 0)
+		goto out_rtnl;
+
+	ret = dev->ethtool_ops->get_preempt(dev, &preempt);
+	if (ret < 0) {
+		if (info)
+			GENL_SET_ERR_MSG(info, "failed to retrieve frame preemption settings");
+		goto out_ops;
+	}
+
+	ethnl_update_u8(&preempt.fp_enabled,
+			tb[ETHTOOL_A_PREEMPT_ACTIVE], &mod);
+	ethnl_update_u32(&preempt.min_frag_size,
+			 tb[ETHTOOL_A_PREEMPT_MIN_FRAG_SIZE], &mod);
+	ethnl_update_u32(&preempt.preemptible_queues_mask,
+			 tb[ETHTOOL_A_PREEMPT_QUEUES_PREEMPTIBLE], &mod);
+
+	ret = 0;
+	if (!mod)
+		goto out_ops;
+
+	ret = dev->ethtool_ops->set_preempt(dev, &preempt);
+	if (ret < 0)
+		GENL_SET_ERR_MSG(info, "frame preemption settings update failed");
+	else
+		ethtool_notify(dev, ETHTOOL_MSG_PREEMPT_NTF, NULL);
+
+out_ops:
+	ethnl_ops_complete(dev);
+out_rtnl:
+	rtnl_unlock();
+out_dev:
+	dev_put(dev);
+	return ret;
+}
-- 
2.26.2

[next-queue RFC 3/4] igc: Add support for configuring frame preemption

[Vinicius Costa Gomes]

WIP

Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
 drivers/net/ethernet/intel/igc/igc.h         |  3 +
 drivers/net/ethernet/intel/igc/igc_defines.h |  6 ++
 drivers/net/ethernet/intel/igc/igc_ethtool.c | 68 ++++++++++++++++++++
 drivers/net/ethernet/intel/igc/igc_tsn.c     | 46 +++++++++++--
 4 files changed, 118 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
index 5dbc5a1..963ac98 100644
--- a/drivers/net/ethernet/intel/igc/igc.h
+++ b/drivers/net/ethernet/intel/igc/igc.h
@@ -87,6 +87,7 @@ struct igc_ring {
 	u8 queue_index;                 /* logical index of the ring*/
 	u8 reg_idx;                     /* physical index of the ring */
 	bool launchtime_enable;         /* true if LaunchTime is enabled */
+	bool preemptible;		/* true if not express */
 
 	u32 start_time;
 	u32 end_time;
@@ -162,6 +163,8 @@ struct igc_adapter {
 
 	ktime_t base_time;
 	ktime_t cycle_time;
+	bool frame_preemption_active;
+	u32 min_frag_size; /* Frame preemption minimum fragment size */
 
 	/* OS defined structs */
 	struct pci_dev *pdev;
diff --git a/drivers/net/ethernet/intel/igc/igc_defines.h b/drivers/net/ethernet/intel/igc/igc_defines.h
index 80b664e..fa823c3 100644
--- a/drivers/net/ethernet/intel/igc/igc_defines.h
+++ b/drivers/net/ethernet/intel/igc/igc_defines.h
@@ -364,6 +364,8 @@
 #define IGC_RXPBS_CFG_TS_EN	0x80000000 /* Timestamp in Rx buffer */
 
 #define IGC_TXPBSIZE_TSN	0x04145145 /* 5k bytes buffer for each queue */
+#define IGC_RXPBSIZE_TSN	0x00010090 /* 16KB for EXP + 16KB for BE + 2KB for BMC */
+#define IGC_RXPBSIZE_SIZE_MASK	0x0001FFFF
 
 #define IGC_DTXMXPKTSZ_TSN	0x19 /* 1600 bytes of max TX DMA packet size */
 #define IGC_DTXMXPKTSZ_DEFAULT	0x98 /* 9728-byte Jumbo frames */
@@ -422,10 +424,14 @@
 /* Transmit Scheduling */
 #define IGC_TQAVCTRL_TRANSMIT_MODE_TSN	0x00000001
 #define IGC_TQAVCTRL_ENHANCED_QAV	0x00000008
+#define IGC_TQAVCTRL_PREEMPT_ENA	0x00000002
+#define IGC_TQAVCTRL_MIN_FRAG_MASK	0x0000C000
+#define IGC_TQAVCTRL_MIN_FRAG_SHIFT	14
 
 #define IGC_TXQCTL_QUEUE_MODE_LAUNCHT	0x00000001
 #define IGC_TXQCTL_STRICT_CYCLE		0x00000002
 #define IGC_TXQCTL_STRICT_END		0x00000004
+#define IGC_TXQCTL_PREEMPTABLE		0x00000008
 
 /* Receive Checksum Control */
 #define IGC_RXCSUM_CRCOFL	0x00000800   /* CRC32 offload enable */
diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
index 2214a5d..48d5d18 100644
--- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
+++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
@@ -7,6 +7,7 @@
 
 #include "igc.h"
 #include "igc_diag.h"
+#include "igc_tsn.h"
 
 /* forward declaration */
 struct igc_stats {
@@ -1549,6 +1550,71 @@ static int igc_ethtool_set_priv_flags(struct net_device *netdev, u32 priv_flags)
 	return 0;
 }
 
+static int igc_ethtool_get_preempt(struct net_device *netdev,
+				   struct ethtool_fp *fpcmd)
+{
+	struct igc_adapter *adapter = netdev_priv(netdev);
+	int i;
+
+	fpcmd->fp_supported = 1;
+	fpcmd->fp_enabled = adapter->frame_preemption_active;
+	fpcmd->min_frag_size = adapter->min_frag_size;
+
+	for (i = 0; i < adapter->num_tx_queues; i++) {
+		struct igc_ring *ring = adapter->tx_ring[i];
+
+		fpcmd->supported_queues_mask |= BIT(i);
+
+		if (ring->preemptible)
+			fpcmd->preemptible_queues_mask |= BIT(i);
+	}
+
+	return 0;
+}
+
+static int igc_ethtool_set_preempt(struct net_device *netdev,
+				   struct ethtool_fp *fpcmd)
+{
+	struct igc_adapter *adapter = netdev_priv(netdev);
+	int i;
+
+	/* The formula is (Section 8.12.4 of the datasheet):
+	 *   MIN_FRAG_SIZE = 4 + (1 + MIN_FRAG)*64
+	 * MIN_FRAG is represented by two bits, so we can only have
+	 * min_frag_size between 68 and 260.
+	 */
+	if (fpcmd->min_frag_size < 68 || fpcmd->min_frag_size > 260)
+		return -EINVAL;
+
+	fpcmd->fp_supported = 1;
+
+	adapter->frame_preemption_active = fpcmd->fp_enabled;
+	adapter->min_frag_size = fpcmd->min_frag_size;
+
+	/* We need to setup a dummy Qbv cycle for frame preemption to
+	 * work, but we only need to set it up if none is set. This
+	 * same check below exists for the same purpose.
+	 */
+	if (!adapter->base_time)
+		adapter->cycle_time = NSEC_PER_SEC;
+
+	for (i = 0; i < adapter->num_tx_queues; i++) {
+		struct igc_ring *ring = adapter->tx_ring[i];
+		bool preemptible = fpcmd->preemptible_queues_mask & BIT(i);
+
+		fpcmd->fp_supported |= BIT(i);
+		ring->preemptible = preemptible;
+
+		if (adapter->base_time)
+			continue;
+
+		ring->start_time = 0;
+		ring->end_time = NSEC_PER_SEC;
+	}
+
+	return igc_tsn_offload_apply(adapter);
+}
+
 static int igc_ethtool_begin(struct net_device *netdev)
 {
 	struct igc_adapter *adapter = netdev_priv(netdev);
@@ -1828,6 +1894,8 @@ static const struct ethtool_ops igc_ethtool_ops = {
 	.get_ts_info		= igc_ethtool_get_ts_info,
 	.get_channels		= igc_ethtool_get_channels,
 	.set_channels		= igc_ethtool_set_channels,
+	.get_preempt		= igc_ethtool_get_preempt,
+	.set_preempt		= igc_ethtool_set_preempt,
 	.get_priv_flags		= igc_ethtool_get_priv_flags,
 	.set_priv_flags		= igc_ethtool_set_priv_flags,
 	.begin			= igc_ethtool_begin,
diff --git a/drivers/net/ethernet/intel/igc/igc_tsn.c b/drivers/net/ethernet/intel/igc/igc_tsn.c
index 174103c..1bebe1c 100644
--- a/drivers/net/ethernet/intel/igc/igc_tsn.c
+++ b/drivers/net/ethernet/intel/igc/igc_tsn.c
@@ -18,26 +18,45 @@ static bool is_any_launchtime(struct igc_adapter *adapter)
 	return false;
 }
 
+static u32 igc_min_frag_size_to_tqavctrl(u32 min_frag)
+{
+	u32 tqavctrl;
+
+	tqavctrl = DIV_ROUND_UP((min_frag - 4), 64);
+	tqavctrl -= 1;
+
+	tqavctrl <<= IGC_TQAVCTRL_MIN_FRAG_SHIFT;
+
+	return tqavctrl;
+}
+
 /* Returns the TSN specific registers to their default values after
  * TSN offloading is disabled.
  */
 static int igc_tsn_disable_offload(struct igc_adapter *adapter)
 {
 	struct igc_hw *hw = &adapter->hw;
-	u32 tqavctrl;
+	u32 tqavctrl, rxpbs;
 	int i;
 
 	if (!(adapter->flags & IGC_FLAG_TSN_QBV_ENABLED))
 		return 0;
 
 	adapter->cycle_time = 0;
+	adapter->frame_preemption_active = false;
+	adapter->min_frag_size = 68;
 
 	wr32(IGC_TXPBS, I225_TXPBSIZE_DEFAULT);
 	wr32(IGC_DTXMXPKTSZ, IGC_DTXMXPKTSZ_DEFAULT);
 
+	rxpbs = rd32(IGC_RXPBS) & ~IGC_RXPBSIZE_SIZE_MASK;
+	rxpbs |= I225_RXPBSIZE_DEFAULT;
+
+	wr32(IGC_RXPBS, rxpbs);
+
 	tqavctrl = rd32(IGC_TQAVCTRL);
 	tqavctrl &= ~(IGC_TQAVCTRL_TRANSMIT_MODE_TSN |
-		      IGC_TQAVCTRL_ENHANCED_QAV);
+		      IGC_TQAVCTRL_ENHANCED_QAV | IGC_TQAVCTRL_PREEMPT_ENA);
 	wr32(IGC_TQAVCTRL, tqavctrl);
 
 	for (i = 0; i < adapter->num_tx_queues; i++) {
@@ -46,6 +65,7 @@ static int igc_tsn_disable_offload(struct igc_adapter *adapter)
 		ring->start_time = 0;
 		ring->end_time = 0;
 		ring->launchtime_enable = false;
+		ring->preemptible = false;
 
 		wr32(IGC_TXQCTL(i), 0);
 		wr32(IGC_STQT(i), 0);
@@ -64,7 +84,7 @@ static int igc_tsn_enable_offload(struct igc_adapter *adapter)
 {
 	struct igc_hw *hw = &adapter->hw;
 	u32 tqavctrl, baset_l, baset_h;
-	u32 sec, nsec, cycle;
+	u32 sec, nsec, cycle, rxpbs;
 	ktime_t base_time, systim;
 	int i;
 
@@ -78,8 +98,20 @@ static int igc_tsn_enable_offload(struct igc_adapter *adapter)
 	wr32(IGC_DTXMXPKTSZ, IGC_DTXMXPKTSZ_TSN);
 	wr32(IGC_TXPBS, IGC_TXPBSIZE_TSN);
 
-	tqavctrl = rd32(IGC_TQAVCTRL);
+	rxpbs = rd32(IGC_RXPBS) & ~IGC_RXPBSIZE_SIZE_MASK;
+	rxpbs |= IGC_RXPBSIZE_TSN;
+
+	wr32(IGC_RXPBS, rxpbs);
+
+	tqavctrl = rd32(IGC_TQAVCTRL) &
+		~(IGC_TQAVCTRL_MIN_FRAG_MASK | IGC_TQAVCTRL_PREEMPT_ENA);
 	tqavctrl |= IGC_TQAVCTRL_TRANSMIT_MODE_TSN | IGC_TQAVCTRL_ENHANCED_QAV;
+
+	if (adapter->frame_preemption_active)
+		tqavctrl |= IGC_TQAVCTRL_PREEMPT_ENA;
+
+	tqavctrl |= igc_min_frag_size_to_tqavctrl(adapter->min_frag_size);
+
 	wr32(IGC_TQAVCTRL, tqavctrl);
 
 	wr32(IGC_QBVCYCLET_S, cycle);
@@ -105,6 +137,9 @@ static int igc_tsn_enable_offload(struct igc_adapter *adapter)
 		if (ring->launchtime_enable)
 			txqctl |= IGC_TXQCTL_QUEUE_MODE_LAUNCHT;
 
+		if (ring->preemptible)
+			txqctl |= IGC_TXQCTL_PREEMPTABLE;
+
 		wr32(IGC_TXQCTL(i), txqctl);
 	}
 
@@ -132,7 +167,8 @@ static int igc_tsn_enable_offload(struct igc_adapter *adapter)
 
 int igc_tsn_offload_apply(struct igc_adapter *adapter)
 {
-	bool is_any_enabled = adapter->base_time || is_any_launchtime(adapter);
+	bool is_any_enabled = adapter->base_time ||
+		is_any_launchtime(adapter) || adapter->frame_preemption_active;
 
 	if (!(adapter->flags & IGC_FLAG_TSN_QBV_ENABLED) && !is_any_enabled)
 		return 0;
-- 
2.26.2

[Intel-wired-lan] [next-queue RFC 3/4] igc: Add support for configuring frame preemption

[Vinicius Costa Gomes]

WIP

Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
 drivers/net/ethernet/intel/igc/igc.h         |  3 +
 drivers/net/ethernet/intel/igc/igc_defines.h |  6 ++
 drivers/net/ethernet/intel/igc/igc_ethtool.c | 68 ++++++++++++++++++++
 drivers/net/ethernet/intel/igc/igc_tsn.c     | 46 +++++++++++--
 4 files changed, 118 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
index 5dbc5a1..963ac98 100644
--- a/drivers/net/ethernet/intel/igc/igc.h
+++ b/drivers/net/ethernet/intel/igc/igc.h
@@ -87,6 +87,7 @@ struct igc_ring {
 	u8 queue_index;                 /* logical index of the ring*/
 	u8 reg_idx;                     /* physical index of the ring */
 	bool launchtime_enable;         /* true if LaunchTime is enabled */
+	bool preemptible;		/* true if not express */
 
 	u32 start_time;
 	u32 end_time;
@@ -162,6 +163,8 @@ struct igc_adapter {
 
 	ktime_t base_time;
 	ktime_t cycle_time;
+	bool frame_preemption_active;
+	u32 min_frag_size; /* Frame preemption minimum fragment size */
 
 	/* OS defined structs */
 	struct pci_dev *pdev;
diff --git a/drivers/net/ethernet/intel/igc/igc_defines.h b/drivers/net/ethernet/intel/igc/igc_defines.h
index 80b664e..fa823c3 100644
--- a/drivers/net/ethernet/intel/igc/igc_defines.h
+++ b/drivers/net/ethernet/intel/igc/igc_defines.h
@@ -364,6 +364,8 @@
 #define IGC_RXPBS_CFG_TS_EN	0x80000000 /* Timestamp in Rx buffer */
 
 #define IGC_TXPBSIZE_TSN	0x04145145 /* 5k bytes buffer for each queue */
+#define IGC_RXPBSIZE_TSN	0x00010090 /* 16KB for EXP + 16KB for BE + 2KB for BMC */
+#define IGC_RXPBSIZE_SIZE_MASK	0x0001FFFF
 
 #define IGC_DTXMXPKTSZ_TSN	0x19 /* 1600 bytes of max TX DMA packet size */
 #define IGC_DTXMXPKTSZ_DEFAULT	0x98 /* 9728-byte Jumbo frames */
@@ -422,10 +424,14 @@
 /* Transmit Scheduling */
 #define IGC_TQAVCTRL_TRANSMIT_MODE_TSN	0x00000001
 #define IGC_TQAVCTRL_ENHANCED_QAV	0x00000008
+#define IGC_TQAVCTRL_PREEMPT_ENA	0x00000002
+#define IGC_TQAVCTRL_MIN_FRAG_MASK	0x0000C000
+#define IGC_TQAVCTRL_MIN_FRAG_SHIFT	14
 
 #define IGC_TXQCTL_QUEUE_MODE_LAUNCHT	0x00000001
 #define IGC_TXQCTL_STRICT_CYCLE		0x00000002
 #define IGC_TXQCTL_STRICT_END		0x00000004
+#define IGC_TXQCTL_PREEMPTABLE		0x00000008
 
 /* Receive Checksum Control */
 #define IGC_RXCSUM_CRCOFL	0x00000800   /* CRC32 offload enable */
diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
index 2214a5d..48d5d18 100644
--- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
+++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
@@ -7,6 +7,7 @@
 
 #include "igc.h"
 #include "igc_diag.h"
+#include "igc_tsn.h"
 
 /* forward declaration */
 struct igc_stats {
@@ -1549,6 +1550,71 @@ static int igc_ethtool_set_priv_flags(struct net_device *netdev, u32 priv_flags)
 	return 0;
 }
 
+static int igc_ethtool_get_preempt(struct net_device *netdev,
+				   struct ethtool_fp *fpcmd)
+{
+	struct igc_adapter *adapter = netdev_priv(netdev);
+	int i;
+
+	fpcmd->fp_supported = 1;
+	fpcmd->fp_enabled = adapter->frame_preemption_active;
+	fpcmd->min_frag_size = adapter->min_frag_size;
+
+	for (i = 0; i < adapter->num_tx_queues; i++) {
+		struct igc_ring *ring = adapter->tx_ring[i];
+
+		fpcmd->supported_queues_mask |= BIT(i);
+
+		if (ring->preemptible)
+			fpcmd->preemptible_queues_mask |= BIT(i);
+	}
+
+	return 0;
+}
+
+static int igc_ethtool_set_preempt(struct net_device *netdev,
+				   struct ethtool_fp *fpcmd)
+{
+	struct igc_adapter *adapter = netdev_priv(netdev);
+	int i;
+
+	/* The formula is (Section 8.12.4 of the datasheet):
+	 *   MIN_FRAG_SIZE = 4 + (1 + MIN_FRAG)*64
+	 * MIN_FRAG is represented by two bits, so we can only have
+	 * min_frag_size between 68 and 260.
+	 */
+	if (fpcmd->min_frag_size < 68 || fpcmd->min_frag_size > 260)
+		return -EINVAL;
+
+	fpcmd->fp_supported = 1;
+
+	adapter->frame_preemption_active = fpcmd->fp_enabled;
+	adapter->min_frag_size = fpcmd->min_frag_size;
+
+	/* We need to setup a dummy Qbv cycle for frame preemption to
+	 * work, but we only need to set it up if none is set. This
+	 * same check below exists for the same purpose.
+	 */
+	if (!adapter->base_time)
+		adapter->cycle_time = NSEC_PER_SEC;
+
+	for (i = 0; i < adapter->num_tx_queues; i++) {
+		struct igc_ring *ring = adapter->tx_ring[i];
+		bool preemptible = fpcmd->preemptible_queues_mask & BIT(i);
+
+		fpcmd->fp_supported |= BIT(i);
+		ring->preemptible = preemptible;
+
+		if (adapter->base_time)
+			continue;
+
+		ring->start_time = 0;
+		ring->end_time = NSEC_PER_SEC;
+	}
+
+	return igc_tsn_offload_apply(adapter);
+}
+
 static int igc_ethtool_begin(struct net_device *netdev)
 {
 	struct igc_adapter *adapter = netdev_priv(netdev);
@@ -1828,6 +1894,8 @@ static const struct ethtool_ops igc_ethtool_ops = {
 	.get_ts_info		= igc_ethtool_get_ts_info,
 	.get_channels		= igc_ethtool_get_channels,
 	.set_channels		= igc_ethtool_set_channels,
+	.get_preempt		= igc_ethtool_get_preempt,
+	.set_preempt		= igc_ethtool_set_preempt,
 	.get_priv_flags		= igc_ethtool_get_priv_flags,
 	.set_priv_flags		= igc_ethtool_set_priv_flags,
 	.begin			= igc_ethtool_begin,
diff --git a/drivers/net/ethernet/intel/igc/igc_tsn.c b/drivers/net/ethernet/intel/igc/igc_tsn.c
index 174103c..1bebe1c 100644
--- a/drivers/net/ethernet/intel/igc/igc_tsn.c
+++ b/drivers/net/ethernet/intel/igc/igc_tsn.c
@@ -18,26 +18,45 @@ static bool is_any_launchtime(struct igc_adapter *adapter)
 	return false;
 }
 
+static u32 igc_min_frag_size_to_tqavctrl(u32 min_frag)
+{
+	u32 tqavctrl;
+
+	tqavctrl = DIV_ROUND_UP((min_frag - 4), 64);
+	tqavctrl -= 1;
+
+	tqavctrl <<= IGC_TQAVCTRL_MIN_FRAG_SHIFT;
+
+	return tqavctrl;
+}
+
 /* Returns the TSN specific registers to their default values after
  * TSN offloading is disabled.
  */
 static int igc_tsn_disable_offload(struct igc_adapter *adapter)
 {
 	struct igc_hw *hw = &adapter->hw;
-	u32 tqavctrl;
+	u32 tqavctrl, rxpbs;
 	int i;
 
 	if (!(adapter->flags & IGC_FLAG_TSN_QBV_ENABLED))
 		return 0;
 
 	adapter->cycle_time = 0;
+	adapter->frame_preemption_active = false;
+	adapter->min_frag_size = 68;
 
 	wr32(IGC_TXPBS, I225_TXPBSIZE_DEFAULT);
 	wr32(IGC_DTXMXPKTSZ, IGC_DTXMXPKTSZ_DEFAULT);
 
+	rxpbs = rd32(IGC_RXPBS) & ~IGC_RXPBSIZE_SIZE_MASK;
+	rxpbs |= I225_RXPBSIZE_DEFAULT;
+
+	wr32(IGC_RXPBS, rxpbs);
+
 	tqavctrl = rd32(IGC_TQAVCTRL);
 	tqavctrl &= ~(IGC_TQAVCTRL_TRANSMIT_MODE_TSN |
-		      IGC_TQAVCTRL_ENHANCED_QAV);
+		      IGC_TQAVCTRL_ENHANCED_QAV | IGC_TQAVCTRL_PREEMPT_ENA);
 	wr32(IGC_TQAVCTRL, tqavctrl);
 
 	for (i = 0; i < adapter->num_tx_queues; i++) {
@@ -46,6 +65,7 @@ static int igc_tsn_disable_offload(struct igc_adapter *adapter)
 		ring->start_time = 0;
 		ring->end_time = 0;
 		ring->launchtime_enable = false;
+		ring->preemptible = false;
 
 		wr32(IGC_TXQCTL(i), 0);
 		wr32(IGC_STQT(i), 0);
@@ -64,7 +84,7 @@ static int igc_tsn_enable_offload(struct igc_adapter *adapter)
 {
 	struct igc_hw *hw = &adapter->hw;
 	u32 tqavctrl, baset_l, baset_h;
-	u32 sec, nsec, cycle;
+	u32 sec, nsec, cycle, rxpbs;
 	ktime_t base_time, systim;
 	int i;
 
@@ -78,8 +98,20 @@ static int igc_tsn_enable_offload(struct igc_adapter *adapter)
 	wr32(IGC_DTXMXPKTSZ, IGC_DTXMXPKTSZ_TSN);
 	wr32(IGC_TXPBS, IGC_TXPBSIZE_TSN);
 
-	tqavctrl = rd32(IGC_TQAVCTRL);
+	rxpbs = rd32(IGC_RXPBS) & ~IGC_RXPBSIZE_SIZE_MASK;
+	rxpbs |= IGC_RXPBSIZE_TSN;
+
+	wr32(IGC_RXPBS, rxpbs);
+
+	tqavctrl = rd32(IGC_TQAVCTRL) &
+		~(IGC_TQAVCTRL_MIN_FRAG_MASK | IGC_TQAVCTRL_PREEMPT_ENA);
 	tqavctrl |= IGC_TQAVCTRL_TRANSMIT_MODE_TSN | IGC_TQAVCTRL_ENHANCED_QAV;
+
+	if (adapter->frame_preemption_active)
+		tqavctrl |= IGC_TQAVCTRL_PREEMPT_ENA;
+
+	tqavctrl |= igc_min_frag_size_to_tqavctrl(adapter->min_frag_size);
+
 	wr32(IGC_TQAVCTRL, tqavctrl);
 
 	wr32(IGC_QBVCYCLET_S, cycle);
@@ -105,6 +137,9 @@ static int igc_tsn_enable_offload(struct igc_adapter *adapter)
 		if (ring->launchtime_enable)
 			txqctl |= IGC_TXQCTL_QUEUE_MODE_LAUNCHT;
 
+		if (ring->preemptible)
+			txqctl |= IGC_TXQCTL_PREEMPTABLE;
+
 		wr32(IGC_TXQCTL(i), txqctl);
 	}
 
@@ -132,7 +167,8 @@ static int igc_tsn_enable_offload(struct igc_adapter *adapter)
 
 int igc_tsn_offload_apply(struct igc_adapter *adapter)
 {
-	bool is_any_enabled = adapter->base_time || is_any_launchtime(adapter);
+	bool is_any_enabled = adapter->base_time ||
+		is_any_launchtime(adapter) || adapter->frame_preemption_active;
 
 	if (!(adapter->flags & IGC_FLAG_TSN_QBV_ENABLED) && !is_any_enabled)
 		return 0;
-- 
2.26.2

[next-queue RFC 4/4] igc: Add support for exposing frame preemption stats registers

[Vinicius Costa Gomes]

[WIP]

Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
 drivers/net/ethernet/intel/igc/igc_ethtool.c |  9 +++++++++
 drivers/net/ethernet/intel/igc/igc_regs.h    | 10 ++++++++++
 2 files changed, 19 insertions(+)

diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
index 48d5d18..09d72f7 100644
--- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
+++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
@@ -322,6 +322,15 @@ static void igc_ethtool_get_regs(struct net_device *netdev,
 
 	for (i = 0; i < 8; i++)
 		regs_buff[205 + i] = rd32(IGC_ETQF(i));
+
+	regs_buff[214] = rd32(IGC_PRMPTDTCNT);
+	regs_buff[215] = rd32(IGC_PRMEVNTTCNT);
+	regs_buff[216] = rd32(IGC_PRMPTDRCNT);
+	regs_buff[217] = rd32(IGC_PRMEVNTRCNT);
+	regs_buff[218] = rd32(IGC_PRMPBLTCNT);
+	regs_buff[219] = rd32(IGC_PRMPBLRCNT);
+	regs_buff[220] = rd32(IGC_PRMEXPTCNT);
+	regs_buff[221] = rd32(IGC_PRMEXPRCNT);
 }
 
 static void igc_ethtool_get_wol(struct net_device *netdev,
diff --git a/drivers/net/ethernet/intel/igc/igc_regs.h b/drivers/net/ethernet/intel/igc/igc_regs.h
index 7f999cf..010bb48 100644
--- a/drivers/net/ethernet/intel/igc/igc_regs.h
+++ b/drivers/net/ethernet/intel/igc/igc_regs.h
@@ -211,6 +211,16 @@
 
 #define IGC_FTQF(_n)	(0x059E0 + (4 * (_n)))  /* 5-tuple Queue Fltr */
 
+/* Time sync registers - preemption statistics */
+#define IGC_PRMPTDTCNT	0x04280  /* Good TX Preempted Packets */
+#define IGC_PRMEVNTTCNT	0x04298  /* TX Preemption event counter */
+#define IGC_PRMPTDRCNT	0x04284  /* Good RX Preempted Packets */
+#define IGC_PRMEVNTRCNT	0x0429C  /* RX Preemption event counter */
+#define IGC_PRMPBLTCNT	0x04288  /* Good TX Preemptable Packets */
+#define IGC_PRMPBLRCNT	0x0428C  /* Good RX Preemptable Packets */
+#define IGC_PRMEXPTCNT	0x04290  /* Good TX Express Packets */
+#define IGC_PRMEXPRCNT	0x042A0  /* Preemption Exception Counter */
+
 /* Transmit Scheduling Registers */
 #define IGC_TQAVCTRL		0x3570
 #define IGC_TXQCTL(_n)		(0x3344 + 0x4 * (_n))
-- 
2.26.2

[Intel-wired-lan] [next-queue RFC 4/4] igc: Add support for exposing frame preemption stats registers

[Vinicius Costa Gomes]

[WIP]

Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
 drivers/net/ethernet/intel/igc/igc_ethtool.c |  9 +++++++++
 drivers/net/ethernet/intel/igc/igc_regs.h    | 10 ++++++++++
 2 files changed, 19 insertions(+)

diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
index 48d5d18..09d72f7 100644
--- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
+++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
@@ -322,6 +322,15 @@ static void igc_ethtool_get_regs(struct net_device *netdev,
 
 	for (i = 0; i < 8; i++)
 		regs_buff[205 + i] = rd32(IGC_ETQF(i));
+
+	regs_buff[214] = rd32(IGC_PRMPTDTCNT);
+	regs_buff[215] = rd32(IGC_PRMEVNTTCNT);
+	regs_buff[216] = rd32(IGC_PRMPTDRCNT);
+	regs_buff[217] = rd32(IGC_PRMEVNTRCNT);
+	regs_buff[218] = rd32(IGC_PRMPBLTCNT);
+	regs_buff[219] = rd32(IGC_PRMPBLRCNT);
+	regs_buff[220] = rd32(IGC_PRMEXPTCNT);
+	regs_buff[221] = rd32(IGC_PRMEXPRCNT);
 }
 
 static void igc_ethtool_get_wol(struct net_device *netdev,
diff --git a/drivers/net/ethernet/intel/igc/igc_regs.h b/drivers/net/ethernet/intel/igc/igc_regs.h
index 7f999cf..010bb48 100644
--- a/drivers/net/ethernet/intel/igc/igc_regs.h
+++ b/drivers/net/ethernet/intel/igc/igc_regs.h
@@ -211,6 +211,16 @@
 
 #define IGC_FTQF(_n)	(0x059E0 + (4 * (_n)))  /* 5-tuple Queue Fltr */
 
+/* Time sync registers - preemption statistics */
+#define IGC_PRMPTDTCNT	0x04280  /* Good TX Preempted Packets */
+#define IGC_PRMEVNTTCNT	0x04298  /* TX Preemption event counter */
+#define IGC_PRMPTDRCNT	0x04284  /* Good RX Preempted Packets */
+#define IGC_PRMEVNTRCNT	0x0429C  /* RX Preemption event counter */
+#define IGC_PRMPBLTCNT	0x04288  /* Good TX Preemptable Packets */
+#define IGC_PRMPBLRCNT	0x0428C  /* Good RX Preemptable Packets */
+#define IGC_PRMEXPTCNT	0x04290  /* Good TX Express Packets */
+#define IGC_PRMEXPRCNT	0x042A0  /* Preemption Exception Counter */
+
 /* Transmit Scheduling Registers */
 #define IGC_TQAVCTRL		0x3570
 #define IGC_TXQCTL(_n)		(0x3344 + 0x4 * (_n))
-- 
2.26.2

Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Michal Kubecek]

On Fri, May 15, 2020 at 06:29:44PM -0700, Vinicius Costa Gomes wrote:
> Hi,
> 
> This series adds support for configuring frame preemption, as defined
> by IEEE 802.1Q-2018 (previously IEEE 802.1Qbu) and IEEE 802.3br.
> 
> Frame preemption allows a packet from a higher priority queue marked
> as "express" to preempt a packet from lower priority queue marked as
> "preemptible". The idea is that this can help reduce the latency for
> higher priority traffic.
> 
> Previously, the proposed interface for configuring these features was
> using the qdisc layer. But as this is very hardware dependent and all
> that qdisc did was pass the information to the driver, it makes sense
> to have this in ethtool.
> 
> One example, for retrieving and setting the configuration:
> 
> $ ethtool $ sudo ./ethtool --show-frame-preemption enp3s0
> Frame preemption settings for enp3s0:
> 	support: supported

IMHO we don't need a special bool for this. IIUC this is not a state
flag that would change value for a particular device; either the device
supports the feature or it does not. If it does not, the ethtool_ops
callbacks would return -EOPNOTSUPP (or would not even exist if the
driver has no support) and ethtool would say so.

> 	active: active
> 	supported queues: 0xf
> 	supported queues: 0xe
> 	minimum fragment size: 68
> 
> 
> $ ethtool --set-frame-preemption enp3s0 fp on min-frag-size 68 preemptible-queues-mask 0xe
> 
> This is a RFC because I wanted to have feedback on some points:
> 
>   - The parameters added are enough for the hardware I have, is it
>     enough in general?
> 
>   - even with the ethtool via netlink effort, I chose to keep the
>     ioctl() way, in case someone wants to backport this to an older
>     kernel, is there a problem with this?

I would prefer not extending ioctl interface with new features, with
obvious exceptions like adding new link modes or so. Not only because
having new features only available through netlink will motivate authors
of userspace tools to support netlink but mostly because the lack of
flexibility and extensibility of ioctl interface inevitably leads to
compromises you wouldn't have to do if you only implement netlink
requests.

One example I can see is the use of u32 for queue bitmaps. Perhaps you
don't expect this feature to be supported on devices with more than 32
queues (and I don't have enough expertise to tell if it's justified at
the moment) but can you be sure it will be the case in 10 or 20 years?
As long as these hardcoded u32 bitmaps are only part of internal kernel
API (ethtool_ops), extending the support for bigger devices will mean
some code churn (possibly large if many drivers implement the feature)
but it's something that can be done. But if you have this limit in
userspace API, you are in a much bigger trouble. The same can be said
for adding new attributes - easy with netlink but with ioctl you never
know if those reserved fields will suffice.

> 
>   - Some space for bikeshedding the names and location (for example,
>     does it make sense for these settings to be per-queue?), as I am
>     not quite happy with them, one example, is the use of preemptible
>     vs. preemptable;
> 
> 
> About the patches, should be quite straightforward:
> 
> Patch 1, adds the ETHTOOL_GFP and ETHOOL_SFP commands and the
> associated data structures;
> 
> Patch 2, adds the ETHTOOL_MSG_PREEMPT_GET and ETHTOOL_MSG_PREEMPT_SET
> netlink messages and the associated attributes;

I didn't look too deeply but one thing I noticed is that setting the
parameters using ioctl() does not trigger netlink notification. If we
decide to implement ioctl support (and I'm not a fan of that), the
notifications should be sent even when ioctl is used.

Michal

[Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Michal Kubecek]

On Fri, May 15, 2020 at 06:29:44PM -0700, Vinicius Costa Gomes wrote:
> Hi,
> 
> This series adds support for configuring frame preemption, as defined
> by IEEE 802.1Q-2018 (previously IEEE 802.1Qbu) and IEEE 802.3br.
> 
> Frame preemption allows a packet from a higher priority queue marked
> as "express" to preempt a packet from lower priority queue marked as
> "preemptible". The idea is that this can help reduce the latency for
> higher priority traffic.
> 
> Previously, the proposed interface for configuring these features was
> using the qdisc layer. But as this is very hardware dependent and all
> that qdisc did was pass the information to the driver, it makes sense
> to have this in ethtool.
> 
> One example, for retrieving and setting the configuration:
> 
> $ ethtool $ sudo ./ethtool --show-frame-preemption enp3s0
> Frame preemption settings for enp3s0:
> 	support: supported

IMHO we don't need a special bool for this. IIUC this is not a state
flag that would change value for a particular device; either the device
supports the feature or it does not. If it does not, the ethtool_ops
callbacks would return -EOPNOTSUPP (or would not even exist if the
driver has no support) and ethtool would say so.

> 	active: active
> 	supported queues: 0xf
> 	supported queues: 0xe
> 	minimum fragment size: 68
> 
> 
> $ ethtool --set-frame-preemption enp3s0 fp on min-frag-size 68 preemptible-queues-mask 0xe
> 
> This is a RFC because I wanted to have feedback on some points:
> 
>   - The parameters added are enough for the hardware I have, is it
>     enough in general?
> 
>   - even with the ethtool via netlink effort, I chose to keep the
>     ioctl() way, in case someone wants to backport this to an older
>     kernel, is there a problem with this?

I would prefer not extending ioctl interface with new features, with
obvious exceptions like adding new link modes or so. Not only because
having new features only available through netlink will motivate authors
of userspace tools to support netlink but mostly because the lack of
flexibility and extensibility of ioctl interface inevitably leads to
compromises you wouldn't have to do if you only implement netlink
requests.

One example I can see is the use of u32 for queue bitmaps. Perhaps you
don't expect this feature to be supported on devices with more than 32
queues (and I don't have enough expertise to tell if it's justified at
the moment) but can you be sure it will be the case in 10 or 20 years?
As long as these hardcoded u32 bitmaps are only part of internal kernel
API (ethtool_ops), extending the support for bigger devices will mean
some code churn (possibly large if many drivers implement the feature)
but it's something that can be done. But if you have this limit in
userspace API, you are in a much bigger trouble. The same can be said
for adding new attributes - easy with netlink but with ioctl you never
know if those reserved fields will suffice.

> 
>   - Some space for bikeshedding the names and location (for example,
>     does it make sense for these settings to be per-queue?), as I am
>     not quite happy with them, one example, is the use of preemptible
>     vs. preemptable;
> 
> 
> About the patches, should be quite straightforward:
> 
> Patch 1, adds the ETHTOOL_GFP and ETHOOL_SFP commands and the
> associated data structures;
> 
> Patch 2, adds the ETHTOOL_MSG_PREEMPT_GET and ETHTOOL_MSG_PREEMPT_SET
> netlink messages and the associated attributes;

I didn't look too deeply but one thing I noticed is that setting the
parameters using ioctl() does not trigger netlink notification. If we
decide to implement ioctl support (and I'm not a fan of that), the
notifications should be sent even when ioctl is used.

Michal

Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption

[David Miller]

From: Vinicius Costa Gomes <vinicius.gomes@intel.com>
Date: Fri, 15 May 2020 18:29:44 -0700

> This series adds support for configuring frame preemption, as defined
> by IEEE 802.1Q-2018 (previously IEEE 802.1Qbu) and IEEE 802.3br.
> 
> Frame preemption allows a packet from a higher priority queue marked
> as "express" to preempt a packet from lower priority queue marked as
> "preemptible". The idea is that this can help reduce the latency for
> higher priority traffic.

Why do we need yet another name for something which is just basic
traffic prioritization and why is this configured via ethtool instead
of the "traffic classifier" which is where all of this stuff should
be done?

[Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption

[David Miller]

From: Vinicius Costa Gomes <vinicius.gomes@intel.com>
Date: Fri, 15 May 2020 18:29:44 -0700

> This series adds support for configuring frame preemption, as defined
> by IEEE 802.1Q-2018 (previously IEEE 802.1Qbu) and IEEE 802.3br.
> 
> Frame preemption allows a packet from a higher priority queue marked
> as "express" to preempt a packet from lower priority queue marked as
> "preemptible". The idea is that this can help reduce the latency for
> higher priority traffic.

Why do we need yet another name for something which is just basic
traffic prioritization and why is this configured via ethtool instead
of the "traffic classifier" which is where all of this stuff should
be done?

Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Vladimir Oltean]

Hi David,

On Sat, 16 May 2020 at 23:39, David Miller <davem@davemloft.net> wrote:
>
> From: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> Date: Fri, 15 May 2020 18:29:44 -0700
>
> > This series adds support for configuring frame preemption, as defined
> > by IEEE 802.1Q-2018 (previously IEEE 802.1Qbu) and IEEE 802.3br.
> >
> > Frame preemption allows a packet from a higher priority queue marked
> > as "express" to preempt a packet from lower priority queue marked as
> > "preemptible". The idea is that this can help reduce the latency for
> > higher priority traffic.
>
> Why do we need yet another name for something which is just basic
> traffic prioritization and why is this configured via ethtool instead
> of the "traffic classifier" which is where all of this stuff should
> be done?

It is not 'just another name for basic traffic prioritization'. With
basic traffic prioritization only, a high-priority packet still has to
wait in the egress queue of a switch until a (potentially large)
low-priority packet has finished transmission and has freed the
medium. Frame preemption changes that. Actually it requires hardware
support on both ends, because the way it is transmitted on the wire is
not compatible with regular Ethernet frames (it uses a special Start
Of Frame Delimiter to encode preemptible traffic).
I know we are talking about ridiculously low improvements in latency,
but the background is that Ethernet is making its way into the
industrial and process control fields, and for that type of
application you need to ensure minimally low and maximally consistent
end-to-end latencies. Frame preemption helps with the "minimally low"
part. The way it works is that typically there are 2 MACs per
interface (1 is "express" - equivalent to the legacy type, and the
other is "preemptible" - the new type) and this new IEEE 802.1Q clause
thing allows some arbitration/preemption event to happen between the
two MACs. When a preemption event happens, the preemptible MAC quickly
wraps up and aborts the frame it's currently transmitting (to come
back and continue later), making room for the express MAC to do its
thing because it's time-constrained. Then, after the express MAC
finishes, the preemptible MAC continues with the rest of the frame
fragments from where it was preempted.
As to why this doesn't go to tc but to ethtool: why would it go to tc?
You can't emulate such behavior in software. It's a hardware feature.
You only* (more or less) need to specify which traffic classes on a
port go to the preemptible MAC and which go to the express MAC. We
discussed about the possibility of extending tc-taprio to configure
frame preemption through it, but the consensus was that somebody might
want to use frame preemption as a standalone feature, without
scheduled traffic, and that inventing another qdisc for frame
preemption alone would be too much of a formalism. (I hope I didn't
omit anything important from the previous discussion on the topic)

Thanks,
-Vladimir

[Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Vladimir Oltean]

Hi David,

On Sat, 16 May 2020 at 23:39, David Miller <davem@davemloft.net> wrote:
>
> From: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> Date: Fri, 15 May 2020 18:29:44 -0700
>
> > This series adds support for configuring frame preemption, as defined
> > by IEEE 802.1Q-2018 (previously IEEE 802.1Qbu) and IEEE 802.3br.
> >
> > Frame preemption allows a packet from a higher priority queue marked
> > as "express" to preempt a packet from lower priority queue marked as
> > "preemptible". The idea is that this can help reduce the latency for
> > higher priority traffic.
>
> Why do we need yet another name for something which is just basic
> traffic prioritization and why is this configured via ethtool instead
> of the "traffic classifier" which is where all of this stuff should
> be done?

It is not 'just another name for basic traffic prioritization'. With
basic traffic prioritization only, a high-priority packet still has to
wait in the egress queue of a switch until a (potentially large)
low-priority packet has finished transmission and has freed the
medium. Frame preemption changes that. Actually it requires hardware
support on both ends, because the way it is transmitted on the wire is
not compatible with regular Ethernet frames (it uses a special Start
Of Frame Delimiter to encode preemptible traffic).
I know we are talking about ridiculously low improvements in latency,
but the background is that Ethernet is making its way into the
industrial and process control fields, and for that type of
application you need to ensure minimally low and maximally consistent
end-to-end latencies. Frame preemption helps with the "minimally low"
part. The way it works is that typically there are 2 MACs per
interface (1 is "express" - equivalent to the legacy type, and the
other is "preemptible" - the new type) and this new IEEE 802.1Q clause
thing allows some arbitration/preemption event to happen between the
two MACs. When a preemption event happens, the preemptible MAC quickly
wraps up and aborts the frame it's currently transmitting (to come
back and continue later), making room for the express MAC to do its
thing because it's time-constrained. Then, after the express MAC
finishes, the preemptible MAC continues with the rest of the frame
fragments from where it was preempted.
As to why this doesn't go to tc but to ethtool: why would it go to tc?
You can't emulate such behavior in software. It's a hardware feature.
You only* (more or less) need to specify which traffic classes on a
port go to the preemptible MAC and which go to the express MAC. We
discussed about the possibility of extending tc-taprio to configure
frame preemption through it, but the consensus was that somebody might
want to use frame preemption as a standalone feature, without
scheduled traffic, and that inventing another qdisc for frame
preemption alone would be too much of a formalism. (I hope I didn't
omit anything important from the previous discussion on the topic)

Thanks,
-Vladimir

Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption

[David Miller]

From: Vladimir Oltean <olteanv@gmail.com>
Date: Sun, 17 May 2020 00:03:39 +0300

> As to why this doesn't go to tc but to ethtool: why would it go to tc?

Maybe you can't %100 duplicate the on-the-wire special format and
whatever, but the queueing behavior ABSOLUTELY you can emulate in
software.

And then you have the proper hooks added for HW offload which can
do the on-the-wire stuff.

That's how we do these things, not with bolted on ethtool stuff.

[Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption

[David Miller]

From: Vladimir Oltean <olteanv@gmail.com>
Date: Sun, 17 May 2020 00:03:39 +0300

> As to why this doesn't go to tc but to ethtool: why would it go to tc?

Maybe you can't %100 duplicate the on-the-wire special format and
whatever, but the queueing behavior ABSOLUTELY you can emulate in
software.

And then you have the proper hooks added for HW offload which can
do the on-the-wire stuff.

That's how we do these things, not with bolted on ethtool stuff.

Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Vladimir Oltean]

On Sun, 17 May 2020 at 01:19, David Miller <davem@davemloft.net> wrote:
>
> From: Vladimir Oltean <olteanv@gmail.com>
> Date: Sun, 17 May 2020 00:03:39 +0300
>
> > As to why this doesn't go to tc but to ethtool: why would it go to tc?
>
> Maybe you can't %100 duplicate the on-the-wire special format and
> whatever, but the queueing behavior ABSOLUTELY you can emulate in
> software.
>
> And then you have the proper hooks added for HW offload which can
> do the on-the-wire stuff.
>
> That's how we do these things, not with bolted on ethtool stuff.

When talking about frame preemption in the way that it is defined in
802.1Qbu and 802.3br, it says or assumes nothing about queuing. It
describes the fact that there are 2 MACs per interface, 1 MAC dealing
with some traffic classes and the other dealing with the rest. Queuing
sits completely above the layer where frame preemption happens:
- The queuing layer does not care if packets go to a traffic class
that is serviced by a preemptible MAC or an express MAC
- The MAC does not care by what means have packets been classified to
one traffic class or another.
I have no idea what emulation of queuing behavior you are talking
about. Frame preemption is a MAC hardware feature. Your NIC supports
it or it doesn't. Which means it can talk to a link partner that
supports frame preemption, or it can't.

Thanks,
-Vladimir

[Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Vladimir Oltean]

On Sun, 17 May 2020 at 01:19, David Miller <davem@davemloft.net> wrote:
>
> From: Vladimir Oltean <olteanv@gmail.com>
> Date: Sun, 17 May 2020 00:03:39 +0300
>
> > As to why this doesn't go to tc but to ethtool: why would it go to tc?
>
> Maybe you can't %100 duplicate the on-the-wire special format and
> whatever, but the queueing behavior ABSOLUTELY you can emulate in
> software.
>
> And then you have the proper hooks added for HW offload which can
> do the on-the-wire stuff.
>
> That's how we do these things, not with bolted on ethtool stuff.

When talking about frame preemption in the way that it is defined in
802.1Qbu and 802.3br, it says or assumes nothing about queuing. It
describes the fact that there are 2 MACs per interface, 1 MAC dealing
with some traffic classes and the other dealing with the rest. Queuing
sits completely above the layer where frame preemption happens:
- The queuing layer does not care if packets go to a traffic class
that is serviced by a preemptible MAC or an express MAC
- The MAC does not care by what means have packets been classified to
one traffic class or another.
I have no idea what emulation of queuing behavior you are talking
about. Frame preemption is a MAC hardware feature. Your NIC supports
it or it doesn't. Which means it can talk to a link partner that
supports frame preemption, or it can't.

Thanks,
-Vladimir

Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Michael Walle]

Am Fri, 15 May 2020 18:29:44 -0700
schrieb Vinicius Costa Gomes <vinicius.gomes@intel.com>:

> Hi,
> 
> This series adds support for configuring frame preemption, as defined
> by IEEE 802.1Q-2018 (previously IEEE 802.1Qbu) and IEEE 802.3br.
> 
> Frame preemption allows a packet from a higher priority queue marked
> as "express" to preempt a packet from lower priority queue marked as
> "preemptible". The idea is that this can help reduce the latency for
> higher priority traffic.
> 
> Previously, the proposed interface for configuring these features was
> using the qdisc layer. But as this is very hardware dependent and all
> that qdisc did was pass the information to the driver, it makes sense
> to have this in ethtool.
> 
> One example, for retrieving and setting the configuration:
> 
> $ ethtool $ sudo ./ethtool --show-frame-preemption enp3s0
> Frame preemption settings for enp3s0:
> 	support: supported
> 	active: active
> 	supported queues: 0xf
> 	supported queues: 0xe
> 	minimum fragment size: 68
> 
> 
> $ ethtool --set-frame-preemption enp3s0 fp on min-frag-size 68
> preemptible-queues-mask 0xe
> 
> This is a RFC because I wanted to have feedback on some points:
> 
>   - The parameters added are enough for the hardware I have, is it
>     enough in general?

What about the Qbu handshake state? And some NICs support overriding
this. I.e. enable frame preemption even if the handshake wasn't
successful.

-michael

> 
>   - even with the ethtool via netlink effort, I chose to keep the
>     ioctl() way, in case someone wants to backport this to an older
>     kernel, is there a problem with this?
> 
>   - Some space for bikeshedding the names and location (for example,
>     does it make sense for these settings to be per-queue?), as I am
>     not quite happy with them, one example, is the use of preemptible
>     vs. preemptable;
> 
> 
> About the patches, should be quite straightforward:
> 
> Patch 1, adds the ETHTOOL_GFP and ETHOOL_SFP commands and the
> associated data structures;
> 
> Patch 2, adds the ETHTOOL_MSG_PREEMPT_GET and ETHTOOL_MSG_PREEMPT_SET
> netlink messages and the associated attributes;
> 
> Patch 3, is the example implementation for the igc driver, the catch
> here is that frame preemption in igc is dependent on the TSN "mode"
> being enabled;
> 
> Patch 4, adds some registers that helped during implementation.
> 
> Another thing is that because igc is still under development, to avoid
> conflicts, I think it might be easier for the PATCH version of this
> series to go through Jeff Kirsher's tree.
> 
> Vinicius Costa Gomes (4):
>   ethtool: Add support for configuring frame preemption
>   ethtool: Add support for configuring frame preemption via netlink
>   igc: Add support for configuring frame preemption
>   igc: Add support for exposing frame preemption stats registers
> 
>  drivers/net/ethernet/intel/igc/igc.h         |   3 +
>  drivers/net/ethernet/intel/igc/igc_defines.h |   6 +
>  drivers/net/ethernet/intel/igc/igc_ethtool.c |  77 ++++++++
>  drivers/net/ethernet/intel/igc/igc_regs.h    |  10 +
>  drivers/net/ethernet/intel/igc/igc_tsn.c     |  46 ++++-
>  include/linux/ethtool.h                      |   6 +
>  include/uapi/linux/ethtool.h                 |  25 +++
>  include/uapi/linux/ethtool_netlink.h         |  19 ++
>  net/ethtool/Makefile                         |   3 +-
>  net/ethtool/ioctl.c                          |  36 ++++
>  net/ethtool/netlink.c                        |  15 ++
>  net/ethtool/netlink.h                        |   2 +
>  net/ethtool/preempt.c                        | 181
> +++++++++++++++++++ 13 files changed, 423 insertions(+), 6
> deletions(-) create mode 100644 net/ethtool/preempt.c
> 

Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Andrew Lunn]

On Sun, May 17, 2020 at 01:51:19PM +0300, Vladimir Oltean wrote:
> On Sun, 17 May 2020 at 01:19, David Miller <davem@davemloft.net> wrote:
> >
> > From: Vladimir Oltean <olteanv@gmail.com>
> > Date: Sun, 17 May 2020 00:03:39 +0300
> >
> > > As to why this doesn't go to tc but to ethtool: why would it go to tc?
> >
> > Maybe you can't %100 duplicate the on-the-wire special format and
> > whatever, but the queueing behavior ABSOLUTELY you can emulate in
> > software.
> >
> > And then you have the proper hooks added for HW offload which can
> > do the on-the-wire stuff.
> >
> > That's how we do these things, not with bolted on ethtool stuff.
> 
> When talking about frame preemption in the way that it is defined in
> 802.1Qbu and 802.3br, it says or assumes nothing about queuing. It
> describes the fact that there are 2 MACs per interface, 1 MAC dealing
> with some traffic classes and the other dealing with the rest.

I did not follow the previous discussion, but i assume you talked
about modelling it in Linux as two MACs? Why was that approach not
followed?

   Andrew

[Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Andrew Lunn]

On Sun, May 17, 2020 at 01:51:19PM +0300, Vladimir Oltean wrote:
> On Sun, 17 May 2020 at 01:19, David Miller <davem@davemloft.net> wrote:
> >
> > From: Vladimir Oltean <olteanv@gmail.com>
> > Date: Sun, 17 May 2020 00:03:39 +0300
> >
> > > As to why this doesn't go to tc but to ethtool: why would it go to tc?
> >
> > Maybe you can't %100 duplicate the on-the-wire special format and
> > whatever, but the queueing behavior ABSOLUTELY you can emulate in
> > software.
> >
> > And then you have the proper hooks added for HW offload which can
> > do the on-the-wire stuff.
> >
> > That's how we do these things, not with bolted on ethtool stuff.
> 
> When talking about frame preemption in the way that it is defined in
> 802.1Qbu and 802.3br, it says or assumes nothing about queuing. It
> describes the fact that there are 2 MACs per interface, 1 MAC dealing
> with some traffic classes and the other dealing with the rest.

I did not follow the previous discussion, but i assume you talked
about modelling it in Linux as two MACs? Why was that approach not
followed?

   Andrew

Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Vladimir Oltean]

Hi Andrew,

On Sun, 17 May 2020 at 21:45, Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Sun, May 17, 2020 at 01:51:19PM +0300, Vladimir Oltean wrote:
> > On Sun, 17 May 2020 at 01:19, David Miller <davem@davemloft.net> wrote:
> > >
> > > From: Vladimir Oltean <olteanv@gmail.com>
> > > Date: Sun, 17 May 2020 00:03:39 +0300
> > >
> > > > As to why this doesn't go to tc but to ethtool: why would it go to tc?
> > >
> > > Maybe you can't %100 duplicate the on-the-wire special format and
> > > whatever, but the queueing behavior ABSOLUTELY you can emulate in
> > > software.
> > >
> > > And then you have the proper hooks added for HW offload which can
> > > do the on-the-wire stuff.
> > >
> > > That's how we do these things, not with bolted on ethtool stuff.
> >
> > When talking about frame preemption in the way that it is defined in
> > 802.1Qbu and 802.3br, it says or assumes nothing about queuing. It
> > describes the fact that there are 2 MACs per interface, 1 MAC dealing
> > with some traffic classes and the other dealing with the rest.
>
> I did not follow the previous discussion, but i assume you talked
> about modelling it in Linux as two MACs? Why was that approach not
> followed?
>
>    Andrew

I don't recall having discussed that option, but I guess that you can
see why, if I quote this portion from IEEE 802.1Q-2018:

6.7.1 Support of the ISS by IEEE Std 802.3 (Ethernet)
Mapping between M_CONTROL.requests/indications and IEEE 802.3
MA_CONTROL.requests/indications is performed as specified in IEEE Std
802.1AC. If the MAC supports IEEE 802.3br (TM) Interspersing Express
Traffic, then PFC M_CONTROL.requests are mapped onto the MAC control
interface associated with the express MAC (eMAC).
If frame preemption (6.7.2) is supported on a Port, then the IEEE
802.3 MAC provides the following two MAC service interfaces (99.4 of
IEEE Std 802.3br (TM)-2016 [B21]):
a) A preemptible MAC (pMAC) service interface
b) An express MAC (eMAC) service interface
For priority values that are identified in the frame preemption status
table (6.7.2) as preemptible, frames that are selected for
transmission shall be transmitted using the pMAC service instance, and
for priority values that are identified in the frame preemption status
table as express, frames that are selected for transmission shall be
transmitted using the eMAC service instance.

_In all other respects, the Port behaves as if it is supported by a
single MAC service interface_ (emphasis mine). In particular, all
frames received by the Port are treated as if they were received on a
single MAC service interface regardless of whether they were received
on the eMAC service interface or the pMAC service interface, except
with respect to frame preemption.

Thanks,
-Vladimir

[Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Vladimir Oltean]

Hi Andrew,

On Sun, 17 May 2020 at 21:45, Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Sun, May 17, 2020 at 01:51:19PM +0300, Vladimir Oltean wrote:
> > On Sun, 17 May 2020 at 01:19, David Miller <davem@davemloft.net> wrote:
> > >
> > > From: Vladimir Oltean <olteanv@gmail.com>
> > > Date: Sun, 17 May 2020 00:03:39 +0300
> > >
> > > > As to why this doesn't go to tc but to ethtool: why would it go to tc?
> > >
> > > Maybe you can't %100 duplicate the on-the-wire special format and
> > > whatever, but the queueing behavior ABSOLUTELY you can emulate in
> > > software.
> > >
> > > And then you have the proper hooks added for HW offload which can
> > > do the on-the-wire stuff.
> > >
> > > That's how we do these things, not with bolted on ethtool stuff.
> >
> > When talking about frame preemption in the way that it is defined in
> > 802.1Qbu and 802.3br, it says or assumes nothing about queuing. It
> > describes the fact that there are 2 MACs per interface, 1 MAC dealing
> > with some traffic classes and the other dealing with the rest.
>
> I did not follow the previous discussion, but i assume you talked
> about modelling it in Linux as two MACs? Why was that approach not
> followed?
>
>    Andrew

I don't recall having discussed that option, but I guess that you can
see why, if I quote this portion from IEEE 802.1Q-2018:

6.7.1 Support of the ISS by IEEE Std 802.3 (Ethernet)
Mapping between M_CONTROL.requests/indications and IEEE 802.3
MA_CONTROL.requests/indications is performed as specified in IEEE Std
802.1AC. If the MAC supports IEEE 802.3br (TM) Interspersing Express
Traffic, then PFC M_CONTROL.requests are mapped onto the MAC control
interface associated with the express MAC (eMAC).
If frame preemption (6.7.2) is supported on a Port, then the IEEE
802.3 MAC provides the following two MAC service interfaces (99.4 of
IEEE Std 802.3br (TM)-2016 [B21]):
a) A preemptible MAC (pMAC) service interface
b) An express MAC (eMAC) service interface
For priority values that are identified in the frame preemption status
table (6.7.2) as preemptible, frames that are selected for
transmission shall be transmitted using the pMAC service instance, and
for priority values that are identified in the frame preemption status
table as express, frames that are selected for transmission shall be
transmitted using the eMAC service instance.

_In all other respects, the Port behaves as if it is supported by a
single MAC service interface_ (emphasis mine). In particular, all
frames received by the Port are treated as if they were received on a
single MAC service interface regardless of whether they were received
on the eMAC service interface or the pMAC service interface, except
with respect to frame preemption.

Thanks,
-Vladimir

Re: [next-queue RFC 2/4] ethtool: Add support for configuring frame preemption via netlink

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 3873 bytes --]

Hi Vinicius,

url:    https://github.com/0day-ci/linux/commits/Vinicius-Costa-Gomes/ethtool-Add-support-for-frame-preemption/20200516-093235
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/next-queue.git dev-queue

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
net/ethtool/preempt.c:152 ethnl_set_preempt() warn: variable dereferenced before check 'info' (see line 127)

# https://github.com/0day-ci/linux/commit/5d130ebd7a21741d4fda1c8829a32353e10d49d5
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 5d130ebd7a21741d4fda1c8829a32353e10d49d5
vim +/info +152 net/ethtool/preempt.c

5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  118  int ethnl_set_preempt(struct sk_buff *skb, struct genl_info *info)
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  119  {
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  120  	struct nlattr *tb[ETHTOOL_A_LINKINFO_MAX + 1];
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  121  	struct ethtool_fp preempt = {};
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  122  	struct ethnl_req_info req_info = {};
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  123  	struct net_device *dev;
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  124  	bool mod = false;
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  125  	int ret;
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  126  
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15 @127  	ret = nlmsg_parse(info->nlhdr, GENL_HDRLEN, tb,
                                                                          ^^^^^^^^^^^
Unchecked dereference.

5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  128  			  ETHTOOL_A_PREEMPT_MAX, preempt_set_policy,
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  129  			  info->extack);
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  130  	if (ret < 0)
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  131  		return ret;
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  132  
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  133  	ret = ethnl_parse_header_dev_get(&req_info,
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  134  					 tb[ETHTOOL_A_PREEMPT_HEADER],
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  135  					 genl_info_net(info), info->extack,
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  136  					 true);
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  137  	if (ret < 0)
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  138  		return ret;
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  139  	dev = req_info.dev;
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  140  	ret = -EOPNOTSUPP;
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  141  	if (!dev->ethtool_ops->get_preempt ||
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  142  	    !dev->ethtool_ops->set_preempt)
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  143  		goto out_dev;
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  144  
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  145  	rtnl_lock();
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  146  	ret = ethnl_ops_begin(dev);
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  147  	if (ret < 0)
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  148  		goto out_rtnl;
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  149  
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  150  	ret = dev->ethtool_ops->get_preempt(dev, &preempt);
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  151  	if (ret < 0) {
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15 @152  		if (info)
                                                                ^^^^^^^^^
Check is too late to help.

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [next-queue RFC 2/4] ethtool: Add support for configuring frame preemption via netlink

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 3873 bytes --]

Hi Vinicius,

url:    https://github.com/0day-ci/linux/commits/Vinicius-Costa-Gomes/ethtool-Add-support-for-frame-preemption/20200516-093235
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/next-queue.git dev-queue

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
net/ethtool/preempt.c:152 ethnl_set_preempt() warn: variable dereferenced before check 'info' (see line 127)

# https://github.com/0day-ci/linux/commit/5d130ebd7a21741d4fda1c8829a32353e10d49d5
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 5d130ebd7a21741d4fda1c8829a32353e10d49d5
vim +/info +152 net/ethtool/preempt.c

5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  118  int ethnl_set_preempt(struct sk_buff *skb, struct genl_info *info)
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  119  {
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  120  	struct nlattr *tb[ETHTOOL_A_LINKINFO_MAX + 1];
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  121  	struct ethtool_fp preempt = {};
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  122  	struct ethnl_req_info req_info = {};
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  123  	struct net_device *dev;
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  124  	bool mod = false;
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  125  	int ret;
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  126  
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15 @127  	ret = nlmsg_parse(info->nlhdr, GENL_HDRLEN, tb,
                                                                          ^^^^^^^^^^^
Unchecked dereference.

5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  128  			  ETHTOOL_A_PREEMPT_MAX, preempt_set_policy,
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  129  			  info->extack);
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  130  	if (ret < 0)
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  131  		return ret;
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  132  
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  133  	ret = ethnl_parse_header_dev_get(&req_info,
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  134  					 tb[ETHTOOL_A_PREEMPT_HEADER],
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  135  					 genl_info_net(info), info->extack,
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  136  					 true);
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  137  	if (ret < 0)
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  138  		return ret;
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  139  	dev = req_info.dev;
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  140  	ret = -EOPNOTSUPP;
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  141  	if (!dev->ethtool_ops->get_preempt ||
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  142  	    !dev->ethtool_ops->set_preempt)
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  143  		goto out_dev;
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  144  
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  145  	rtnl_lock();
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  146  	ret = ethnl_ops_begin(dev);
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  147  	if (ret < 0)
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  148  		goto out_rtnl;
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  149  
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  150  	ret = dev->ethtool_ops->get_preempt(dev, &preempt);
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15  151  	if (ret < 0) {
5d130ebd7a2174 Vinicius Costa Gomes 2020-05-15 @152  		if (info)
                                                                ^^^^^^^^^
Check is too late to help.

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Murali Karicheri]

Hi,

On 5/17/20 11:06 AM, Michael Walle wrote:
> What about the Qbu handshake state? And some NICs support overriding
> this. I.e. enable frame preemption even if the handshake wasn't
> successful.

You are talking about Verify procedure to hand shake with peer to
know if remote support IET fragmentation and re-assembly? If yes,
this manual mode of provisioning is required as well. So one
optional parameter needed is enable-verify. If that is not enabled
then device assumes the remote is capable of fragmentation and
re-assembly.

-- 
Murali Karicheri
Texas Instruments

Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Vinicius Costa Gomes]

Hi,

David Miller <davem@davemloft.net> writes:

> From: Vladimir Oltean <olteanv@gmail.com>
> Date: Sun, 17 May 2020 00:03:39 +0300
>
>> As to why this doesn't go to tc but to ethtool: why would it go to tc?
>
> Maybe you can't %100 duplicate the on-the-wire special format and
> whatever, but the queueing behavior ABSOLUTELY you can emulate in
> software.

Just saying what Vladimir said in different words: the queueing behavior
is already implemented in software, by mqprio or taprio, for example.

That is to say, if we add frame preemption support to those qdiscs all
they will do is pass the information to the driver, and that's it. They
won't be able to use that information at all.

The mental model I have for this feature is that is more similar to the
segmentation offloads, energy efficient ethernet or auto-negotiation
than it is to a traffic shaper like CBS.

>
> And then you have the proper hooks added for HW offload which can
> do the on-the-wire stuff.
>
> That's how we do these things, not with bolted on ethtool stuff.


Cheers,
-- 
Vinicius

[Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Vinicius Costa Gomes]

Hi,

David Miller <davem@davemloft.net> writes:

> From: Vladimir Oltean <olteanv@gmail.com>
> Date: Sun, 17 May 2020 00:03:39 +0300
>
>> As to why this doesn't go to tc but to ethtool: why would it go to tc?
>
> Maybe you can't %100 duplicate the on-the-wire special format and
> whatever, but the queueing behavior ABSOLUTELY you can emulate in
> software.

Just saying what Vladimir said in different words: the queueing behavior
is already implemented in software, by mqprio or taprio, for example.

That is to say, if we add frame preemption support to those qdiscs all
they will do is pass the information to the driver, and that's it. They
won't be able to use that information at all.

The mental model I have for this feature is that is more similar to the
segmentation offloads, energy efficient ethernet or auto-negotiation
than it is to a traffic shaper like CBS.

>
> And then you have the proper hooks added for HW offload which can
> do the on-the-wire stuff.
>
> That's how we do these things, not with bolted on ethtool stuff.


Cheers,
-- 
Vinicius

Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Vinicius Costa Gomes]

Hi,

Michal Kubecek <mkubecek@suse.cz> writes:

> On Fri, May 15, 2020 at 06:29:44PM -0700, Vinicius Costa Gomes wrote:
>> Hi,
>> 
>> This series adds support for configuring frame preemption, as defined
>> by IEEE 802.1Q-2018 (previously IEEE 802.1Qbu) and IEEE 802.3br.
>> 
>> Frame preemption allows a packet from a higher priority queue marked
>> as "express" to preempt a packet from lower priority queue marked as
>> "preemptible". The idea is that this can help reduce the latency for
>> higher priority traffic.
>> 
>> Previously, the proposed interface for configuring these features was
>> using the qdisc layer. But as this is very hardware dependent and all
>> that qdisc did was pass the information to the driver, it makes sense
>> to have this in ethtool.
>> 
>> One example, for retrieving and setting the configuration:
>> 
>> $ ethtool $ sudo ./ethtool --show-frame-preemption enp3s0
>> Frame preemption settings for enp3s0:
>> 	support: supported
>
> IMHO we don't need a special bool for this. IIUC this is not a state
> flag that would change value for a particular device; either the device
> supports the feature or it does not. If it does not, the ethtool_ops
> callbacks would return -EOPNOTSUPP (or would not even exist if the
> driver has no support) and ethtool would say so.

(I know that the comments below only apply if "ethtool-way" is what's
decided)

Cool. Will remove the supported bit.

>
>> 	active: active
>> 	supported queues: 0xf
>> 	supported queues: 0xe
>> 	minimum fragment size: 68
>> 
>> 
>> $ ethtool --set-frame-preemption enp3s0 fp on min-frag-size 68 preemptible-queues-mask 0xe
>> 
>> This is a RFC because I wanted to have feedback on some points:
>> 
>>   - The parameters added are enough for the hardware I have, is it
>>     enough in general?
>> 
>>   - even with the ethtool via netlink effort, I chose to keep the
>>     ioctl() way, in case someone wants to backport this to an older
>>     kernel, is there a problem with this?
>
> I would prefer not extending ioctl interface with new features, with
> obvious exceptions like adding new link modes or so. Not only because
> having new features only available through netlink will motivate authors
> of userspace tools to support netlink but mostly because the lack of
> flexibility and extensibility of ioctl interface inevitably leads to
> compromises you wouldn't have to do if you only implement netlink
> requests.

Agreed. Will send the next version with only the netlink interface, and
let's see who complains.

>
> One example I can see is the use of u32 for queue bitmaps. Perhaps you
> don't expect this feature to be supported on devices with more than 32
> queues (and I don't have enough expertise to tell if it's justified at
> the moment) but can you be sure it will be the case in 10 or 20 years?
> As long as these hardcoded u32 bitmaps are only part of internal kernel
> API (ethtool_ops), extending the support for bigger devices will mean
> some code churn (possibly large if many drivers implement the feature)
> but it's something that can be done. But if you have this limit in
> userspace API, you are in a much bigger trouble. The same can be said
> for adding new attributes - easy with netlink but with ioctl you never
> know if those reserved fields will suffice.

A bit of background for this decision (using u32), frame preemption has
dimishing returns in relation to link speeds, my gut feeling is that for
links faster than 2.5G it stops making sense, at least in Linux, the
measurement noise will hide any latency improvement brought by frame
preemption. And I don't see many 2.5G NICs supporting more than 32
queues.

But I agree that keeping the interface future proof is better. Will
change to expose the queues configuration as bitset.

>
>> 
>>   - Some space for bikeshedding the names and location (for example,
>>     does it make sense for these settings to be per-queue?), as I am
>>     not quite happy with them, one example, is the use of preemptible
>>     vs. preemptable;
>> 
>> 
>> About the patches, should be quite straightforward:
>> 
>> Patch 1, adds the ETHTOOL_GFP and ETHOOL_SFP commands and the
>> associated data structures;
>> 
>> Patch 2, adds the ETHTOOL_MSG_PREEMPT_GET and ETHTOOL_MSG_PREEMPT_SET
>> netlink messages and the associated attributes;
>
> I didn't look too deeply but one thing I noticed is that setting the
> parameters using ioctl() does not trigger netlink notification. If we
> decide to implement ioctl support (and I'm not a fan of that), the
> notifications should be sent even when ioctl is used.

Oh, yeah, that's right. Nice catch.


Cheers,
-- 
Vinicius

[Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Vinicius Costa Gomes]

Hi,

Michal Kubecek <mkubecek@suse.cz> writes:

> On Fri, May 15, 2020 at 06:29:44PM -0700, Vinicius Costa Gomes wrote:
>> Hi,
>> 
>> This series adds support for configuring frame preemption, as defined
>> by IEEE 802.1Q-2018 (previously IEEE 802.1Qbu) and IEEE 802.3br.
>> 
>> Frame preemption allows a packet from a higher priority queue marked
>> as "express" to preempt a packet from lower priority queue marked as
>> "preemptible". The idea is that this can help reduce the latency for
>> higher priority traffic.
>> 
>> Previously, the proposed interface for configuring these features was
>> using the qdisc layer. But as this is very hardware dependent and all
>> that qdisc did was pass the information to the driver, it makes sense
>> to have this in ethtool.
>> 
>> One example, for retrieving and setting the configuration:
>> 
>> $ ethtool $ sudo ./ethtool --show-frame-preemption enp3s0
>> Frame preemption settings for enp3s0:
>> 	support: supported
>
> IMHO we don't need a special bool for this. IIUC this is not a state
> flag that would change value for a particular device; either the device
> supports the feature or it does not. If it does not, the ethtool_ops
> callbacks would return -EOPNOTSUPP (or would not even exist if the
> driver has no support) and ethtool would say so.

(I know that the comments below only apply if "ethtool-way" is what's
decided)

Cool. Will remove the supported bit.

>
>> 	active: active
>> 	supported queues: 0xf
>> 	supported queues: 0xe
>> 	minimum fragment size: 68
>> 
>> 
>> $ ethtool --set-frame-preemption enp3s0 fp on min-frag-size 68 preemptible-queues-mask 0xe
>> 
>> This is a RFC because I wanted to have feedback on some points:
>> 
>>   - The parameters added are enough for the hardware I have, is it
>>     enough in general?
>> 
>>   - even with the ethtool via netlink effort, I chose to keep the
>>     ioctl() way, in case someone wants to backport this to an older
>>     kernel, is there a problem with this?
>
> I would prefer not extending ioctl interface with new features, with
> obvious exceptions like adding new link modes or so. Not only because
> having new features only available through netlink will motivate authors
> of userspace tools to support netlink but mostly because the lack of
> flexibility and extensibility of ioctl interface inevitably leads to
> compromises you wouldn't have to do if you only implement netlink
> requests.

Agreed. Will send the next version with only the netlink interface, and
let's see who complains.

>
> One example I can see is the use of u32 for queue bitmaps. Perhaps you
> don't expect this feature to be supported on devices with more than 32
> queues (and I don't have enough expertise to tell if it's justified at
> the moment) but can you be sure it will be the case in 10 or 20 years?
> As long as these hardcoded u32 bitmaps are only part of internal kernel
> API (ethtool_ops), extending the support for bigger devices will mean
> some code churn (possibly large if many drivers implement the feature)
> but it's something that can be done. But if you have this limit in
> userspace API, you are in a much bigger trouble. The same can be said
> for adding new attributes - easy with netlink but with ioctl you never
> know if those reserved fields will suffice.

A bit of background for this decision (using u32), frame preemption has
dimishing returns in relation to link speeds, my gut feeling is that for
links faster than 2.5G it stops making sense, at least in Linux, the
measurement noise will hide any latency improvement brought by frame
preemption. And I don't see many 2.5G NICs supporting more than 32
queues.

But I agree that keeping the interface future proof is better. Will
change to expose the queues configuration as bitset.

>
>> 
>>   - Some space for bikeshedding the names and location (for example,
>>     does it make sense for these settings to be per-queue?), as I am
>>     not quite happy with them, one example, is the use of preemptible
>>     vs. preemptable;
>> 
>> 
>> About the patches, should be quite straightforward:
>> 
>> Patch 1, adds the ETHTOOL_GFP and ETHOOL_SFP commands and the
>> associated data structures;
>> 
>> Patch 2, adds the ETHTOOL_MSG_PREEMPT_GET and ETHTOOL_MSG_PREEMPT_SET
>> netlink messages and the associated attributes;
>
> I didn't look too deeply but one thing I noticed is that setting the
> parameters using ioctl() does not trigger netlink notification. If we
> decide to implement ioctl support (and I'm not a fan of that), the
> notifications should be sent even when ioctl is used.

Oh, yeah, that's right. Nice catch.


Cheers,
-- 
Vinicius

Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Jakub Kicinski]

On Mon, 18 May 2020 12:05:04 -0700 Vinicius Costa Gomes wrote:
> David Miller <davem@davemloft.net> writes:
> > From: Vladimir Oltean <olteanv@gmail.com>
> > Date: Sun, 17 May 2020 00:03:39 +0300
> >  
> >> As to why this doesn't go to tc but to ethtool: why would it go to tc?  
> >
> > Maybe you can't %100 duplicate the on-the-wire special format and
> > whatever, but the queueing behavior ABSOLUTELY you can emulate in
> > software.  
> 
> Just saying what Vladimir said in different words: the queueing behavior
> is already implemented in software, by mqprio or taprio, for example.
> 
> That is to say, if we add frame preemption support to those qdiscs all
> they will do is pass the information to the driver, and that's it. They
> won't be able to use that information at all.
> 
> The mental model I have for this feature is that is more similar to the
> segmentation offloads, energy efficient ethernet or auto-negotiation
> than it is to a traffic shaper like CBS.

Please take a look at the example from the cover letter:

$ ethtool $ sudo ./ethtool --show-frame-preemption
enp3s0 Frame preemption settings for enp3s0:
	support: supported
	active: active
	supported queues: 0xf
	supported queues: 0xe
	minimum fragment size: 68

Reading this I have no idea what 0xe is. I have to go and query TC API
to see what priorities and queues that will be. Which IMHO is a strong
argument that this information belongs there in the first place.

[Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Jakub Kicinski]

On Mon, 18 May 2020 12:05:04 -0700 Vinicius Costa Gomes wrote:
> David Miller <davem@davemloft.net> writes:
> > From: Vladimir Oltean <olteanv@gmail.com>
> > Date: Sun, 17 May 2020 00:03:39 +0300
> >  
> >> As to why this doesn't go to tc but to ethtool: why would it go to tc?  
> >
> > Maybe you can't %100 duplicate the on-the-wire special format and
> > whatever, but the queueing behavior ABSOLUTELY you can emulate in
> > software.  
> 
> Just saying what Vladimir said in different words: the queueing behavior
> is already implemented in software, by mqprio or taprio, for example.
> 
> That is to say, if we add frame preemption support to those qdiscs all
> they will do is pass the information to the driver, and that's it. They
> won't be able to use that information at all.
> 
> The mental model I have for this feature is that is more similar to the
> segmentation offloads, energy efficient ethernet or auto-negotiation
> than it is to a traffic shaper like CBS.

Please take a look at the example from the cover letter:

$ ethtool $ sudo ./ethtool --show-frame-preemption
enp3s0 Frame preemption settings for enp3s0:
	support: supported
	active: active
	supported queues: 0xf
	supported queues: 0xe
	minimum fragment size: 68

Reading this I have no idea what 0xe is. I have to go and query TC API
to see what priorities and queues that will be. Which IMHO is a strong
argument that this information belongs there in the first place.

Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Vinicius Costa Gomes]

Hi,

Jakub Kicinski <kuba@kernel.org> writes:
>
> Please take a look at the example from the cover letter:
>
> $ ethtool $ sudo ./ethtool --show-frame-preemption
> enp3s0 Frame preemption settings for enp3s0:
> 	support: supported
> 	active: active
> 	supported queues: 0xf
> 	supported queues: 0xe
> 	minimum fragment size: 68
>
> Reading this I have no idea what 0xe is. I have to go and query TC API
> to see what priorities and queues that will be. Which IMHO is a strong
> argument that this information belongs there in the first place.

That was the (only?) strong argument in favor of having frame preemption
in the TC side when this was last discussed.

We can have a hybrid solution, we can move the express/preemptible per
queue map to mqprio/taprio/whatever. And have the more specific
configuration knobs, minimum fragment size, etc, in ethtool.

What do you think?


Cheers,
-- 
Vinicius

[Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Vinicius Costa Gomes]

Hi,

Jakub Kicinski <kuba@kernel.org> writes:
>
> Please take a look at the example from the cover letter:
>
> $ ethtool $ sudo ./ethtool --show-frame-preemption
> enp3s0 Frame preemption settings for enp3s0:
> 	support: supported
> 	active: active
> 	supported queues: 0xf
> 	supported queues: 0xe
> 	minimum fragment size: 68
>
> Reading this I have no idea what 0xe is. I have to go and query TC API
> to see what priorities and queues that will be. Which IMHO is a strong
> argument that this information belongs there in the first place.

That was the (only?) strong argument in favor of having frame preemption
in the TC side when this was last discussed.

We can have a hybrid solution, we can move the express/preemptible per
queue map to mqprio/taprio/whatever. And have the more specific
configuration knobs, minimum fragment size, etc, in ethtool.

What do you think?


Cheers,
-- 
Vinicius

Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Jakub Kicinski]

On Mon, 18 May 2020 15:06:26 -0700 Vinicius Costa Gomes wrote:
> Jakub Kicinski <kuba@kernel.org> writes:
> >
> > Please take a look at the example from the cover letter:
> >
> > $ ethtool $ sudo ./ethtool --show-frame-preemption
> > enp3s0 Frame preemption settings for enp3s0:
> > 	support: supported
> > 	active: active
> > 	supported queues: 0xf
> > 	supported queues: 0xe
> > 	minimum fragment size: 68
> >
> > Reading this I have no idea what 0xe is. I have to go and query TC API
> > to see what priorities and queues that will be. Which IMHO is a strong
> > argument that this information belongs there in the first place.  
> 
> That was the (only?) strong argument in favor of having frame preemption
> in the TC side when this was last discussed.
> 
> We can have a hybrid solution, we can move the express/preemptible per
> queue map to mqprio/taprio/whatever. And have the more specific
> configuration knobs, minimum fragment size, etc, in ethtool.
> 
> What do you think?

Does the standard specify minimum fragment size as a global MAC setting?

[Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Jakub Kicinski]

On Mon, 18 May 2020 15:06:26 -0700 Vinicius Costa Gomes wrote:
> Jakub Kicinski <kuba@kernel.org> writes:
> >
> > Please take a look at the example from the cover letter:
> >
> > $ ethtool $ sudo ./ethtool --show-frame-preemption
> > enp3s0 Frame preemption settings for enp3s0:
> > 	support: supported
> > 	active: active
> > 	supported queues: 0xf
> > 	supported queues: 0xe
> > 	minimum fragment size: 68
> >
> > Reading this I have no idea what 0xe is. I have to go and query TC API
> > to see what priorities and queues that will be. Which IMHO is a strong
> > argument that this information belongs there in the first place.  
> 
> That was the (only?) strong argument in favor of having frame preemption
> in the TC side when this was last discussed.
> 
> We can have a hybrid solution, we can move the express/preemptible per
> queue map to mqprio/taprio/whatever. And have the more specific
> configuration knobs, minimum fragment size, etc, in ethtool.
> 
> What do you think?

Does the standard specify minimum fragment size as a global MAC setting?

Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Vinicius Costa Gomes]

Jakub Kicinski <kuba@kernel.org> writes:

>> That was the (only?) strong argument in favor of having frame preemption
>> in the TC side when this was last discussed.
>> 
>> We can have a hybrid solution, we can move the express/preemptible per
>> queue map to mqprio/taprio/whatever. And have the more specific
>> configuration knobs, minimum fragment size, etc, in ethtool.
>> 
>> What do you think?
>
> Does the standard specify minimum fragment size as a global MAC setting?

Yes, it's a per-MAC setting, not per-queue. 


-- 
Vinicius

[Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Vinicius Costa Gomes]

Jakub Kicinski <kuba@kernel.org> writes:

>> That was the (only?) strong argument in favor of having frame preemption
>> in the TC side when this was last discussed.
>> 
>> We can have a hybrid solution, we can move the express/preemptible per
>> queue map to mqprio/taprio/whatever. And have the more specific
>> configuration knobs, minimum fragment size, etc, in ethtool.
>> 
>> What do you think?
>
> Does the standard specify minimum fragment size as a global MAC setting?

Yes, it's a per-MAC setting, not per-queue. 


-- 
Vinicius

Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Jakub Kicinski]

On Mon, 18 May 2020 16:05:08 -0700 Vinicius Costa Gomes wrote:
> Jakub Kicinski <kuba@kernel.org> writes:
> >> That was the (only?) strong argument in favor of having frame preemption
> >> in the TC side when this was last discussed.
> >> 
> >> We can have a hybrid solution, we can move the express/preemptible per
> >> queue map to mqprio/taprio/whatever. And have the more specific
> >> configuration knobs, minimum fragment size, etc, in ethtool.
> >> 
> >> What do you think?  
> >
> > Does the standard specify minimum fragment size as a global MAC setting?  
> 
> Yes, it's a per-MAC setting, not per-queue. 

If standard defines it as per-MAC and we can reasonably expect vendors
won't try to "add value" and make it per queue (unlikely here AFAIU),
then for this part ethtool configuration seems okay to me.

[Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Jakub Kicinski]

On Mon, 18 May 2020 16:05:08 -0700 Vinicius Costa Gomes wrote:
> Jakub Kicinski <kuba@kernel.org> writes:
> >> That was the (only?) strong argument in favor of having frame preemption
> >> in the TC side when this was last discussed.
> >> 
> >> We can have a hybrid solution, we can move the express/preemptible per
> >> queue map to mqprio/taprio/whatever. And have the more specific
> >> configuration knobs, minimum fragment size, etc, in ethtool.
> >> 
> >> What do you think?  
> >
> > Does the standard specify minimum fragment size as a global MAC setting?  
> 
> Yes, it's a per-MAC setting, not per-queue. 

If standard defines it as per-MAC and we can reasonably expect vendors
won't try to "add value" and make it per queue (unlikely here AFAIU),
then for this part ethtool configuration seems okay to me.

Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Murali Karicheri]

Hi Vinicius,

On 5/15/20 9:29 PM, Vinicius Costa Gomes wrote:
> Hi,
> 
> This series adds support for configuring frame preemption, as defined
> by IEEE 802.1Q-2018 (previously IEEE 802.1Qbu) and IEEE 802.3br.
> 
> Frame preemption allows a packet from a higher priority queue marked
> as "express" to preempt a packet from lower priority queue marked as
> "preemptible". The idea is that this can help reduce the latency for
> higher priority traffic.
> 
> Previously, the proposed interface for configuring these features was
> using the qdisc layer. But as this is very hardware dependent and all
> that qdisc did was pass the information to the driver, it makes sense
> to have this in ethtool.
> 
> One example, for retrieving and setting the configuration:
> 
> $ ethtool $ sudo ./ethtool --show-frame-preemption enp3s0
> Frame preemption settings for enp3s0:
> 	support: supported
> 	active: active
> 	supported queues: 0xf

I assume this is will be in sync with ethtool -L output which indicates
how many tx h/w queues present? I mean if there are 8 h/w queues,
supported queues will show 0xff.

> 	supported queues: 0xe
 From the command below, it appears this is the preemptible queue mask.
bit 0  is Q0, bit 1 Q1 and so forth. Right? In that case isn't it more
clear to display
         preemptible queues : 0xef

In the above Q7 is express queue and Q6-Q0 are preemptible.

Also there is a handshake called verify that happens which initiated
by the h/w to check the capability of peer. It looks like
not all vendor's hardware supports it and good to have it displayed
something like

         Verify supported/{not supported}

If Verify is supported, FPE is enabled only if it succeeds. So may be
good to show a status of Verify if it is supported something like
         Verify success/Failed

> 	minimum fragment size: 68
> 
> 
> $ ethtool --set-frame-preemption enp3s0 fp on min-frag-size 68 preemptible-queues-mask 0xe
> 
> This is a RFC because I wanted to have feedback on some points:
> 
>    - The parameters added are enough for the hardware I have, is it
>      enough in general?

As described above, it would be good to add an optional parameter for
verify

ethtool --set-frame-preemption enp3s0 fp on min-frag-size 68 
preemptible-queues-mask 0xe verify on

> 
>    - even with the ethtool via netlink effort, I chose to keep the
>      ioctl() way, in case someone wants to backport this to an older
>      kernel, is there a problem with this?
> 
>    - Some space for bikeshedding the names and location (for example,
>      does it make sense for these settings to be per-queue?), as I am
>      not quite happy with them, one example, is the use of preemptible
>      vs. preemptable;
> 
> 
> About the patches, should be quite straightforward:
> 
> Patch 1, adds the ETHTOOL_GFP and ETHOOL_SFP commands and the
> associated data structures;
> 
> Patch 2, adds the ETHTOOL_MSG_PREEMPT_GET and ETHTOOL_MSG_PREEMPT_SET
> netlink messages and the associated attributes;
> 
> Patch 3, is the example implementation for the igc driver, the catch
> here is that frame preemption in igc is dependent on the TSN "mode"
> being enabled;
> 
> Patch 4, adds some registers that helped during implementation.
> 
> Another thing is that because igc is still under development, to avoid
> conflicts, I think it might be easier for the PATCH version of this
> series to go through Jeff Kirsher's tree.
> 
> Vinicius Costa Gomes (4):
>    ethtool: Add support for configuring frame preemption
>    ethtool: Add support for configuring frame preemption via netlink
>    igc: Add support for configuring frame preemption
>    igc: Add support for exposing frame preemption stats registers
> 
>   drivers/net/ethernet/intel/igc/igc.h         |   3 +
>   drivers/net/ethernet/intel/igc/igc_defines.h |   6 +
>   drivers/net/ethernet/intel/igc/igc_ethtool.c |  77 ++++++++
>   drivers/net/ethernet/intel/igc/igc_regs.h    |  10 +
>   drivers/net/ethernet/intel/igc/igc_tsn.c     |  46 ++++-
>   include/linux/ethtool.h                      |   6 +
>   include/uapi/linux/ethtool.h                 |  25 +++
>   include/uapi/linux/ethtool_netlink.h         |  19 ++
>   net/ethtool/Makefile                         |   3 +-
>   net/ethtool/ioctl.c                          |  36 ++++
>   net/ethtool/netlink.c                        |  15 ++
>   net/ethtool/netlink.h                        |   2 +
>   net/ethtool/preempt.c                        | 181 +++++++++++++++++++
>   13 files changed, 423 insertions(+), 6 deletions(-)
>   create mode 100644 net/ethtool/preempt.c
> 

-- 
Murali Karicheri
Texas Instruments

[Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Murali Karicheri]

Hi Vinicius,

On 5/15/20 9:29 PM, Vinicius Costa Gomes wrote:
> Hi,
> 
> This series adds support for configuring frame preemption, as defined
> by IEEE 802.1Q-2018 (previously IEEE 802.1Qbu) and IEEE 802.3br.
> 
> Frame preemption allows a packet from a higher priority queue marked
> as "express" to preempt a packet from lower priority queue marked as
> "preemptible". The idea is that this can help reduce the latency for
> higher priority traffic.
> 
> Previously, the proposed interface for configuring these features was
> using the qdisc layer. But as this is very hardware dependent and all
> that qdisc did was pass the information to the driver, it makes sense
> to have this in ethtool.
> 
> One example, for retrieving and setting the configuration:
> 
> $ ethtool $ sudo ./ethtool --show-frame-preemption enp3s0
> Frame preemption settings for enp3s0:
> 	support: supported
> 	active: active
> 	supported queues: 0xf

I assume this is will be in sync with ethtool -L output which indicates
how many tx h/w queues present? I mean if there are 8 h/w queues,
supported queues will show 0xff.

> 	supported queues: 0xe
 From the command below, it appears this is the preemptible queue mask.
bit 0  is Q0, bit 1 Q1 and so forth. Right? In that case isn't it more
clear to display
         preemptible queues : 0xef

In the above Q7 is express queue and Q6-Q0 are preemptible.

Also there is a handshake called verify that happens which initiated
by the h/w to check the capability of peer. It looks like
not all vendor's hardware supports it and good to have it displayed
something like

         Verify supported/{not supported}

If Verify is supported, FPE is enabled only if it succeeds. So may be
good to show a status of Verify if it is supported something like
         Verify success/Failed

> 	minimum fragment size: 68
> 
> 
> $ ethtool --set-frame-preemption enp3s0 fp on min-frag-size 68 preemptible-queues-mask 0xe
> 
> This is a RFC because I wanted to have feedback on some points:
> 
>    - The parameters added are enough for the hardware I have, is it
>      enough in general?

As described above, it would be good to add an optional parameter for
verify

ethtool --set-frame-preemption enp3s0 fp on min-frag-size 68 
preemptible-queues-mask 0xe verify on

> 
>    - even with the ethtool via netlink effort, I chose to keep the
>      ioctl() way, in case someone wants to backport this to an older
>      kernel, is there a problem with this?
> 
>    - Some space for bikeshedding the names and location (for example,
>      does it make sense for these settings to be per-queue?), as I am
>      not quite happy with them, one example, is the use of preemptible
>      vs. preemptable;
> 
> 
> About the patches, should be quite straightforward:
> 
> Patch 1, adds the ETHTOOL_GFP and ETHOOL_SFP commands and the
> associated data structures;
> 
> Patch 2, adds the ETHTOOL_MSG_PREEMPT_GET and ETHTOOL_MSG_PREEMPT_SET
> netlink messages and the associated attributes;
> 
> Patch 3, is the example implementation for the igc driver, the catch
> here is that frame preemption in igc is dependent on the TSN "mode"
> being enabled;
> 
> Patch 4, adds some registers that helped during implementation.
> 
> Another thing is that because igc is still under development, to avoid
> conflicts, I think it might be easier for the PATCH version of this
> series to go through Jeff Kirsher's tree.
> 
> Vinicius Costa Gomes (4):
>    ethtool: Add support for configuring frame preemption
>    ethtool: Add support for configuring frame preemption via netlink
>    igc: Add support for configuring frame preemption
>    igc: Add support for exposing frame preemption stats registers
> 
>   drivers/net/ethernet/intel/igc/igc.h         |   3 +
>   drivers/net/ethernet/intel/igc/igc_defines.h |   6 +
>   drivers/net/ethernet/intel/igc/igc_ethtool.c |  77 ++++++++
>   drivers/net/ethernet/intel/igc/igc_regs.h    |  10 +
>   drivers/net/ethernet/intel/igc/igc_tsn.c     |  46 ++++-
>   include/linux/ethtool.h                      |   6 +
>   include/uapi/linux/ethtool.h                 |  25 +++
>   include/uapi/linux/ethtool_netlink.h         |  19 ++
>   net/ethtool/Makefile                         |   3 +-
>   net/ethtool/ioctl.c                          |  36 ++++
>   net/ethtool/netlink.c                        |  15 ++
>   net/ethtool/netlink.h                        |   2 +
>   net/ethtool/preempt.c                        | 181 +++++++++++++++++++
>   13 files changed, 423 insertions(+), 6 deletions(-)
>   create mode 100644 net/ethtool/preempt.c
> 

-- 
Murali Karicheri
Texas Instruments

Re: [next-queue RFC 1/4] ethtool: Add support for configuring frame preemption

[Murali Karicheri]

Hi Vinicius,

On 5/15/20 9:29 PM, Vinicius Costa Gomes wrote:
> Frame preemption (described in IEEE 802.3br-2016) defines the concept
> of preemptible and express queues. It allows traffic from express
> queues to "interrupt" traffic from preemptible queues, which are
> "resumed" after the express traffic has finished transmitting.
> 
> Frame preemption can only be used when both the local device and the
> link partner support it.
> 
> A new ethtool command was added to support the configuration
> parameters.
> 
> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> ---
>   include/linux/ethtool.h      |  6 ++++++
>   include/uapi/linux/ethtool.h | 25 +++++++++++++++++++++++++
>   net/ethtool/ioctl.c          | 36 ++++++++++++++++++++++++++++++++++++
>   3 files changed, 67 insertions(+)
> 
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index a23b26e..e4a6710 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -360,6 +360,8 @@ bool ethtool_convert_link_mode_to_legacy_u32(u32 *legacy_u32,
>    * @get_ethtool_phy_stats: Return extended statistics about the PHY device.
>    *	This is only useful if the device maintains PHY statistics and
>    *	cannot use the standard PHY library helpers.
> + * @get_preempt: Get the network device Frame Preemption parameters.
> + * @set_preempt: Set the network device Frame Preemption parameters.
>    *
>    * All operations are optional (i.e. the function pointer may be set
>    * to %NULL) and callers must take this into account.  Callers must
> @@ -454,6 +456,10 @@ struct ethtool_ops {
>   				      struct ethtool_fecparam *);
>   	int	(*set_fecparam)(struct net_device *,
>   				      struct ethtool_fecparam *);
> +	int	(*get_preempt)(struct net_device *,
> +			       struct ethtool_fp *);
> +	int	(*set_preempt)(struct net_device *,
> +			       struct ethtool_fp *);
>   	void	(*get_ethtool_phy_stats)(struct net_device *,
>   					 struct ethtool_stats *, u64 *);

I understand this series for IET Preemption. But want to see if we can
also add EST parameter, queueMaxSDU, that is also configurable.

This is defined as up to 8 entries of queueMaxSDU (unsigned int) defined
in 12.29.1.1,12.29.1.1.1 of 802.1Q 2018 edition. May be
set_queue_max_sdu() with per traffic class queue value as an array
of __u16 values?


>   };
> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> index f4662b3..d63f9f8 100644
> --- a/include/uapi/linux/ethtool.h
> +++ b/include/uapi/linux/ethtool.h
> @@ -369,6 +369,28 @@ struct ethtool_eee {
>   	__u32	reserved[2];
>   };
>   
> +/**
> + * struct ethtool_fp - Frame Preemption information
> + * @cmd: ETHTOOL_{G,S}FP
> + * @fp_supported: If frame preemption is supported.
> + * @fp_enabled: If frame preemption should be advertised to the link partner
> + *	as enabled.
> + * @supported_queues_mask: Bitmask indicating which queues support being
> + *	configured as preemptible (bit 0 -> queue 0, bit N -> queue N).
> + * @preemptible_queues_mask: Bitmask indicating which queues are
> + *	configured as preemptible (bit 0 -> queue 0, bit N -> queue N).
> + * @min_frag_size: Minimum size for all non-final fragment size.
> + */
> +struct ethtool_fp {
> +	__u32	cmd;
> +	__u8	fp_supported;
> +	__u8	fp_enabled;

Could we add verify_supported and verify_enabled?

> +	__u32	supported_queues_mask;
> +	__u32	preemptible_queues_mask;
> +	__u32	min_frag_size;
> +	__u32	reserved[2];
> +};
> +
>   /**
>    * struct ethtool_modinfo - plugin module eeprom information
>    * @cmd: %ETHTOOL_GMODULEINFO
> @@ -1441,6 +1463,9 @@ enum ethtool_fec_config_bits {
>   #define ETHTOOL_GFECPARAM	0x00000050 /* Get FEC settings */
>   #define ETHTOOL_SFECPARAM	0x00000051 /* Set FEC settings */
>   
> +#define ETHTOOL_GFP		0x00000052 /* Get Frame Preemption settings */
> +#define ETHTOOL_SFP		0x00000053 /* Set Frame Preemption settings */
> +
>   /* compatibility with older code */
>   #define SPARC_ETH_GSET		ETHTOOL_GSET
>   #define SPARC_ETH_SSET		ETHTOOL_SSET
> diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
> index 52102ab..e15ad5c 100644
> --- a/net/ethtool/ioctl.c
> +++ b/net/ethtool/ioctl.c
> @@ -2531,6 +2531,36 @@ static int ethtool_set_fecparam(struct net_device *dev, void __user *useraddr)
>   	return dev->ethtool_ops->set_fecparam(dev, &fecparam);
>   }
>   
> +static int ethtool_get_preempt(struct net_device *dev, void __user *useraddr)
> +{
> +	struct ethtool_fp fpparam = { .cmd = ETHTOOL_GFP };
> +	int rc;
> +
> +	if (!dev->ethtool_ops->get_preempt)
> +		return -EOPNOTSUPP;
> +
> +	rc = dev->ethtool_ops->get_preempt(dev, &fpparam);
> +	if (rc)
> +		return rc;
> +
> +	if (copy_to_user(useraddr, &fpparam, sizeof(fpparam)))
> +		return -EFAULT;
> +	return 0;
> +}
> +
> +static int ethtool_set_preempt(struct net_device *dev, void __user *useraddr)
> +{
> +	struct ethtool_fp fpparam;
> +
> +	if (!dev->ethtool_ops->set_preempt)
> +		return -EOPNOTSUPP;
> +
> +	if (copy_from_user(&fpparam, useraddr, sizeof(fpparam)))
> +		return -EFAULT;
> +
> +	return dev->ethtool_ops->set_preempt(dev, &fpparam);
> +}
> +
>   /* The main entry point in this file.  Called from net/core/dev_ioctl.c */
>   
>   int dev_ethtool(struct net *net, struct ifreq *ifr)
> @@ -2810,6 +2840,12 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
>   	case ETHTOOL_SFECPARAM:
>   		rc = ethtool_set_fecparam(dev, useraddr);
>   		break;
> +	case ETHTOOL_GFP:
> +		rc = ethtool_get_preempt(dev, useraddr);
> +		break;
> +	case ETHTOOL_SFP:
> +		rc = ethtool_set_preempt(dev, useraddr);
> +		break;
>   	default:
>   		rc = -EOPNOTSUPP;
>   	}
> 

-- 
Murali Karicheri
Texas Instruments

[Intel-wired-lan] [next-queue RFC 1/4] ethtool: Add support for configuring frame preemption

[Murali Karicheri]

Hi Vinicius,

On 5/15/20 9:29 PM, Vinicius Costa Gomes wrote:
> Frame preemption (described in IEEE 802.3br-2016) defines the concept
> of preemptible and express queues. It allows traffic from express
> queues to "interrupt" traffic from preemptible queues, which are
> "resumed" after the express traffic has finished transmitting.
> 
> Frame preemption can only be used when both the local device and the
> link partner support it.
> 
> A new ethtool command was added to support the configuration
> parameters.
> 
> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> ---
>   include/linux/ethtool.h      |  6 ++++++
>   include/uapi/linux/ethtool.h | 25 +++++++++++++++++++++++++
>   net/ethtool/ioctl.c          | 36 ++++++++++++++++++++++++++++++++++++
>   3 files changed, 67 insertions(+)
> 
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index a23b26e..e4a6710 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -360,6 +360,8 @@ bool ethtool_convert_link_mode_to_legacy_u32(u32 *legacy_u32,
>    * @get_ethtool_phy_stats: Return extended statistics about the PHY device.
>    *	This is only useful if the device maintains PHY statistics and
>    *	cannot use the standard PHY library helpers.
> + * @get_preempt: Get the network device Frame Preemption parameters.
> + * @set_preempt: Set the network device Frame Preemption parameters.
>    *
>    * All operations are optional (i.e. the function pointer may be set
>    * to %NULL) and callers must take this into account.  Callers must
> @@ -454,6 +456,10 @@ struct ethtool_ops {
>   				      struct ethtool_fecparam *);
>   	int	(*set_fecparam)(struct net_device *,
>   				      struct ethtool_fecparam *);
> +	int	(*get_preempt)(struct net_device *,
> +			       struct ethtool_fp *);
> +	int	(*set_preempt)(struct net_device *,
> +			       struct ethtool_fp *);
>   	void	(*get_ethtool_phy_stats)(struct net_device *,
>   					 struct ethtool_stats *, u64 *);

I understand this series for IET Preemption. But want to see if we can
also add EST parameter, queueMaxSDU, that is also configurable.

This is defined as up to 8 entries of queueMaxSDU (unsigned int) defined
in 12.29.1.1,12.29.1.1.1 of 802.1Q 2018 edition. May be
set_queue_max_sdu() with per traffic class queue value as an array
of __u16 values?


>   };
> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> index f4662b3..d63f9f8 100644
> --- a/include/uapi/linux/ethtool.h
> +++ b/include/uapi/linux/ethtool.h
> @@ -369,6 +369,28 @@ struct ethtool_eee {
>   	__u32	reserved[2];
>   };
>   
> +/**
> + * struct ethtool_fp - Frame Preemption information
> + * @cmd: ETHTOOL_{G,S}FP
> + * @fp_supported: If frame preemption is supported.
> + * @fp_enabled: If frame preemption should be advertised to the link partner
> + *	as enabled.
> + * @supported_queues_mask: Bitmask indicating which queues support being
> + *	configured as preemptible (bit 0 -> queue 0, bit N -> queue N).
> + * @preemptible_queues_mask: Bitmask indicating which queues are
> + *	configured as preemptible (bit 0 -> queue 0, bit N -> queue N).
> + * @min_frag_size: Minimum size for all non-final fragment size.
> + */
> +struct ethtool_fp {
> +	__u32	cmd;
> +	__u8	fp_supported;
> +	__u8	fp_enabled;

Could we add verify_supported and verify_enabled?

> +	__u32	supported_queues_mask;
> +	__u32	preemptible_queues_mask;
> +	__u32	min_frag_size;
> +	__u32	reserved[2];
> +};
> +
>   /**
>    * struct ethtool_modinfo - plugin module eeprom information
>    * @cmd: %ETHTOOL_GMODULEINFO
> @@ -1441,6 +1463,9 @@ enum ethtool_fec_config_bits {
>   #define ETHTOOL_GFECPARAM	0x00000050 /* Get FEC settings */
>   #define ETHTOOL_SFECPARAM	0x00000051 /* Set FEC settings */
>   
> +#define ETHTOOL_GFP		0x00000052 /* Get Frame Preemption settings */
> +#define ETHTOOL_SFP		0x00000053 /* Set Frame Preemption settings */
> +
>   /* compatibility with older code */
>   #define SPARC_ETH_GSET		ETHTOOL_GSET
>   #define SPARC_ETH_SSET		ETHTOOL_SSET
> diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
> index 52102ab..e15ad5c 100644
> --- a/net/ethtool/ioctl.c
> +++ b/net/ethtool/ioctl.c
> @@ -2531,6 +2531,36 @@ static int ethtool_set_fecparam(struct net_device *dev, void __user *useraddr)
>   	return dev->ethtool_ops->set_fecparam(dev, &fecparam);
>   }
>   
> +static int ethtool_get_preempt(struct net_device *dev, void __user *useraddr)
> +{
> +	struct ethtool_fp fpparam = { .cmd = ETHTOOL_GFP };
> +	int rc;
> +
> +	if (!dev->ethtool_ops->get_preempt)
> +		return -EOPNOTSUPP;
> +
> +	rc = dev->ethtool_ops->get_preempt(dev, &fpparam);
> +	if (rc)
> +		return rc;
> +
> +	if (copy_to_user(useraddr, &fpparam, sizeof(fpparam)))
> +		return -EFAULT;
> +	return 0;
> +}
> +
> +static int ethtool_set_preempt(struct net_device *dev, void __user *useraddr)
> +{
> +	struct ethtool_fp fpparam;
> +
> +	if (!dev->ethtool_ops->set_preempt)
> +		return -EOPNOTSUPP;
> +
> +	if (copy_from_user(&fpparam, useraddr, sizeof(fpparam)))
> +		return -EFAULT;
> +
> +	return dev->ethtool_ops->set_preempt(dev, &fpparam);
> +}
> +
>   /* The main entry point in this file.  Called from net/core/dev_ioctl.c */
>   
>   int dev_ethtool(struct net *net, struct ifreq *ifr)
> @@ -2810,6 +2840,12 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
>   	case ETHTOOL_SFECPARAM:
>   		rc = ethtool_set_fecparam(dev, useraddr);
>   		break;
> +	case ETHTOOL_GFP:
> +		rc = ethtool_get_preempt(dev, useraddr);
> +		break;
> +	case ETHTOOL_SFP:
> +		rc = ethtool_set_preempt(dev, useraddr);
> +		break;
>   	default:
>   		rc = -EOPNOTSUPP;
>   	}
> 

-- 
Murali Karicheri
Texas Instruments

Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Vinicius Costa Gomes]

Murali Karicheri <m-karicheri2@ti.com> writes:

>> $ ethtool $ sudo ./ethtool --show-frame-preemption enp3s0
>> Frame preemption settings for enp3s0:
>> 	support: supported
>> 	active: active
>> 	supported queues: 0xf
>
> I assume this is will be in sync with ethtool -L output which indicates
> how many tx h/w queues present? I mean if there are 8 h/w queues,
> supported queues will show 0xff.

In this approach, the driver builds these bitmasks, so it's responsible
to keep it consistent with the rest of the stuff that's exposed in
ethtool.

>
>> 	supported queues: 0xe
>  From the command below, it appears this is the preemptible queue mask.
> bit 0  is Q0, bit 1 Q1 and so forth. Right? In that case isn't it more
> clear to display
>          preemptible queues : 0xef
>
> In the above Q7 is express queue and Q6-Q0 are preemptible.

In my case, the controller I have here only has 4 queues, and Queue 0 is
the highest priority one, and it's marked as express.

>
> Also there is a handshake called verify that happens which initiated
> by the h/w to check the capability of peer. It looks like
> not all vendor's hardware supports it and good to have it displayed
> something like
>
>          Verify supported/{not supported}
>
> If Verify is supported, FPE is enabled only if it succeeds. So may be
> good to show a status of Verify if it is supported something like
>          Verify success/Failed
>
>> 	minimum fragment size: 68
>> 
>> 
>> $ ethtool --set-frame-preemption enp3s0 fp on min-frag-size 68 preemptible-queues-mask 0xe
>> 
>> This is a RFC because I wanted to have feedback on some points:
>> 
>>    - The parameters added are enough for the hardware I have, is it
>>      enough in general?
>
> As described above, it would be good to add an optional parameter for
> verify
>
> ethtool --set-frame-preemption enp3s0 fp on min-frag-size 68 
> preemptible-queues-mask 0xe verify on
>

The hardware I have do not support this, but this makes sense.


Cheers,
-- 
Vinicius

[Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Vinicius Costa Gomes]

Murali Karicheri <m-karicheri2@ti.com> writes:

>> $ ethtool $ sudo ./ethtool --show-frame-preemption enp3s0
>> Frame preemption settings for enp3s0:
>> 	support: supported
>> 	active: active
>> 	supported queues: 0xf
>
> I assume this is will be in sync with ethtool -L output which indicates
> how many tx h/w queues present? I mean if there are 8 h/w queues,
> supported queues will show 0xff.

In this approach, the driver builds these bitmasks, so it's responsible
to keep it consistent with the rest of the stuff that's exposed in
ethtool.

>
>> 	supported queues: 0xe
>  From the command below, it appears this is the preemptible queue mask.
> bit 0  is Q0, bit 1 Q1 and so forth. Right? In that case isn't it more
> clear to display
>          preemptible queues : 0xef
>
> In the above Q7 is express queue and Q6-Q0 are preemptible.

In my case, the controller I have here only has 4 queues, and Queue 0 is
the highest priority one, and it's marked as express.

>
> Also there is a handshake called verify that happens which initiated
> by the h/w to check the capability of peer. It looks like
> not all vendor's hardware supports it and good to have it displayed
> something like
>
>          Verify supported/{not supported}
>
> If Verify is supported, FPE is enabled only if it succeeds. So may be
> good to show a status of Verify if it is supported something like
>          Verify success/Failed
>
>> 	minimum fragment size: 68
>> 
>> 
>> $ ethtool --set-frame-preemption enp3s0 fp on min-frag-size 68 preemptible-queues-mask 0xe
>> 
>> This is a RFC because I wanted to have feedback on some points:
>> 
>>    - The parameters added are enough for the hardware I have, is it
>>      enough in general?
>
> As described above, it would be good to add an optional parameter for
> verify
>
> ethtool --set-frame-preemption enp3s0 fp on min-frag-size 68 
> preemptible-queues-mask 0xe verify on
>

The hardware I have do not support this, but this makes sense.


Cheers,
-- 
Vinicius

Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Murali Karicheri]

Hi Vinicius,

On 5/19/20 11:32 AM, Vinicius Costa Gomes wrote:
> Murali Karicheri <m-karicheri2@ti.com> writes:
> 
>>> $ ethtool $ sudo ./ethtool --show-frame-preemption enp3s0
>>> Frame preemption settings for enp3s0:
>>> 	support: supported
>>> 	active: active
>>> 	supported queues: 0xf
>>
>> I assume this is will be in sync with ethtool -L output which indicates
>> how many tx h/w queues present? I mean if there are 8 h/w queues,
>> supported queues will show 0xff.
> 
> In this approach, the driver builds these bitmasks, so it's responsible
> to keep it consistent with the rest of the stuff that's exposed in
> ethtool.
Agree
> 
>>
>>> 	supported queues: 0xe
>>   From the command below, it appears this is the preemptible queue mask.
>> bit 0  is Q0, bit 1 Q1 and so forth. Right? In that case isn't it more
>> clear to display
>>           preemptible queues : 0xef
>>
>> In the above Q7 is express queue and Q6-Q0 are preemptible.
> 
> In my case, the controller I have here only has 4 queues, and Queue 0 is
> the highest priority one, and it's marked as express.
> 

Ok. So it is up to the device driver to manage this.

>>
>> Also there is a handshake called verify that happens which initiated
>> by the h/w to check the capability of peer. It looks like
>> not all vendor's hardware supports it and good to have it displayed
>> something like
>>
>>           Verify supported/{not supported}
>>
>> If Verify is supported, FPE is enabled only if it succeeds. So may be
>> good to show a status of Verify if it is supported something like
>>           Verify success/Failed
>>
>>> 	minimum fragment size: 68
>>>
>>>
>>> $ ethtool --set-frame-preemption enp3s0 fp on min-frag-size 68 preemptible-queues-mask 0xe
>>>
>>> This is a RFC because I wanted to have feedback on some points:
>>>
>>>     - The parameters added are enough for the hardware I have, is it
>>>       enough in general?
>>
>> As described above, it would be good to add an optional parameter for
>> verify
>>
>> ethtool --set-frame-preemption enp3s0 fp on min-frag-size 68
>> preemptible-queues-mask 0xe verify on
>>
> 
> The hardware I have do not support this, but this makes sense.

I can work to support this for TI AM65 CPSW once a formal patch is
available. AM65 CPSW supports Verify as an optional feature. H/w works
also in manual mode where it is assumed that it is connected to a
IET FPE capable partner, but can't do Verify.

Thanks for sending the RFC.

regards,

Murali
> 
> 
> Cheers,
> 

-- 
Murali Karicheri
Texas Instruments

[Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Murali Karicheri]

Hi Vinicius,

On 5/19/20 11:32 AM, Vinicius Costa Gomes wrote:
> Murali Karicheri <m-karicheri2@ti.com> writes:
> 
>>> $ ethtool $ sudo ./ethtool --show-frame-preemption enp3s0
>>> Frame preemption settings for enp3s0:
>>> 	support: supported
>>> 	active: active
>>> 	supported queues: 0xf
>>
>> I assume this is will be in sync with ethtool -L output which indicates
>> how many tx h/w queues present? I mean if there are 8 h/w queues,
>> supported queues will show 0xff.
> 
> In this approach, the driver builds these bitmasks, so it's responsible
> to keep it consistent with the rest of the stuff that's exposed in
> ethtool.
Agree
> 
>>
>>> 	supported queues: 0xe
>>   From the command below, it appears this is the preemptible queue mask.
>> bit 0  is Q0, bit 1 Q1 and so forth. Right? In that case isn't it more
>> clear to display
>>           preemptible queues : 0xef
>>
>> In the above Q7 is express queue and Q6-Q0 are preemptible.
> 
> In my case, the controller I have here only has 4 queues, and Queue 0 is
> the highest priority one, and it's marked as express.
> 

Ok. So it is up to the device driver to manage this.

>>
>> Also there is a handshake called verify that happens which initiated
>> by the h/w to check the capability of peer. It looks like
>> not all vendor's hardware supports it and good to have it displayed
>> something like
>>
>>           Verify supported/{not supported}
>>
>> If Verify is supported, FPE is enabled only if it succeeds. So may be
>> good to show a status of Verify if it is supported something like
>>           Verify success/Failed
>>
>>> 	minimum fragment size: 68
>>>
>>>
>>> $ ethtool --set-frame-preemption enp3s0 fp on min-frag-size 68 preemptible-queues-mask 0xe
>>>
>>> This is a RFC because I wanted to have feedback on some points:
>>>
>>>     - The parameters added are enough for the hardware I have, is it
>>>       enough in general?
>>
>> As described above, it would be good to add an optional parameter for
>> verify
>>
>> ethtool --set-frame-preemption enp3s0 fp on min-frag-size 68
>> preemptible-queues-mask 0xe verify on
>>
> 
> The hardware I have do not support this, but this makes sense.

I can work to support this for TI AM65 CPSW once a formal patch is
available. AM65 CPSW supports Verify as an optional feature. H/w works
also in manual mode where it is assumed that it is connected to a
IET FPE capable partner, but can't do Verify.

Thanks for sending the RFC.

regards,

Murali
> 
> 
> Cheers,
> 

-- 
Murali Karicheri
Texas Instruments

Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Murali Karicheri]

On 5/18/20 6:06 PM, Vinicius Costa Gomes wrote:
> Hi,
> 
> Jakub Kicinski <kuba@kernel.org> writes:
>>
>> Please take a look at the example from the cover letter:
>>
>> $ ethtool $ sudo ./ethtool --show-frame-preemption
>> enp3s0 Frame preemption settings for enp3s0:
>> 	support: supported
>> 	active: active
>> 	supported queues: 0xf
>> 	supported queues: 0xe
>> 	minimum fragment size: 68
>>
>> Reading this I have no idea what 0xe is. I have to go and query TC API
>> to see what priorities and queues that will be. Which IMHO is a strong
>> argument that this information belongs there in the first place.
> 
> That was the (only?) strong argument in favor of having frame preemption
> in the TC side when this was last discussed.
> 
> We can have a hybrid solution, we can move the express/preemptible per
> queue map to mqprio/taprio/whatever. And have the more specific
> configuration knobs, minimum fragment size, etc, in ethtool.

Isn't this a pure h/w feature? FPE is implemented at L2 and involves
fragments that are only seen by h/w and never at Linux network core
unlike IP fragments and is transparent to network stack. However it
enhances priority handling at h/w to the next level by pre-empting 
existing lower priority traffic to give way to express queue traffic
and improve latency. So everything happens in h/w. So ethtool makes
perfect sense here as it is a queue configuration. I agree with Vinicius
and Vladmir to support this in ethtool instead of TC.

Murali
> 
> What do you think?
> 
> 
> Cheers,
> 

-- 
Murali Karicheri
Texas Instruments

[Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Murali Karicheri]

On 5/18/20 6:06 PM, Vinicius Costa Gomes wrote:
> Hi,
> 
> Jakub Kicinski <kuba@kernel.org> writes:
>>
>> Please take a look at the example from the cover letter:
>>
>> $ ethtool $ sudo ./ethtool --show-frame-preemption
>> enp3s0 Frame preemption settings for enp3s0:
>> 	support: supported
>> 	active: active
>> 	supported queues: 0xf
>> 	supported queues: 0xe
>> 	minimum fragment size: 68
>>
>> Reading this I have no idea what 0xe is. I have to go and query TC API
>> to see what priorities and queues that will be. Which IMHO is a strong
>> argument that this information belongs there in the first place.
> 
> That was the (only?) strong argument in favor of having frame preemption
> in the TC side when this was last discussed.
> 
> We can have a hybrid solution, we can move the express/preemptible per
> queue map to mqprio/taprio/whatever. And have the more specific
> configuration knobs, minimum fragment size, etc, in ethtool.

Isn't this a pure h/w feature? FPE is implemented at L2 and involves
fragments that are only seen by h/w and never at Linux network core
unlike IP fragments and is transparent to network stack. However it
enhances priority handling at h/w to the next level by pre-empting 
existing lower priority traffic to give way to express queue traffic
and improve latency. So everything happens in h/w. So ethtool makes
perfect sense here as it is a queue configuration. I agree with Vinicius
and Vladmir to support this in ethtool instead of TC.

Murali
> 
> What do you think?
> 
> 
> Cheers,
> 

-- 
Murali Karicheri
Texas Instruments

Re: [next-queue RFC 3/4] igc: Add support for configuring frame preemption

[Murali Karicheri]

Hi Vinicius,

On 5/15/20 9:29 PM, Vinicius Costa Gomes wrote:
> WIP
> 
- cut -
>   
>   /* forward declaration */
>   struct igc_stats {
> @@ -1549,6 +1550,71 @@ static int igc_ethtool_set_priv_flags(struct net_device *netdev, u32 priv_flags)
>   	return 0;
>   }
>   
> +static int igc_ethtool_get_preempt(struct net_device *netdev,
> +				   struct ethtool_fp *fpcmd)
> +{
> +	struct igc_adapter *adapter = netdev_priv(netdev);
> +	int i;
> +
> +	fpcmd->fp_supported = 1;
> +	fpcmd->fp_enabled = adapter->frame_preemption_active;
> +	fpcmd->min_frag_size = adapter->min_frag_size;
> +
> +	for (i = 0; i < adapter->num_tx_queues; i++) {
> +		struct igc_ring *ring = adapter->tx_ring[i];
> +
> +		fpcmd->supported_queues_mask |= BIT(i);
> +
> +		if (ring->preemptible)
> +			fpcmd->preemptible_queues_mask |= BIT(i);
> +	}
> +
> +	return 0;
> +}
> +
Is this something that can be provided by the driver when netdevice
is registered so that a common function at net core layer be
implemented instead of duplicating this in individual device drivers?

> +static int igc_ethtool_set_preempt(struct net_device *netdev,
> +				   struct ethtool_fp *fpcmd)
> +{
> +	struct igc_adapter *adapter = netdev_priv(netdev);
> +	int i;
> +
> +	/* The formula is (Section 8.12.4 of the datasheet):
> +	 *   MIN_FRAG_SIZE = 4 + (1 + MIN_FRAG)*64
> +	 * MIN_FRAG is represented by two bits, so we can only have
> +	 * min_frag_size between 68 and 260.
> +	 */
> +	if (fpcmd->min_frag_size < 68 || fpcmd->min_frag_size > 260)
> +		return -EINVAL;

- cut-

-- 
Murali Karicheri
Texas Instruments

[Intel-wired-lan] [next-queue RFC 3/4] igc: Add support for configuring frame preemption

[Murali Karicheri]

Hi Vinicius,

On 5/15/20 9:29 PM, Vinicius Costa Gomes wrote:
> WIP
> 
- cut -
>   
>   /* forward declaration */
>   struct igc_stats {
> @@ -1549,6 +1550,71 @@ static int igc_ethtool_set_priv_flags(struct net_device *netdev, u32 priv_flags)
>   	return 0;
>   }
>   
> +static int igc_ethtool_get_preempt(struct net_device *netdev,
> +				   struct ethtool_fp *fpcmd)
> +{
> +	struct igc_adapter *adapter = netdev_priv(netdev);
> +	int i;
> +
> +	fpcmd->fp_supported = 1;
> +	fpcmd->fp_enabled = adapter->frame_preemption_active;
> +	fpcmd->min_frag_size = adapter->min_frag_size;
> +
> +	for (i = 0; i < adapter->num_tx_queues; i++) {
> +		struct igc_ring *ring = adapter->tx_ring[i];
> +
> +		fpcmd->supported_queues_mask |= BIT(i);
> +
> +		if (ring->preemptible)
> +			fpcmd->preemptible_queues_mask |= BIT(i);
> +	}
> +
> +	return 0;
> +}
> +
Is this something that can be provided by the driver when netdevice
is registered so that a common function at net core layer be
implemented instead of duplicating this in individual device drivers?

> +static int igc_ethtool_set_preempt(struct net_device *netdev,
> +				   struct ethtool_fp *fpcmd)
> +{
> +	struct igc_adapter *adapter = netdev_priv(netdev);
> +	int i;
> +
> +	/* The formula is (Section 8.12.4 of the datasheet):
> +	 *   MIN_FRAG_SIZE = 4 + (1 + MIN_FRAG)*64
> +	 * MIN_FRAG is represented by two bits, so we can only have
> +	 * min_frag_size between 68 and 260.
> +	 */
> +	if (fpcmd->min_frag_size < 68 || fpcmd->min_frag_size > 260)
> +		return -EINVAL;

- cut-

-- 
Murali Karicheri
Texas Instruments

Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Vinicius Costa Gomes]

Murali Karicheri <m-karicheri2@ti.com> writes:

>> That was the (only?) strong argument in favor of having frame preemption
>> in the TC side when this was last discussed.
>> 
>> We can have a hybrid solution, we can move the express/preemptible per
>> queue map to mqprio/taprio/whatever. And have the more specific
>> configuration knobs, minimum fragment size, etc, in ethtool.
>
> Isn't this a pure h/w feature? FPE is implemented at L2 and involves
> fragments that are only seen by h/w and never at Linux network core
> unlike IP fragments and is transparent to network stack. However it
> enhances priority handling at h/w to the next level by pre-empting 
> existing lower priority traffic to give way to express queue traffic
> and improve latency. So everything happens in h/w. So ethtool makes
> perfect sense here as it is a queue configuration. I agree with Vinicius
> and Vladmir to support this in ethtool instead of TC.

The way I see, the issue that Jakub is pointing here is more of
usability/understandability.

By having the express/preemptible queue mapping in TC, we have the
configuration near where the "priority to queue" mapping happens. That
improves the ease of configuration, makes it easier to spot mistakes,
that kind of thing, all of which are a big plus.

Right now, I am seeing this hybrid approach as a good compromise, we
have the queue settings near to where the kinds of traffic are mapped to
queues, and we have the rest of the hardware configuration in ethtool.


Cheers,
-- 
Vinicius

[Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Vinicius Costa Gomes]

Murali Karicheri <m-karicheri2@ti.com> writes:

>> That was the (only?) strong argument in favor of having frame preemption
>> in the TC side when this was last discussed.
>> 
>> We can have a hybrid solution, we can move the express/preemptible per
>> queue map to mqprio/taprio/whatever. And have the more specific
>> configuration knobs, minimum fragment size, etc, in ethtool.
>
> Isn't this a pure h/w feature? FPE is implemented at L2 and involves
> fragments that are only seen by h/w and never at Linux network core
> unlike IP fragments and is transparent to network stack. However it
> enhances priority handling at h/w to the next level by pre-empting 
> existing lower priority traffic to give way to express queue traffic
> and improve latency. So everything happens in h/w. So ethtool makes
> perfect sense here as it is a queue configuration. I agree with Vinicius
> and Vladmir to support this in ethtool instead of TC.

The way I see, the issue that Jakub is pointing here is more of
usability/understandability.

By having the express/preemptible queue mapping in TC, we have the
configuration near where the "priority to queue" mapping happens. That
improves the ease of configuration, makes it easier to spot mistakes,
that kind of thing, all of which are a big plus.

Right now, I am seeing this hybrid approach as a good compromise, we
have the queue settings near to where the kinds of traffic are mapped to
queues, and we have the rest of the hardware configuration in ethtool.


Cheers,
-- 
Vinicius

Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Michael Walle]

Hi,

Am 2020-05-18 15:36, schrieb Murali Karicheri:
> Hi,
> 
> On 5/17/20 11:06 AM, Michael Walle wrote:
>> What about the Qbu handshake state? And some NICs support overriding
>> this. I.e. enable frame preemption even if the handshake wasn't
>> successful.
> 
> You are talking about Verify procedure to hand shake with peer to
> know if remote support IET fragmentation and re-assembly?
yes

> If yes, this manual mode of provisioning is required as well. So
> one optional parameter needed is enable-verify. If that is not
> enabled then device assumes the remote is capable of fragmentation
> and re-assembly.

sounds good.

-michael

Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Andre Guedes]

Hi,

Quoting Vinicius Costa Gomes (2020-05-15 18:29:44)
> One example, for retrieving and setting the configuration:
> 
> $ ethtool $ sudo ./ethtool --show-frame-preemption enp3s0
> Frame preemption settings for enp3s0:
>         support: supported
>         active: active

IIUC the code in patch 2, 'active' is the actual configuration knob that
enables or disables the FP functionality on the NIC.

That sounded a bit confusing to me since the spec uses the term 'active' to
indicate FP is currently enabled at both ends, and it is a read-only
information (see 12.30.1.4 from IEEE 802.1Q-2018). Maybe if we called this
'enabled' it would be more clear.

>         supported queues: 0xf
>         supported queues: 0xe
>         minimum fragment size: 68

I'm assuming this is the configuration knob for the minimal non-final fragment
defined in 802.3br.

My understanding from the specs is that this value must be a multiple from 64
and cannot assume arbitrary values like shown here. See 99.4.7.3 from IEEE
802.3 and Note 1 in S.2 from IEEE 802.1Q. In the previous discussion about FP,
we had this as a multiplier factor, not absolute value.

Regards,

Andre

[Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Andre Guedes]

Hi,

Quoting Vinicius Costa Gomes (2020-05-15 18:29:44)
> One example, for retrieving and setting the configuration:
> 
> $ ethtool $ sudo ./ethtool --show-frame-preemption enp3s0
> Frame preemption settings for enp3s0:
>         support: supported
>         active: active

IIUC the code in patch 2, 'active' is the actual configuration knob that
enables or disables the FP functionality on the NIC.

That sounded a bit confusing to me since the spec uses the term 'active' to
indicate FP is currently enabled at both ends, and it is a read-only
information (see 12.30.1.4 from IEEE 802.1Q-2018). Maybe if we called this
'enabled' it would be more clear.

>         supported queues: 0xf
>         supported queues: 0xe
>         minimum fragment size: 68

I'm assuming this is the configuration knob for the minimal non-final fragment
defined in 802.3br.

My understanding from the specs is that this value must be a multiple from 64
and cannot assume arbitrary values like shown here. See 99.4.7.3 from IEEE
802.3 and Note 1 in S.2 from IEEE 802.1Q. In the previous discussion about FP,
we had this as a multiplier factor, not absolute value.

Regards,

Andre

Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Andre Guedes]

Hi,

Quoting Vinicius Costa Gomes (2020-05-18 12:34:22)
> Hi,
> 
> Michal Kubecek <mkubecek@suse.cz> writes:
> 
> > On Fri, May 15, 2020 at 06:29:44PM -0700, Vinicius Costa Gomes wrote:
> >> Hi,
> >> 
> >> This series adds support for configuring frame preemption, as defined
> >> by IEEE 802.1Q-2018 (previously IEEE 802.1Qbu) and IEEE 802.3br.
> >> 
> >> Frame preemption allows a packet from a higher priority queue marked
> >> as "express" to preempt a packet from lower priority queue marked as
> >> "preemptible". The idea is that this can help reduce the latency for
> >> higher priority traffic.
> >> 
> >> Previously, the proposed interface for configuring these features was
> >> using the qdisc layer. But as this is very hardware dependent and all
> >> that qdisc did was pass the information to the driver, it makes sense
> >> to have this in ethtool.
> >> 
> >> One example, for retrieving and setting the configuration:
> >> 
> >> $ ethtool $ sudo ./ethtool --show-frame-preemption enp3s0
> >> Frame preemption settings for enp3s0:
> >>      support: supported
> >
> > IMHO we don't need a special bool for this. IIUC this is not a state
> > flag that would change value for a particular device; either the device
> > supports the feature or it does not. If it does not, the ethtool_ops
> > callbacks would return -EOPNOTSUPP (or would not even exist if the
> > driver has no support) and ethtool would say so.
> 
> (I know that the comments below only apply if "ethtool-way" is what's
> decided)
> 
> Cool. Will remove the supported bit.
> 
> >
> >>      active: active
> >>      supported queues: 0xf

Following the same rationale, is this 'supported queue' going aways as well?

- Andre

[Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Andre Guedes]

Hi,

Quoting Vinicius Costa Gomes (2020-05-18 12:34:22)
> Hi,
> 
> Michal Kubecek <mkubecek@suse.cz> writes:
> 
> > On Fri, May 15, 2020 at 06:29:44PM -0700, Vinicius Costa Gomes wrote:
> >> Hi,
> >> 
> >> This series adds support for configuring frame preemption, as defined
> >> by IEEE 802.1Q-2018 (previously IEEE 802.1Qbu) and IEEE 802.3br.
> >> 
> >> Frame preemption allows a packet from a higher priority queue marked
> >> as "express" to preempt a packet from lower priority queue marked as
> >> "preemptible". The idea is that this can help reduce the latency for
> >> higher priority traffic.
> >> 
> >> Previously, the proposed interface for configuring these features was
> >> using the qdisc layer. But as this is very hardware dependent and all
> >> that qdisc did was pass the information to the driver, it makes sense
> >> to have this in ethtool.
> >> 
> >> One example, for retrieving and setting the configuration:
> >> 
> >> $ ethtool $ sudo ./ethtool --show-frame-preemption enp3s0
> >> Frame preemption settings for enp3s0:
> >>      support: supported
> >
> > IMHO we don't need a special bool for this. IIUC this is not a state
> > flag that would change value for a particular device; either the device
> > supports the feature or it does not. If it does not, the ethtool_ops
> > callbacks would return -EOPNOTSUPP (or would not even exist if the
> > driver has no support) and ethtool would say so.
> 
> (I know that the comments below only apply if "ethtool-way" is what's
> decided)
> 
> Cool. Will remove the supported bit.
> 
> >
> >>      active: active
> >>      supported queues: 0xf

Following the same rationale, is this 'supported queue' going aways as well?

- Andre

Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Vinicius Costa Gomes]

Andre Guedes <andre.guedes@intel.com> writes:

>> >
>> >>      active: active
>> >>      supported queues: 0xf
>
> Following the same rationale, is this 'supported queue' going aways as well?
>

I think so, with good error messages, when trying to set an express-only
queue as preemptible, no need to expose this information.


-- 
Vinicius

[Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Vinicius Costa Gomes]

Andre Guedes <andre.guedes@intel.com> writes:

>> >
>> >>      active: active
>> >>      supported queues: 0xf
>
> Following the same rationale, is this 'supported queue' going aways as well?
>

I think so, with good error messages, when trying to set an express-only
queue as preemptible, no need to expose this information.


-- 
Vinicius

Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Vinicius Costa Gomes]

Andre Guedes <andre.guedes@intel.com> writes:

> Hi,
>
> Quoting Vinicius Costa Gomes (2020-05-15 18:29:44)
>> One example, for retrieving and setting the configuration:
>> 
>> $ ethtool $ sudo ./ethtool --show-frame-preemption enp3s0
>> Frame preemption settings for enp3s0:
>>         support: supported
>>         active: active
>
> IIUC the code in patch 2, 'active' is the actual configuration knob that
> enables or disables the FP functionality on the NIC.
>
> That sounded a bit confusing to me since the spec uses the term 'active' to
> indicate FP is currently enabled at both ends, and it is a read-only
> information (see 12.30.1.4 from IEEE 802.1Q-2018). Maybe if we called this
> 'enabled' it would be more clear.

Good point. Will rename this to "enabled".

>
>>         supported queues: 0xf
>>         supported queues: 0xe
>>         minimum fragment size: 68
>
> I'm assuming this is the configuration knob for the minimal non-final fragment
> defined in 802.3br.
>
> My understanding from the specs is that this value must be a multiple from 64
> and cannot assume arbitrary values like shown here. See 99.4.7.3 from IEEE
> 802.3 and Note 1 in S.2 from IEEE 802.1Q. In the previous discussion about FP,
> we had this as a multiplier factor, not absolute value.

I thought that exposing this as "(1 + N)*64" (with 0 <= N <= 3) that it
was more related to what's exposed via LLDP than the actual capabilities
of the hardware. And for the hardware I have actually the values
supported are: (1 + N)*64 + 4 (for N = 0, 1, 2, 3).

So I thought I was better to let the driver decide what values are
acceptable.

This is a good question for people working with other hardware.


-- 
Vinicius

[Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Vinicius Costa Gomes]

Andre Guedes <andre.guedes@intel.com> writes:

> Hi,
>
> Quoting Vinicius Costa Gomes (2020-05-15 18:29:44)
>> One example, for retrieving and setting the configuration:
>> 
>> $ ethtool $ sudo ./ethtool --show-frame-preemption enp3s0
>> Frame preemption settings for enp3s0:
>>         support: supported
>>         active: active
>
> IIUC the code in patch 2, 'active' is the actual configuration knob that
> enables or disables the FP functionality on the NIC.
>
> That sounded a bit confusing to me since the spec uses the term 'active' to
> indicate FP is currently enabled at both ends, and it is a read-only
> information (see 12.30.1.4 from IEEE 802.1Q-2018). Maybe if we called this
> 'enabled' it would be more clear.

Good point. Will rename this to "enabled".

>
>>         supported queues: 0xf
>>         supported queues: 0xe
>>         minimum fragment size: 68
>
> I'm assuming this is the configuration knob for the minimal non-final fragment
> defined in 802.3br.
>
> My understanding from the specs is that this value must be a multiple from 64
> and cannot assume arbitrary values like shown here. See 99.4.7.3 from IEEE
> 802.3 and Note 1 in S.2 from IEEE 802.1Q. In the previous discussion about FP,
> we had this as a multiplier factor, not absolute value.

I thought that exposing this as "(1 + N)*64" (with 0 <= N <= 3) that it
was more related to what's exposed via LLDP than the actual capabilities
of the hardware. And for the hardware I have actually the values
supported are: (1 + N)*64 + 4 (for N = 0, 1, 2, 3).

So I thought I was better to let the driver decide what values are
acceptable.

This is a good question for people working with other hardware.


-- 
Vinicius

Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Murali Karicheri]

Hi Vinicius,

On 5/19/20 7:37 PM, Vinicius Costa Gomes wrote:
> Andre Guedes <andre.guedes@intel.com> writes:
> 
>> Hi,
>>
>> Quoting Vinicius Costa Gomes (2020-05-15 18:29:44)
>>> One example, for retrieving and setting the configuration:
>>>
>>> $ ethtool $ sudo ./ethtool --show-frame-preemption enp3s0
>>> Frame preemption settings for enp3s0:
>>>          support: supported
>>>          active: active
>>
>> IIUC the code in patch 2, 'active' is the actual configuration knob that
>> enables or disables the FP functionality on the NIC.
>>
>> That sounded a bit confusing to me since the spec uses the term 'active' to
>> indicate FP is currently enabled at both ends, and it is a read-only
>> information (see 12.30.1.4 from IEEE 802.1Q-2018). Maybe if we called this
>> 'enabled' it would be more clear.
> 
> Good point. Will rename this to "enabled".
> 
>>
>>>          supported queues: 0xf
>>>          supported queues: 0xe
>>>          minimum fragment size: 68
>>
>> I'm assuming this is the configuration knob for the minimal non-final fragment
>> defined in 802.3br.
>>
>> My understanding from the specs is that this value must be a multiple from 64
>> and cannot assume arbitrary values like shown here. See 99.4.7.3 from IEEE
>> 802.3 and Note 1 in S.2 from IEEE 802.1Q. In the previous discussion about FP,
>> we had this as a multiplier factor, not absolute value.
> 
> I thought that exposing this as "(1 + N)*64" (with 0 <= N <= 3) that it
> was more related to what's exposed via LLDP than the actual capabilities
> of the hardware. And for the hardware I have actually the values
> supported are: (1 + N)*64 + 4 (for N = 0, 1, 2, 3).
> 
> So I thought I was better to let the driver decide what values are
> acceptable.
> 
> This is a good question for people working with other hardware.
> 
> 
AM65 CPSW supports this as a multiple of 64. Since this ethtool
configuration is for the hardware, might want to make it as a multiple
of 64.

Murali

-- 
Murali Karicheri
Texas Instruments

[Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Murali Karicheri]

Hi Vinicius,

On 5/19/20 7:37 PM, Vinicius Costa Gomes wrote:
> Andre Guedes <andre.guedes@intel.com> writes:
> 
>> Hi,
>>
>> Quoting Vinicius Costa Gomes (2020-05-15 18:29:44)
>>> One example, for retrieving and setting the configuration:
>>>
>>> $ ethtool $ sudo ./ethtool --show-frame-preemption enp3s0
>>> Frame preemption settings for enp3s0:
>>>          support: supported
>>>          active: active
>>
>> IIUC the code in patch 2, 'active' is the actual configuration knob that
>> enables or disables the FP functionality on the NIC.
>>
>> That sounded a bit confusing to me since the spec uses the term 'active' to
>> indicate FP is currently enabled at both ends, and it is a read-only
>> information (see 12.30.1.4 from IEEE 802.1Q-2018). Maybe if we called this
>> 'enabled' it would be more clear.
> 
> Good point. Will rename this to "enabled".
> 
>>
>>>          supported queues: 0xf
>>>          supported queues: 0xe
>>>          minimum fragment size: 68
>>
>> I'm assuming this is the configuration knob for the minimal non-final fragment
>> defined in 802.3br.
>>
>> My understanding from the specs is that this value must be a multiple from 64
>> and cannot assume arbitrary values like shown here. See 99.4.7.3 from IEEE
>> 802.3 and Note 1 in S.2 from IEEE 802.1Q. In the previous discussion about FP,
>> we had this as a multiplier factor, not absolute value.
> 
> I thought that exposing this as "(1 + N)*64" (with 0 <= N <= 3) that it
> was more related to what's exposed via LLDP than the actual capabilities
> of the hardware. And for the hardware I have actually the values
> supported are: (1 + N)*64 + 4 (for N = 0, 1, 2, 3).
> 
> So I thought I was better to let the driver decide what values are
> acceptable.
> 
> This is a good question for people working with other hardware.
> 
> 
AM65 CPSW supports this as a multiple of 64. Since this ethtool
configuration is for the hardware, might want to make it as a multiple
of 64.

Murali

-- 
Murali Karicheri
Texas Instruments

Re: [next-queue RFC 4/4] igc: Add support for exposing frame preemption stats registers

[Murali Karicheri]

Hi Vinicious,

On 5/15/20 9:29 PM, Vinicius Costa Gomes wrote:
> [WIP]
> 
> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> ---
>   drivers/net/ethernet/intel/igc/igc_ethtool.c |  9 +++++++++
>   drivers/net/ethernet/intel/igc/igc_regs.h    | 10 ++++++++++
>   2 files changed, 19 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
> index 48d5d18..09d72f7 100644
> --- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
> +++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
> @@ -322,6 +322,15 @@ static void igc_ethtool_get_regs(struct net_device *netdev,
>   
>   	for (i = 0; i < 8; i++)
>   		regs_buff[205 + i] = rd32(IGC_ETQF(i));
> +
> +	regs_buff[214] = rd32(IGC_PRMPTDTCNT);
> +	regs_buff[215] = rd32(IGC_PRMEVNTTCNT);
> +	regs_buff[216] = rd32(IGC_PRMPTDRCNT);
> +	regs_buff[217] = rd32(IGC_PRMEVNTRCNT);
> +	regs_buff[218] = rd32(IGC_PRMPBLTCNT);
> +	regs_buff[219] = rd32(IGC_PRMPBLRCNT);
> +	regs_buff[220] = rd32(IGC_PRMEXPTCNT);
> +	regs_buff[221] = rd32(IGC_PRMEXPRCNT);
>   }
>   
>   static void igc_ethtool_get_wol(struct net_device *netdev,
> diff --git a/drivers/net/ethernet/intel/igc/igc_regs.h b/drivers/net/ethernet/intel/igc/igc_regs.h
> index 7f999cf..010bb48 100644
> --- a/drivers/net/ethernet/intel/igc/igc_regs.h
> +++ b/drivers/net/ethernet/intel/igc/igc_regs.h
> @@ -211,6 +211,16 @@
>   
>   #define IGC_FTQF(_n)	(0x059E0 + (4 * (_n)))  /* 5-tuple Queue Fltr */
>   
> +/* Time sync registers - preemption statistics */
> +#define IGC_PRMPTDTCNT	0x04280  /* Good TX Preempted Packets */
> +#define IGC_PRMEVNTTCNT	0x04298  /* TX Preemption event counter */
> +#define IGC_PRMPTDRCNT	0x04284  /* Good RX Preempted Packets */
> +#define IGC_PRMEVNTRCNT	0x0429C  /* RX Preemption event counter */
> +#define IGC_PRMPBLTCNT	0x04288  /* Good TX Preemptable Packets */
> +#define IGC_PRMPBLRCNT	0x0428C  /* Good RX Preemptable Packets */
> +#define IGC_PRMEXPTCNT	0x04290  /* Good TX Express Packets */
> +#define IGC_PRMEXPRCNT	0x042A0  /* Preemption Exception Counter */
> +
>   /* Transmit Scheduling Registers */
>   #define IGC_TQAVCTRL		0x3570
>   #define IGC_TXQCTL(_n)		(0x3344 + 0x4 * (_n))
> 
There are some statistics supported by AM65 CPSW as well. So do you plan
to add this to ethtool stats that is dumped using ethtool -S option?

Thanks and regards,
-- 
Murali Karicheri
Texas Instruments

[Intel-wired-lan] [next-queue RFC 4/4] igc: Add support for exposing frame preemption stats registers

[Murali Karicheri]

Hi Vinicious,

On 5/15/20 9:29 PM, Vinicius Costa Gomes wrote:
> [WIP]
> 
> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> ---
>   drivers/net/ethernet/intel/igc/igc_ethtool.c |  9 +++++++++
>   drivers/net/ethernet/intel/igc/igc_regs.h    | 10 ++++++++++
>   2 files changed, 19 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
> index 48d5d18..09d72f7 100644
> --- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
> +++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
> @@ -322,6 +322,15 @@ static void igc_ethtool_get_regs(struct net_device *netdev,
>   
>   	for (i = 0; i < 8; i++)
>   		regs_buff[205 + i] = rd32(IGC_ETQF(i));
> +
> +	regs_buff[214] = rd32(IGC_PRMPTDTCNT);
> +	regs_buff[215] = rd32(IGC_PRMEVNTTCNT);
> +	regs_buff[216] = rd32(IGC_PRMPTDRCNT);
> +	regs_buff[217] = rd32(IGC_PRMEVNTRCNT);
> +	regs_buff[218] = rd32(IGC_PRMPBLTCNT);
> +	regs_buff[219] = rd32(IGC_PRMPBLRCNT);
> +	regs_buff[220] = rd32(IGC_PRMEXPTCNT);
> +	regs_buff[221] = rd32(IGC_PRMEXPRCNT);
>   }
>   
>   static void igc_ethtool_get_wol(struct net_device *netdev,
> diff --git a/drivers/net/ethernet/intel/igc/igc_regs.h b/drivers/net/ethernet/intel/igc/igc_regs.h
> index 7f999cf..010bb48 100644
> --- a/drivers/net/ethernet/intel/igc/igc_regs.h
> +++ b/drivers/net/ethernet/intel/igc/igc_regs.h
> @@ -211,6 +211,16 @@
>   
>   #define IGC_FTQF(_n)	(0x059E0 + (4 * (_n)))  /* 5-tuple Queue Fltr */
>   
> +/* Time sync registers - preemption statistics */
> +#define IGC_PRMPTDTCNT	0x04280  /* Good TX Preempted Packets */
> +#define IGC_PRMEVNTTCNT	0x04298  /* TX Preemption event counter */
> +#define IGC_PRMPTDRCNT	0x04284  /* Good RX Preempted Packets */
> +#define IGC_PRMEVNTRCNT	0x0429C  /* RX Preemption event counter */
> +#define IGC_PRMPBLTCNT	0x04288  /* Good TX Preemptable Packets */
> +#define IGC_PRMPBLRCNT	0x0428C  /* Good RX Preemptable Packets */
> +#define IGC_PRMEXPTCNT	0x04290  /* Good TX Express Packets */
> +#define IGC_PRMEXPRCNT	0x042A0  /* Preemption Exception Counter */
> +
>   /* Transmit Scheduling Registers */
>   #define IGC_TQAVCTRL		0x3570
>   #define IGC_TXQCTL(_n)		(0x3344 + 0x4 * (_n))
> 
There are some statistics supported by AM65 CPSW as well. So do you plan
to add this to ethtool stats that is dumped using ethtool -S option?

Thanks and regards,
-- 
Murali Karicheri
Texas Instruments

Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Joergen Andreasen]

The 05/19/2020 16:37, Vinicius Costa Gomes wrote:
> 
> Andre Guedes <andre.guedes@intel.com> writes:
> 
> > Hi,
> >
> > Quoting Vinicius Costa Gomes (2020-05-15 18:29:44)
> >> One example, for retrieving and setting the configuration:
> >>
> >> $ ethtool $ sudo ./ethtool --show-frame-preemption enp3s0
> >> Frame preemption settings for enp3s0:
> >>         support: supported
> >>         active: active
> >
> > IIUC the code in patch 2, 'active' is the actual configuration knob that
> > enables or disables the FP functionality on the NIC.
> >
> > That sounded a bit confusing to me since the spec uses the term 'active' to
> > indicate FP is currently enabled at both ends, and it is a read-only
> > information (see 12.30.1.4 from IEEE 802.1Q-2018). Maybe if we called this
> > 'enabled' it would be more clear.
> 
> Good point. Will rename this to "enabled".
> 
> >
> >>         supported queues: 0xf
> >>         supported queues: 0xe
> >>         minimum fragment size: 68
> >
> > I'm assuming this is the configuration knob for the minimal non-final fragment
> > defined in 802.3br.
> >
> > My understanding from the specs is that this value must be a multiple from 64
> > and cannot assume arbitrary values like shown here. See 99.4.7.3 from IEEE
> > 802.3 and Note 1 in S.2 from IEEE 802.1Q. In the previous discussion about FP,
> > we had this as a multiplier factor, not absolute value.
> 
> I thought that exposing this as "(1 + N)*64" (with 0 <= N <= 3) that it
> was more related to what's exposed via LLDP than the actual capabilities
> of the hardware. And for the hardware I have actually the values
> supported are: (1 + N)*64 + 4 (for N = 0, 1, 2, 3).
> 
> So I thought I was better to let the driver decide what values are
> acceptable.
> 
> This is a good question for people working with other hardware.
> 

I think it's most intuitive to use the values for AddFragSize as described in
802.3br (N = 0, 1, 2, 3).
You will anyway have to use one of these values when you want to expose the
requirements of your receiver through LLDP.

> 
> --
> Vinicius

-- 
Joergen Andreasen, Microchip

[Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Joergen Andreasen]

The 05/19/2020 16:37, Vinicius Costa Gomes wrote:
> 
> Andre Guedes <andre.guedes@intel.com> writes:
> 
> > Hi,
> >
> > Quoting Vinicius Costa Gomes (2020-05-15 18:29:44)
> >> One example, for retrieving and setting the configuration:
> >>
> >> $ ethtool $ sudo ./ethtool --show-frame-preemption enp3s0
> >> Frame preemption settings for enp3s0:
> >>         support: supported
> >>         active: active
> >
> > IIUC the code in patch 2, 'active' is the actual configuration knob that
> > enables or disables the FP functionality on the NIC.
> >
> > That sounded a bit confusing to me since the spec uses the term 'active' to
> > indicate FP is currently enabled at both ends, and it is a read-only
> > information (see 12.30.1.4 from IEEE 802.1Q-2018). Maybe if we called this
> > 'enabled' it would be more clear.
> 
> Good point. Will rename this to "enabled".
> 
> >
> >>         supported queues: 0xf
> >>         supported queues: 0xe
> >>         minimum fragment size: 68
> >
> > I'm assuming this is the configuration knob for the minimal non-final fragment
> > defined in 802.3br.
> >
> > My understanding from the specs is that this value must be a multiple from 64
> > and cannot assume arbitrary values like shown here. See 99.4.7.3 from IEEE
> > 802.3 and Note 1 in S.2 from IEEE 802.1Q. In the previous discussion about FP,
> > we had this as a multiplier factor, not absolute value.
> 
> I thought that exposing this as "(1 + N)*64" (with 0 <= N <= 3) that it
> was more related to what's exposed via LLDP than the actual capabilities
> of the hardware. And for the hardware I have actually the values
> supported are: (1 + N)*64 + 4 (for N = 0, 1, 2, 3).
> 
> So I thought I was better to let the driver decide what values are
> acceptable.
> 
> This is a good question for people working with other hardware.
> 

I think it's most intuitive to use the values for AddFragSize as described in
802.3br (N = 0, 1, 2, 3).
You will anyway have to use one of these values when you want to expose the
requirements of your receiver through LLDP.

> 
> --
> Vinicius

-- 
Joergen Andreasen, Microchip

Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Vinicius Costa Gomes]

Joergen Andreasen <joergen.andreasen@microchip.com> writes:

>> So I thought I was better to let the driver decide what values are
>> acceptable.
>> 
>> This is a good question for people working with other hardware.
>> 
>
> I think it's most intuitive to use the values for AddFragSize as described in
> 802.3br (N = 0, 1, 2, 3).
> You will anyway have to use one of these values when you want to expose the
> requirements of your receiver through LLDP.
>

Thanks. Seems that keeping this value restricted to multiples of 64 is
the way to go. Will fix for the next version of the series.


Cheers,
-- 
Vinicius

[Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Vinicius Costa Gomes]

Joergen Andreasen <joergen.andreasen@microchip.com> writes:

>> So I thought I was better to let the driver decide what values are
>> acceptable.
>> 
>> This is a good question for people working with other hardware.
>> 
>
> I think it's most intuitive to use the values for AddFragSize as described in
> 802.3br (N = 0, 1, 2, 3).
> You will anyway have to use one of these values when you want to expose the
> requirements of your receiver through LLDP.
>

Thanks. Seems that keeping this value restricted to multiples of 64 is
the way to go. Will fix for the next version of the series.


Cheers,
-- 
Vinicius

Re: [Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Andre Guedes]

Hi,

Quoting Jakub Kicinski (2020-05-18 16:09:06)
> On Mon, 18 May 2020 16:05:08 -0700 Vinicius Costa Gomes wrote:
> > Jakub Kicinski <kuba@kernel.org> writes:
> > >> That was the (only?) strong argument in favor of having frame preemption
> > >> in the TC side when this was last discussed.
> > >> 
> > >> We can have a hybrid solution, we can move the express/preemptible per
> > >> queue map to mqprio/taprio/whatever. And have the more specific
> > >> configuration knobs, minimum fragment size, etc, in ethtool.
> > >> 
> > >> What do you think?  
> > >
> > > Does the standard specify minimum fragment size as a global MAC setting?  
> > 
> > Yes, it's a per-MAC setting, not per-queue. 
> 
> If standard defines it as per-MAC and we can reasonably expect vendors
> won't try to "add value" and make it per queue (unlikely here AFAIU),
> then for this part ethtool configuration seems okay to me.

Before we move forward with this hybrid approach, let's recap a few points that
we discussed in the previous thread and make sure it addresses them properly.

1) Frame Preemption (FP) can be enabled without EST, as described in IEEE
802.1Q. In this case, the user has to create a dummy EST schedule in taprio
just to be able to enable FP, which doesn't look natural.

2) Mpqrio already looks overloaded. Besides mapping traffic classes into
hardware queues, it also supports different modes and traffic shaping. Do we
want to add yet another setting to it?

Regards,

Andre

[Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Andre Guedes]

Hi,

Quoting Jakub Kicinski (2020-05-18 16:09:06)
> On Mon, 18 May 2020 16:05:08 -0700 Vinicius Costa Gomes wrote:
> > Jakub Kicinski <kuba@kernel.org> writes:
> > >> That was the (only?) strong argument in favor of having frame preemption
> > >> in the TC side when this was last discussed.
> > >> 
> > >> We can have a hybrid solution, we can move the express/preemptible per
> > >> queue map to mqprio/taprio/whatever. And have the more specific
> > >> configuration knobs, minimum fragment size, etc, in ethtool.
> > >> 
> > >> What do you think?  
> > >
> > > Does the standard specify minimum fragment size as a global MAC setting?  
> > 
> > Yes, it's a per-MAC setting, not per-queue. 
> 
> If standard defines it as per-MAC and we can reasonably expect vendors
> won't try to "add value" and make it per queue (unlikely here AFAIU),
> then for this part ethtool configuration seems okay to me.

Before we move forward with this hybrid approach, let's recap a few points that
we discussed in the previous thread and make sure it addresses them properly.

1) Frame Preemption (FP) can be enabled without EST, as described in IEEE
802.1Q. In this case, the user has to create a dummy EST schedule in taprio
just to be able to enable FP, which doesn't look natural.

2) Mpqrio already looks overloaded. Besides mapping traffic classes into
hardware queues, it also supports different modes and traffic shaping. Do we
want to add yet another setting to it?

Regards,

Andre

Re: [Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Vinicius Costa Gomes]

Andre Guedes <andre.guedes@intel.com> writes:

>> If standard defines it as per-MAC and we can reasonably expect vendors
>> won't try to "add value" and make it per queue (unlikely here AFAIU),
>> then for this part ethtool configuration seems okay to me.
>
> Before we move forward with this hybrid approach, let's recap a few points that
> we discussed in the previous thread and make sure it addresses them
> properly.

Thanks for bringing them up.

>
> 1) Frame Preemption (FP) can be enabled without EST, as described in IEEE
> 802.1Q. In this case, the user has to create a dummy EST schedule in taprio
> just to be able to enable FP, which doesn't look natural.

What I meant by "dummy" schedule, is to configure taprio without
specifying any "sched-entry". And since we have support for adding
schedules during runtime, this might be even useful in general.

>
> 2) Mpqrio already looks overloaded. Besides mapping traffic classes into
> hardware queues, it also supports different modes and traffic shaping. Do we
> want to add yet another setting to it?

I also don't see this as a problem. The parameters that mqprio has carry
a lot of information, but the number of them is not that big.


Cheers,
-- 
Vinicius

[Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption

[Vinicius Costa Gomes]

Andre Guedes <andre.guedes@intel.com> writes:

>> If standard defines it as per-MAC and we can reasonably expect vendors
>> won't try to "add value" and make it per queue (unlikely here AFAIU),
>> then for this part ethtool configuration seems okay to me.
>
> Before we move forward with this hybrid approach, let's recap a few points that
> we discussed in the previous thread and make sure it addresses them
> properly.

Thanks for bringing them up.

>
> 1) Frame Preemption (FP) can be enabled without EST, as described in IEEE
> 802.1Q. In this case, the user has to create a dummy EST schedule in taprio
> just to be able to enable FP, which doesn't look natural.

What I meant by "dummy" schedule, is to configure taprio without
specifying any "sched-entry". And since we have support for adding
schedules during runtime, this might be even useful in general.

>
> 2) Mpqrio already looks overloaded. Besides mapping traffic classes into
> hardware queues, it also supports different modes and traffic shaping. Do we
> want to add yet another setting to it?

I also don't see this as a problem. The parameters that mqprio has carry
a lot of information, but the number of them is not that big.


Cheers,
-- 
Vinicius