* [Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption
@ 2020-05-16 1:29 ` Vinicius Costa Gomes
0 siblings, 0 replies; 77+ messages in thread
From: Vinicius Costa Gomes @ 2020-05-16 1:29 UTC (permalink / raw)
To: intel-wired-lan
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
^ permalink raw reply [flat|nested] 77+ messages in thread* [next-queue RFC 1/4] ethtool: Add support for configuring frame preemption
2020-05-16 1:29 ` [Intel-wired-lan] " Vinicius Costa Gomes
@ 2020-05-16 1:29 ` Vinicius Costa Gomes
-1 siblings, 0 replies; 77+ messages in thread
From: Vinicius Costa Gomes @ 2020-05-16 1:29 UTC (permalink / raw)
To: intel-wired-lan
Cc: Vinicius Costa Gomes, jeffrey.t.kirsher, netdev, vladimir.oltean,
po.liu, m-karicheri2, Jose.Abreu
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
^ permalink raw reply related [flat|nested] 77+ messages in thread* [Intel-wired-lan] [next-queue RFC 1/4] ethtool: Add support for configuring frame preemption
@ 2020-05-16 1:29 ` Vinicius Costa Gomes
0 siblings, 0 replies; 77+ messages in thread
From: Vinicius Costa Gomes @ 2020-05-16 1:29 UTC (permalink / raw)
To: intel-wired-lan
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
^ permalink raw reply related [flat|nested] 77+ messages in thread* Re: [next-queue RFC 1/4] ethtool: Add support for configuring frame preemption
2020-05-16 1:29 ` [Intel-wired-lan] " Vinicius Costa Gomes
@ 2020-05-19 15:27 ` Murali Karicheri
-1 siblings, 0 replies; 77+ messages in thread
From: Murali Karicheri @ 2020-05-19 15:27 UTC (permalink / raw)
To: Vinicius Costa Gomes, intel-wired-lan
Cc: jeffrey.t.kirsher, netdev, vladimir.oltean, po.liu, Jose.Abreu
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
^ permalink raw reply [flat|nested] 77+ messages in thread* [Intel-wired-lan] [next-queue RFC 1/4] ethtool: Add support for configuring frame preemption
@ 2020-05-19 15:27 ` Murali Karicheri
0 siblings, 0 replies; 77+ messages in thread
From: Murali Karicheri @ 2020-05-19 15:27 UTC (permalink / raw)
To: intel-wired-lan
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
^ permalink raw reply [flat|nested] 77+ messages in thread
* [next-queue RFC 2/4] ethtool: Add support for configuring frame preemption via netlink
2020-05-16 1:29 ` [Intel-wired-lan] " Vinicius Costa Gomes
@ 2020-05-16 1:29 ` Vinicius Costa Gomes
-1 siblings, 0 replies; 77+ messages in thread
From: Vinicius Costa Gomes @ 2020-05-16 1:29 UTC (permalink / raw)
To: intel-wired-lan
Cc: Vinicius Costa Gomes, jeffrey.t.kirsher, netdev, vladimir.oltean,
po.liu, m-karicheri2, Jose.Abreu
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] = ðnl_pause_request_ops,
[ETHTOOL_MSG_EEE_GET] = ðnl_eee_request_ops,
[ETHTOOL_MSG_TSINFO_GET] = ðnl_tsinfo_request_ops,
+ [ETHTOOL_MSG_PREEMPT_GET] = ðnl_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] = ðnl_coalesce_request_ops,
[ETHTOOL_MSG_PAUSE_NTF] = ðnl_pause_request_ops,
[ETHTOOL_MSG_EEE_NTF] = ðnl_eee_request_ops,
+ [ETHTOOL_MSG_PREEMPT_NTF] = ðnl_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
^ permalink raw reply related [flat|nested] 77+ messages in thread* [Intel-wired-lan] [next-queue RFC 2/4] ethtool: Add support for configuring frame preemption via netlink
@ 2020-05-16 1:29 ` Vinicius Costa Gomes
0 siblings, 0 replies; 77+ messages in thread
From: Vinicius Costa Gomes @ 2020-05-16 1:29 UTC (permalink / raw)
To: intel-wired-lan
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] = ðnl_pause_request_ops,
[ETHTOOL_MSG_EEE_GET] = ðnl_eee_request_ops,
[ETHTOOL_MSG_TSINFO_GET] = ðnl_tsinfo_request_ops,
+ [ETHTOOL_MSG_PREEMPT_GET] = ðnl_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] = ðnl_coalesce_request_ops,
[ETHTOOL_MSG_PAUSE_NTF] = ðnl_pause_request_ops,
[ETHTOOL_MSG_EEE_NTF] = ðnl_eee_request_ops,
+ [ETHTOOL_MSG_PREEMPT_NTF] = ðnl_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
^ permalink raw reply related [flat|nested] 77+ messages in thread* Re: [next-queue RFC 2/4] ethtool: Add support for configuring frame preemption via netlink
2020-05-16 1:29 ` [Intel-wired-lan] " Vinicius Costa Gomes
@ 2020-05-18 12:27 ` Dan Carpenter
-1 siblings, 0 replies; 77+ messages in thread
From: Dan Carpenter @ 2020-05-18 12:27 UTC (permalink / raw)
To: kbuild
[-- 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
^ permalink raw reply [flat|nested] 77+ messages in thread* Re: [next-queue RFC 2/4] ethtool: Add support for configuring frame preemption via netlink
@ 2020-05-18 12:27 ` Dan Carpenter
0 siblings, 0 replies; 77+ messages in thread
From: Dan Carpenter @ 2020-05-18 12:27 UTC (permalink / raw)
To: kbuild-all
[-- 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
^ permalink raw reply [flat|nested] 77+ messages in thread
* [next-queue RFC 3/4] igc: Add support for configuring frame preemption
2020-05-16 1:29 ` [Intel-wired-lan] " Vinicius Costa Gomes
@ 2020-05-16 1:29 ` Vinicius Costa Gomes
-1 siblings, 0 replies; 77+ messages in thread
From: Vinicius Costa Gomes @ 2020-05-16 1:29 UTC (permalink / raw)
To: intel-wired-lan
Cc: Vinicius Costa Gomes, jeffrey.t.kirsher, netdev, vladimir.oltean,
po.liu, m-karicheri2, Jose.Abreu
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
^ permalink raw reply related [flat|nested] 77+ messages in thread* [Intel-wired-lan] [next-queue RFC 3/4] igc: Add support for configuring frame preemption
@ 2020-05-16 1:29 ` Vinicius Costa Gomes
0 siblings, 0 replies; 77+ messages in thread
From: Vinicius Costa Gomes @ 2020-05-16 1:29 UTC (permalink / raw)
To: intel-wired-lan
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
^ permalink raw reply related [flat|nested] 77+ messages in thread* Re: [next-queue RFC 3/4] igc: Add support for configuring frame preemption
2020-05-16 1:29 ` [Intel-wired-lan] " Vinicius Costa Gomes
@ 2020-05-19 16:36 ` Murali Karicheri
-1 siblings, 0 replies; 77+ messages in thread
From: Murali Karicheri @ 2020-05-19 16:36 UTC (permalink / raw)
To: Vinicius Costa Gomes, intel-wired-lan
Cc: jeffrey.t.kirsher, netdev, vladimir.oltean, po.liu, Jose.Abreu
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
^ permalink raw reply [flat|nested] 77+ messages in thread* [Intel-wired-lan] [next-queue RFC 3/4] igc: Add support for configuring frame preemption
@ 2020-05-19 16:36 ` Murali Karicheri
0 siblings, 0 replies; 77+ messages in thread
From: Murali Karicheri @ 2020-05-19 16:36 UTC (permalink / raw)
To: intel-wired-lan
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
^ permalink raw reply [flat|nested] 77+ messages in thread
* [next-queue RFC 4/4] igc: Add support for exposing frame preemption stats registers
2020-05-16 1:29 ` [Intel-wired-lan] " Vinicius Costa Gomes
@ 2020-05-16 1:29 ` Vinicius Costa Gomes
-1 siblings, 0 replies; 77+ messages in thread
From: Vinicius Costa Gomes @ 2020-05-16 1:29 UTC (permalink / raw)
To: intel-wired-lan
Cc: Vinicius Costa Gomes, jeffrey.t.kirsher, netdev, vladimir.oltean,
po.liu, m-karicheri2, Jose.Abreu
[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
^ permalink raw reply related [flat|nested] 77+ messages in thread
* [Intel-wired-lan] [next-queue RFC 4/4] igc: Add support for exposing frame preemption stats registers
@ 2020-05-16 1:29 ` Vinicius Costa Gomes
0 siblings, 0 replies; 77+ messages in thread
From: Vinicius Costa Gomes @ 2020-05-16 1:29 UTC (permalink / raw)
To: intel-wired-lan
[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
^ permalink raw reply related [flat|nested] 77+ messages in thread
* Re: [next-queue RFC 4/4] igc: Add support for exposing frame preemption stats registers
2020-05-16 1:29 ` [Intel-wired-lan] " Vinicius Costa Gomes
@ 2020-05-20 12:50 ` Murali Karicheri
-1 siblings, 0 replies; 77+ messages in thread
From: Murali Karicheri @ 2020-05-20 12:50 UTC (permalink / raw)
To: Vinicius Costa Gomes, intel-wired-lan
Cc: jeffrey.t.kirsher, netdev, vladimir.oltean, po.liu, Jose.Abreu
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
^ permalink raw reply [flat|nested] 77+ messages in thread
* [Intel-wired-lan] [next-queue RFC 4/4] igc: Add support for exposing frame preemption stats registers
@ 2020-05-20 12:50 ` Murali Karicheri
0 siblings, 0 replies; 77+ messages in thread
From: Murali Karicheri @ 2020-05-20 12:50 UTC (permalink / raw)
To: intel-wired-lan
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
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption
2020-05-16 1:29 ` [Intel-wired-lan] " Vinicius Costa Gomes
@ 2020-05-16 9:33 ` Michal Kubecek
-1 siblings, 0 replies; 77+ messages in thread
From: Michal Kubecek @ 2020-05-16 9:33 UTC (permalink / raw)
To: netdev
Cc: Vinicius Costa Gomes, intel-wired-lan, jeffrey.t.kirsher,
vladimir.oltean, po.liu, m-karicheri2, Jose.Abreu
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
^ permalink raw reply [flat|nested] 77+ messages in thread
* [Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption
@ 2020-05-16 9:33 ` Michal Kubecek
0 siblings, 0 replies; 77+ messages in thread
From: Michal Kubecek @ 2020-05-16 9:33 UTC (permalink / raw)
To: intel-wired-lan
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
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption
2020-05-16 9:33 ` [Intel-wired-lan] " Michal Kubecek
@ 2020-05-18 19:34 ` Vinicius Costa Gomes
-1 siblings, 0 replies; 77+ messages in thread
From: Vinicius Costa Gomes @ 2020-05-18 19:34 UTC (permalink / raw)
To: Michal Kubecek, netdev
Cc: intel-wired-lan, jeffrey.t.kirsher, vladimir.oltean, po.liu,
m-karicheri2, Jose.Abreu
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
^ permalink raw reply [flat|nested] 77+ messages in thread
* [Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption
@ 2020-05-18 19:34 ` Vinicius Costa Gomes
0 siblings, 0 replies; 77+ messages in thread
From: Vinicius Costa Gomes @ 2020-05-18 19:34 UTC (permalink / raw)
To: intel-wired-lan
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
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption
2020-05-18 19:34 ` [Intel-wired-lan] " Vinicius Costa Gomes
@ 2020-05-19 22:40 ` Andre Guedes
-1 siblings, 0 replies; 77+ messages in thread
From: Andre Guedes @ 2020-05-19 22:40 UTC (permalink / raw)
To: Michal Kubecek, Vinicius Costa Gomes, netdev
Cc: intel-wired-lan, jeffrey.t.kirsher, vladimir.oltean, po.liu,
m-karicheri2, Jose.Abreu
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
^ permalink raw reply [flat|nested] 77+ messages in thread
* [Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption
@ 2020-05-19 22:40 ` Andre Guedes
0 siblings, 0 replies; 77+ messages in thread
From: Andre Guedes @ 2020-05-19 22:40 UTC (permalink / raw)
To: intel-wired-lan
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
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption
2020-05-19 22:40 ` [Intel-wired-lan] " Andre Guedes
@ 2020-05-19 22:53 ` Vinicius Costa Gomes
-1 siblings, 0 replies; 77+ messages in thread
From: Vinicius Costa Gomes @ 2020-05-19 22:53 UTC (permalink / raw)
To: Andre Guedes, Michal Kubecek, netdev
Cc: intel-wired-lan, jeffrey.t.kirsher, vladimir.oltean, po.liu,
m-karicheri2, Jose.Abreu
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
^ permalink raw reply [flat|nested] 77+ messages in thread
* [Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption
@ 2020-05-19 22:53 ` Vinicius Costa Gomes
0 siblings, 0 replies; 77+ messages in thread
From: Vinicius Costa Gomes @ 2020-05-19 22:53 UTC (permalink / raw)
To: intel-wired-lan
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
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption
2020-05-16 1:29 ` [Intel-wired-lan] " Vinicius Costa Gomes
@ 2020-05-16 20:37 ` David Miller
-1 siblings, 0 replies; 77+ messages in thread
From: David Miller @ 2020-05-16 20:37 UTC (permalink / raw)
To: vinicius.gomes
Cc: intel-wired-lan, jeffrey.t.kirsher, netdev, vladimir.oltean,
po.liu, m-karicheri2, Jose.Abreu
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?
^ permalink raw reply [flat|nested] 77+ messages in thread
* [Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption
@ 2020-05-16 20:37 ` David Miller
0 siblings, 0 replies; 77+ messages in thread
From: David Miller @ 2020-05-16 20:37 UTC (permalink / raw)
To: intel-wired-lan
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?
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption
2020-05-16 20:37 ` [Intel-wired-lan] " David Miller
@ 2020-05-16 21:03 ` Vladimir Oltean
-1 siblings, 0 replies; 77+ messages in thread
From: Vladimir Oltean @ 2020-05-16 21:03 UTC (permalink / raw)
To: David Miller
Cc: Vinicius Costa Gomes, intel-wired-lan, Jeff Kirsher, netdev,
Vladimir Oltean, Po Liu, Murali Karicheri, Jose Abreu
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
^ permalink raw reply [flat|nested] 77+ messages in thread
* [Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption
@ 2020-05-16 21:03 ` Vladimir Oltean
0 siblings, 0 replies; 77+ messages in thread
From: Vladimir Oltean @ 2020-05-16 21:03 UTC (permalink / raw)
To: intel-wired-lan
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
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption
2020-05-16 21:03 ` [Intel-wired-lan] " Vladimir Oltean
@ 2020-05-16 22:19 ` David Miller
-1 siblings, 0 replies; 77+ messages in thread
From: David Miller @ 2020-05-16 22:19 UTC (permalink / raw)
To: olteanv
Cc: vinicius.gomes, intel-wired-lan, jeffrey.t.kirsher, netdev,
vladimir.oltean, po.liu, m-karicheri2, Jose.Abreu
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.
^ permalink raw reply [flat|nested] 77+ messages in thread
* [Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption
@ 2020-05-16 22:19 ` David Miller
0 siblings, 0 replies; 77+ messages in thread
From: David Miller @ 2020-05-16 22:19 UTC (permalink / raw)
To: intel-wired-lan
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.
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption
2020-05-16 22:19 ` [Intel-wired-lan] " David Miller
@ 2020-05-17 10:51 ` Vladimir Oltean
-1 siblings, 0 replies; 77+ messages in thread
From: Vladimir Oltean @ 2020-05-17 10:51 UTC (permalink / raw)
To: David Miller
Cc: Vinicius Costa Gomes, intel-wired-lan, Jeff Kirsher, netdev,
Vladimir Oltean, Po Liu, Murali Karicheri, Jose Abreu
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
^ permalink raw reply [flat|nested] 77+ messages in thread
* [Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption
@ 2020-05-17 10:51 ` Vladimir Oltean
0 siblings, 0 replies; 77+ messages in thread
From: Vladimir Oltean @ 2020-05-17 10:51 UTC (permalink / raw)
To: intel-wired-lan
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
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption
2020-05-17 10:51 ` [Intel-wired-lan] " Vladimir Oltean
@ 2020-05-17 18:45 ` Andrew Lunn
-1 siblings, 0 replies; 77+ messages in thread
From: Andrew Lunn @ 2020-05-17 18:45 UTC (permalink / raw)
To: Vladimir Oltean
Cc: David Miller, Vinicius Costa Gomes, intel-wired-lan,
Jeff Kirsher, netdev, Vladimir Oltean, Po Liu, Murali Karicheri,
Jose Abreu
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
^ permalink raw reply [flat|nested] 77+ messages in thread
* [Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption
@ 2020-05-17 18:45 ` Andrew Lunn
0 siblings, 0 replies; 77+ messages in thread
From: Andrew Lunn @ 2020-05-17 18:45 UTC (permalink / raw)
To: intel-wired-lan
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
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption
2020-05-17 18:45 ` [Intel-wired-lan] " Andrew Lunn
@ 2020-05-17 19:04 ` Vladimir Oltean
-1 siblings, 0 replies; 77+ messages in thread
From: Vladimir Oltean @ 2020-05-17 19:04 UTC (permalink / raw)
To: Andrew Lunn
Cc: David Miller, Vinicius Costa Gomes, intel-wired-lan,
Jeff Kirsher, netdev, Vladimir Oltean, Po Liu, Murali Karicheri,
Jose Abreu
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
^ permalink raw reply [flat|nested] 77+ messages in thread
* [Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption
@ 2020-05-17 19:04 ` Vladimir Oltean
0 siblings, 0 replies; 77+ messages in thread
From: Vladimir Oltean @ 2020-05-17 19:04 UTC (permalink / raw)
To: intel-wired-lan
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
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption
2020-05-16 22:19 ` [Intel-wired-lan] " David Miller
@ 2020-05-18 19:05 ` Vinicius Costa Gomes
-1 siblings, 0 replies; 77+ messages in thread
From: Vinicius Costa Gomes @ 2020-05-18 19:05 UTC (permalink / raw)
To: David Miller, olteanv
Cc: intel-wired-lan, jeffrey.t.kirsher, netdev, vladimir.oltean,
po.liu, m-karicheri2, Jose.Abreu
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
^ permalink raw reply [flat|nested] 77+ messages in thread
* [Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption
@ 2020-05-18 19:05 ` Vinicius Costa Gomes
0 siblings, 0 replies; 77+ messages in thread
From: Vinicius Costa Gomes @ 2020-05-18 19:05 UTC (permalink / raw)
To: intel-wired-lan
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
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption
2020-05-18 19:05 ` [Intel-wired-lan] " Vinicius Costa Gomes
@ 2020-05-18 20:56 ` Jakub Kicinski
-1 siblings, 0 replies; 77+ messages in thread
From: Jakub Kicinski @ 2020-05-18 20:56 UTC (permalink / raw)
To: Vinicius Costa Gomes
Cc: David Miller, olteanv, intel-wired-lan, jeffrey.t.kirsher,
netdev, vladimir.oltean, po.liu, m-karicheri2, Jose.Abreu
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.
^ permalink raw reply [flat|nested] 77+ messages in thread
* [Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption
@ 2020-05-18 20:56 ` Jakub Kicinski
0 siblings, 0 replies; 77+ messages in thread
From: Jakub Kicinski @ 2020-05-18 20:56 UTC (permalink / raw)
To: intel-wired-lan
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.
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption
2020-05-18 20:56 ` [Intel-wired-lan] " Jakub Kicinski
@ 2020-05-18 22:06 ` Vinicius Costa Gomes
-1 siblings, 0 replies; 77+ messages in thread
From: Vinicius Costa Gomes @ 2020-05-18 22:06 UTC (permalink / raw)
To: Jakub Kicinski
Cc: David Miller, olteanv, intel-wired-lan, jeffrey.t.kirsher,
netdev, vladimir.oltean, po.liu, m-karicheri2, Jose.Abreu
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
^ permalink raw reply [flat|nested] 77+ messages in thread
* [Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption
@ 2020-05-18 22:06 ` Vinicius Costa Gomes
0 siblings, 0 replies; 77+ messages in thread
From: Vinicius Costa Gomes @ 2020-05-18 22:06 UTC (permalink / raw)
To: intel-wired-lan
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
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption
2020-05-18 22:06 ` [Intel-wired-lan] " Vinicius Costa Gomes
@ 2020-05-18 22:22 ` Jakub Kicinski
-1 siblings, 0 replies; 77+ messages in thread
From: Jakub Kicinski @ 2020-05-18 22:22 UTC (permalink / raw)
To: Vinicius Costa Gomes
Cc: David Miller, olteanv, intel-wired-lan, jeffrey.t.kirsher,
netdev, vladimir.oltean, po.liu, m-karicheri2, Jose.Abreu
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?
^ permalink raw reply [flat|nested] 77+ messages in thread
* [Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption
@ 2020-05-18 22:22 ` Jakub Kicinski
0 siblings, 0 replies; 77+ messages in thread
From: Jakub Kicinski @ 2020-05-18 22:22 UTC (permalink / raw)
To: intel-wired-lan
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?
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption
2020-05-18 22:22 ` [Intel-wired-lan] " Jakub Kicinski
@ 2020-05-18 23:05 ` Vinicius Costa Gomes
-1 siblings, 0 replies; 77+ messages in thread
From: Vinicius Costa Gomes @ 2020-05-18 23:05 UTC (permalink / raw)
To: Jakub Kicinski
Cc: David Miller, olteanv, intel-wired-lan, jeffrey.t.kirsher,
netdev, vladimir.oltean, po.liu, m-karicheri2, Jose.Abreu
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
^ permalink raw reply [flat|nested] 77+ messages in thread
* [Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption
@ 2020-05-18 23:05 ` Vinicius Costa Gomes
0 siblings, 0 replies; 77+ messages in thread
From: Vinicius Costa Gomes @ 2020-05-18 23:05 UTC (permalink / raw)
To: intel-wired-lan
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
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption
2020-05-18 23:05 ` [Intel-wired-lan] " Vinicius Costa Gomes
@ 2020-05-18 23:09 ` Jakub Kicinski
-1 siblings, 0 replies; 77+ messages in thread
From: Jakub Kicinski @ 2020-05-18 23:09 UTC (permalink / raw)
To: Vinicius Costa Gomes
Cc: David Miller, olteanv, intel-wired-lan, jeffrey.t.kirsher,
netdev, vladimir.oltean, po.liu, m-karicheri2, Jose.Abreu
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.
^ permalink raw reply [flat|nested] 77+ messages in thread
* [Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption
@ 2020-05-18 23:09 ` Jakub Kicinski
0 siblings, 0 replies; 77+ messages in thread
From: Jakub Kicinski @ 2020-05-18 23:09 UTC (permalink / raw)
To: intel-wired-lan
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.
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption
2020-05-18 23:09 ` [Intel-wired-lan] " Jakub Kicinski
@ 2020-05-20 21:42 ` Andre Guedes
-1 siblings, 0 replies; 77+ messages in thread
From: Andre Guedes @ 2020-05-20 21:42 UTC (permalink / raw)
To: Jakub Kicinski, Vinicius Costa Gomes
Cc: Jose.Abreu, vladimir.oltean, po.liu, m-karicheri2,
intel-wired-lan, netdev, olteanv, David Miller
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
^ permalink raw reply [flat|nested] 77+ messages in thread
* [Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption
@ 2020-05-20 21:42 ` Andre Guedes
0 siblings, 0 replies; 77+ messages in thread
From: Andre Guedes @ 2020-05-20 21:42 UTC (permalink / raw)
To: intel-wired-lan
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
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption
2020-05-20 21:42 ` Andre Guedes
@ 2020-05-20 22:35 ` Vinicius Costa Gomes
-1 siblings, 0 replies; 77+ messages in thread
From: Vinicius Costa Gomes @ 2020-05-20 22:35 UTC (permalink / raw)
To: Andre Guedes, Jakub Kicinski
Cc: Jose.Abreu, vladimir.oltean, po.liu, m-karicheri2,
intel-wired-lan, netdev, olteanv, David Miller
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
^ permalink raw reply [flat|nested] 77+ messages in thread
* [Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption
@ 2020-05-20 22:35 ` Vinicius Costa Gomes
0 siblings, 0 replies; 77+ messages in thread
From: Vinicius Costa Gomes @ 2020-05-20 22:35 UTC (permalink / raw)
To: intel-wired-lan
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
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption
2020-05-18 22:06 ` [Intel-wired-lan] " Vinicius Costa Gomes
@ 2020-05-19 16:34 ` Murali Karicheri
-1 siblings, 0 replies; 77+ messages in thread
From: Murali Karicheri @ 2020-05-19 16:34 UTC (permalink / raw)
To: Vinicius Costa Gomes, Jakub Kicinski
Cc: David Miller, olteanv, intel-wired-lan, jeffrey.t.kirsher,
netdev, vladimir.oltean, po.liu, Jose.Abreu
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
^ permalink raw reply [flat|nested] 77+ messages in thread
* [Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption
@ 2020-05-19 16:34 ` Murali Karicheri
0 siblings, 0 replies; 77+ messages in thread
From: Murali Karicheri @ 2020-05-19 16:34 UTC (permalink / raw)
To: intel-wired-lan
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
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption
2020-05-19 16:34 ` [Intel-wired-lan] " Murali Karicheri
@ 2020-05-19 17:49 ` Vinicius Costa Gomes
-1 siblings, 0 replies; 77+ messages in thread
From: Vinicius Costa Gomes @ 2020-05-19 17:49 UTC (permalink / raw)
To: Murali Karicheri, Jakub Kicinski
Cc: David Miller, olteanv, intel-wired-lan, jeffrey.t.kirsher,
netdev, vladimir.oltean, po.liu, Jose.Abreu
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
^ permalink raw reply [flat|nested] 77+ messages in thread
* [Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption
@ 2020-05-19 17:49 ` Vinicius Costa Gomes
0 siblings, 0 replies; 77+ messages in thread
From: Vinicius Costa Gomes @ 2020-05-19 17:49 UTC (permalink / raw)
To: intel-wired-lan
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
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption
2020-05-16 1:29 ` [Intel-wired-lan] " Vinicius Costa Gomes
` (6 preceding siblings ...)
(?)
@ 2020-05-17 15:06 ` Michael Walle
2020-05-18 13:36 ` Murali Karicheri
-1 siblings, 1 reply; 77+ messages in thread
From: Michael Walle @ 2020-05-17 15:06 UTC (permalink / raw)
To: Vinicius Costa Gomes
Cc: jeffrey.t.kirsher, netdev, vladimir.oltean, po.liu, m-karicheri2,
Jose.Abreu
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
>
^ permalink raw reply [flat|nested] 77+ messages in thread* Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption
2020-05-17 15:06 ` Michael Walle
@ 2020-05-18 13:36 ` Murali Karicheri
2020-05-19 20:41 ` Michael Walle
0 siblings, 1 reply; 77+ messages in thread
From: Murali Karicheri @ 2020-05-18 13:36 UTC (permalink / raw)
To: Michael Walle, Vinicius Costa Gomes
Cc: jeffrey.t.kirsher, netdev, vladimir.oltean, po.liu, Jose.Abreu
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
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption
2020-05-18 13:36 ` Murali Karicheri
@ 2020-05-19 20:41 ` Michael Walle
0 siblings, 0 replies; 77+ messages in thread
From: Michael Walle @ 2020-05-19 20:41 UTC (permalink / raw)
To: Murali Karicheri
Cc: Vinicius Costa Gomes, jeffrey.t.kirsher, netdev, vladimir.oltean,
po.liu, Jose.Abreu
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
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption
2020-05-16 1:29 ` [Intel-wired-lan] " Vinicius Costa Gomes
@ 2020-05-19 14:53 ` Murali Karicheri
-1 siblings, 0 replies; 77+ messages in thread
From: Murali Karicheri @ 2020-05-19 14:53 UTC (permalink / raw)
To: Vinicius Costa Gomes, intel-wired-lan
Cc: jeffrey.t.kirsher, netdev, vladimir.oltean, po.liu, Jose.Abreu
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
^ permalink raw reply [flat|nested] 77+ messages in thread* [Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption
@ 2020-05-19 14:53 ` Murali Karicheri
0 siblings, 0 replies; 77+ messages in thread
From: Murali Karicheri @ 2020-05-19 14:53 UTC (permalink / raw)
To: intel-wired-lan
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
^ permalink raw reply [flat|nested] 77+ messages in thread* Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption
2020-05-19 14:53 ` [Intel-wired-lan] " Murali Karicheri
@ 2020-05-19 15:32 ` Vinicius Costa Gomes
-1 siblings, 0 replies; 77+ messages in thread
From: Vinicius Costa Gomes @ 2020-05-19 15:32 UTC (permalink / raw)
To: Murali Karicheri, intel-wired-lan
Cc: jeffrey.t.kirsher, netdev, vladimir.oltean, po.liu, Jose.Abreu
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
^ permalink raw reply [flat|nested] 77+ messages in thread* [Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption
@ 2020-05-19 15:32 ` Vinicius Costa Gomes
0 siblings, 0 replies; 77+ messages in thread
From: Vinicius Costa Gomes @ 2020-05-19 15:32 UTC (permalink / raw)
To: intel-wired-lan
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
^ permalink raw reply [flat|nested] 77+ messages in thread* Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption
2020-05-19 15:32 ` [Intel-wired-lan] " Vinicius Costa Gomes
@ 2020-05-19 16:11 ` Murali Karicheri
-1 siblings, 0 replies; 77+ messages in thread
From: Murali Karicheri @ 2020-05-19 16:11 UTC (permalink / raw)
To: Vinicius Costa Gomes, intel-wired-lan
Cc: jeffrey.t.kirsher, netdev, vladimir.oltean, po.liu, Jose.Abreu
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
^ permalink raw reply [flat|nested] 77+ messages in thread* [Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption
@ 2020-05-19 16:11 ` Murali Karicheri
0 siblings, 0 replies; 77+ messages in thread
From: Murali Karicheri @ 2020-05-19 16:11 UTC (permalink / raw)
To: intel-wired-lan
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
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption
2020-05-16 1:29 ` [Intel-wired-lan] " Vinicius Costa Gomes
@ 2020-05-19 22:39 ` Andre Guedes
-1 siblings, 0 replies; 77+ messages in thread
From: Andre Guedes @ 2020-05-19 22:39 UTC (permalink / raw)
To: Vinicius Costa Gomes, intel-wired-lan
Cc: Vinicius Costa Gomes, jeffrey.t.kirsher, netdev, vladimir.oltean,
po.liu, m-karicheri2, Jose.Abreu
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
^ permalink raw reply [flat|nested] 77+ messages in thread
* [Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption
@ 2020-05-19 22:39 ` Andre Guedes
0 siblings, 0 replies; 77+ messages in thread
From: Andre Guedes @ 2020-05-19 22:39 UTC (permalink / raw)
To: intel-wired-lan
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
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption
2020-05-19 22:39 ` [Intel-wired-lan] " Andre Guedes
@ 2020-05-19 23:37 ` Vinicius Costa Gomes
-1 siblings, 0 replies; 77+ messages in thread
From: Vinicius Costa Gomes @ 2020-05-19 23:37 UTC (permalink / raw)
To: Andre Guedes, intel-wired-lan
Cc: jeffrey.t.kirsher, netdev, vladimir.oltean, po.liu, m-karicheri2,
Jose.Abreu
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
^ permalink raw reply [flat|nested] 77+ messages in thread
* [Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption
@ 2020-05-19 23:37 ` Vinicius Costa Gomes
0 siblings, 0 replies; 77+ messages in thread
From: Vinicius Costa Gomes @ 2020-05-19 23:37 UTC (permalink / raw)
To: intel-wired-lan
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
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption
2020-05-19 23:37 ` [Intel-wired-lan] " Vinicius Costa Gomes
@ 2020-05-20 12:47 ` Murali Karicheri
-1 siblings, 0 replies; 77+ messages in thread
From: Murali Karicheri @ 2020-05-20 12:47 UTC (permalink / raw)
To: Vinicius Costa Gomes, Andre Guedes, intel-wired-lan
Cc: jeffrey.t.kirsher, netdev, vladimir.oltean, po.liu, Jose.Abreu
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
^ permalink raw reply [flat|nested] 77+ messages in thread
* [Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption
@ 2020-05-20 12:47 ` Murali Karicheri
0 siblings, 0 replies; 77+ messages in thread
From: Murali Karicheri @ 2020-05-20 12:47 UTC (permalink / raw)
To: intel-wired-lan
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
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption
2020-05-19 23:37 ` [Intel-wired-lan] " Vinicius Costa Gomes
@ 2020-05-20 12:52 ` Joergen Andreasen
-1 siblings, 0 replies; 77+ messages in thread
From: Joergen Andreasen @ 2020-05-20 12:52 UTC (permalink / raw)
To: Vinicius Costa Gomes
Cc: Andre Guedes, intel-wired-lan, jeffrey.t.kirsher, netdev,
vladimir.oltean, po.liu, m-karicheri2, Jose.Abreu
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
^ permalink raw reply [flat|nested] 77+ messages in thread
* [Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption
@ 2020-05-20 12:52 ` Joergen Andreasen
0 siblings, 0 replies; 77+ messages in thread
From: Joergen Andreasen @ 2020-05-20 12:52 UTC (permalink / raw)
To: intel-wired-lan
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
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [next-queue RFC 0/4] ethtool: Add support for frame preemption
2020-05-20 12:52 ` [Intel-wired-lan] " Joergen Andreasen
@ 2020-05-20 21:32 ` Vinicius Costa Gomes
-1 siblings, 0 replies; 77+ messages in thread
From: Vinicius Costa Gomes @ 2020-05-20 21:32 UTC (permalink / raw)
To: Joergen Andreasen
Cc: Andre Guedes, intel-wired-lan, jeffrey.t.kirsher, netdev,
vladimir.oltean, po.liu, m-karicheri2, Jose.Abreu
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
^ permalink raw reply [flat|nested] 77+ messages in thread
* [Intel-wired-lan] [next-queue RFC 0/4] ethtool: Add support for frame preemption
@ 2020-05-20 21:32 ` Vinicius Costa Gomes
0 siblings, 0 replies; 77+ messages in thread
From: Vinicius Costa Gomes @ 2020-05-20 21:32 UTC (permalink / raw)
To: intel-wired-lan
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
^ permalink raw reply [flat|nested] 77+ messages in thread