by Simon Horman

[permalink] [raw]

Subject: Re: [PATCH net-next v6 3/9] net: add IEEE 802.1q specific helpers

On Wed, Apr 10, 2024 at 10:05:50AM +0200, Oleksij Rempel wrote:
> IEEE 802.1q specification provides recommendation and examples which can
> be used as good default values for different drivers.
>
> This patch implements mapping examples documented in IEEE 802.1Q-2022 in
> Annex I "I.3 Traffic type to traffic class mapping" and IETF DSCP naming
> and mapping DSCP to Traffic Type inspired by RFC8325.
>
> This helpers will be used in followup patches for dsa/microchip DCB
> implementation.
>
> Signed-off-by: Oleksij Rempel <[email protected]>

..

> diff --git a/include/net/ieee8021q.h b/include/net/ieee8021q.h
> new file mode 100644
> index 0000000000000..3bec7ec951362
> --- /dev/null
> +++ b/include/net/ieee8021q.h
> @@ -0,0 +1,55 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (c) 2024 Pengutronix, Oleksij Rempel <[email protected]> */
> +
> +#ifndef _NET_IEEE8021Q_H
> +#define _NET_IEEE8021Q_H
> +
> +#include <linux/errno.h>
> +
> +/**
> + * enum ieee8021q_traffic_type - 802.1Q traffic type priority values (802.1Q-2022)
> + *
> + * @IEEE8021Q_TT_BK: Background
> + * @IEEE8021Q_TT_BE: Best Effort (default). According to 802.1Q-2022, BE is 0
> + * but has higher priority than BK which is 1.
> + * @IEEE8021Q_TT_EE: Excellent Effort
> + * @IEEE8021Q_TT_CA: Critical Applications
> + * @IEEE8021Q_TT_VI: Video, < 100 ms latency and jitter
> + * @IEEE8021Q_TT_VO: Voice, < 10 ms latency and jitter
> + * @IEEE8021Q_TT_IC: Internetwork Control
> + * @IEEE8021Q_TT_NC: Network Control
> + */
> +enum ieee8021q_traffic_type {
> + IEEE8021Q_TT_BK = 0,
> + IEEE8021Q_TT_BE = 1,
> + IEEE8021Q_TT_EE = 2,
> + IEEE8021Q_TT_CA = 3,
> + IEEE8021Q_TT_VI = 4,
> + IEEE8021Q_TT_VO = 5,
> + IEEE8021Q_TT_IC = 6,
> + IEEE8021Q_TT_NC = 7,
> +

Hi Oleksij,

I think the following line should go here to keep ./scripts/kernel-doc -none
happy:

/* private: */

> + IEEE8021Q_TT_MAX,
> +};

..

2024-04-11 11:52:16

by Vladimir Oltean

[permalink] [raw]

Subject: Re: [PATCH net-next v6 3/9] net: add IEEE 802.1q specific helpers

I have the following comments already written; sending the email mainly
to close the window. All of these comments are guarded by a big:
"I'm not sure if this really belongs in the kernel." Anyway, here goes.

On Wed, Apr 10, 2024 at 10:05:50AM +0200, Oleksij Rempel wrote:
> IEEE 802.1q specification provides recommendation and examples which can
> be used as good default values for different drivers.
>
> This patch implements mapping examples documented in IEEE 802.1Q-2022 in
> Annex I "I.3 Traffic type to traffic class mapping" and IETF DSCP naming
> and mapping DSCP to Traffic Type inspired by RFC8325.
>
> This helpers will be used in followup patches for dsa/microchip DCB
> implementation.
>
> Signed-off-by: Oleksij Rempel <[email protected]>
> ---
> diff --git a/include/net/ieee8021q.h b/include/net/ieee8021q.h
> new file mode 100644
> index 0000000000000..3bec7ec951362
> --- /dev/null
> +++ b/include/net/ieee8021q.h
> @@ -0,0 +1,55 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (c) 2024 Pengutronix, Oleksij Rempel <[email protected]> */
> +
> +#ifndef _NET_IEEE8021Q_H
> +#define _NET_IEEE8021Q_H
> +
> +#include <linux/errno.h>
> +
> +/**
> + * enum ieee8021q_traffic_type - 802.1Q traffic type priority values (802.1Q-2022)
> + *
> + * @IEEE8021Q_TT_BK: Background
> + * @IEEE8021Q_TT_BE: Best Effort (default). According to 802.1Q-2022, BE is 0
> + * but has higher priority than BK which is 1.
> + * @IEEE8021Q_TT_EE: Excellent Effort
> + * @IEEE8021Q_TT_CA: Critical Applications
> + * @IEEE8021Q_TT_VI: Video, < 100 ms latency and jitter
> + * @IEEE8021Q_TT_VO: Voice, < 10 ms latency and jitter
> + * @IEEE8021Q_TT_IC: Internetwork Control
> + * @IEEE8021Q_TT_NC: Network Control

We get kernel-doc warnings about IEEE8021Q_TT_MAX not being documented.
Simon also suggested to make it private, which I guess will work.

> + */
> +enum ieee8021q_traffic_type {
> + IEEE8021Q_TT_BK = 0,
> + IEEE8021Q_TT_BE = 1,
> + IEEE8021Q_TT_EE = 2,
> + IEEE8021Q_TT_CA = 3,
> + IEEE8021Q_TT_VI = 4,
> + IEEE8021Q_TT_VO = 5,
> + IEEE8021Q_TT_IC = 6,
> + IEEE8021Q_TT_NC = 7,
> +
> + IEEE8021Q_TT_MAX,
> +};
> +
> +#define SIMPLE_IETF_DSCP_TO_IEEE8021Q_TT(dscp) ((dscp >> 3) & 0x7)
> +
> +#if IS_ENABLED(CONFIG_NET_IEEE8021Q_HELPERS)
> +
> +int ietf_dscp_to_ieee8021q_tt(u8 dscp);
> +int ieee8021q_tt_to_tc(unsigned int tt, unsigned int num_queues);
> +
> +#else
> +
> +static inline int ietf_dscp_to_ieee8021q_tt(int dscp)

Function prototype differs when CONFIG_NET_IEEE8021Q_HELPERS is disabled
and when it is enabled (u8 dscp vs int dscp).

> +{
> + return -EOPNOTSUPP;
> +}
> +
> +static inline int ieee8021q_tt_to_tc(int tt, int num_queues)

Same here (unsigned int tt vs int tt).

> +{
> + return -EOPNOTSUPP;
> +}
> +
> +#endif
> +#endif /* _NET_IEEE8021Q_H */
> diff --git a/net/Kconfig b/net/Kconfig
> index d5ab791f7afa2..f0a8692496ffa 100644
> --- a/net/Kconfig
> +++ b/net/Kconfig
> @@ -452,6 +452,9 @@ config GRO_CELLS
> config SOCK_VALIDATE_XMIT
> bool
>
> +config NET_IEEE8021Q_HELPERS
> + bool
> +
> config NET_SELFTESTS
> def_tristate PHYLIB
> depends on PHYLIB && INET
> diff --git a/net/core/Makefile b/net/core/Makefile
> index 21d6fbc7e884c..62be9aef25285 100644
> --- a/net/core/Makefile
> +++ b/net/core/Makefile
> @@ -26,6 +26,7 @@ obj-$(CONFIG_NETPOLL) += netpoll.o
> obj-$(CONFIG_FIB_RULES) += fib_rules.o
> obj-$(CONFIG_TRACEPOINTS) += net-traces.o
> obj-$(CONFIG_NET_DROP_MONITOR) += drop_monitor.o
> +obj-$(CONFIG_NET_IEEE8021Q_HELPERS) += ieee8021q_helpers.o
> obj-$(CONFIG_NET_SELFTESTS) += selftests.o
> obj-$(CONFIG_NETWORK_PHY_TIMESTAMPING) += timestamping.o
> obj-$(CONFIG_NET_PTP_CLASSIFY) += ptp_classifier.o
> diff --git a/net/core/ieee8021q_helpers.c b/net/core/ieee8021q_helpers.c
> new file mode 100644
> index 0000000000000..74b42334746da
> --- /dev/null
> +++ b/net/core/ieee8021q_helpers.c
> @@ -0,0 +1,208 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2024 Pengutronix, Oleksij Rempel <[email protected]>
> +
> +#include <linux/array_size.h>
> +#include <linux/printk.h>
> +#include <linux/types.h>
> +#include <net/dscp.h>
> +#include <net/ieee8021q.h>
> +
> +/* The following arrays map Traffic Types (TT) to traffic classes (TC) for
> + * different number of queues as shown in the example provided by
> + * IEEE 802.1Q-2022 in Annex I "I.3 Traffic type to traffic class mapping" and
> + * Table I-1 "Traffic type to traffic class mapping".
> + */
> +static const u8 ieee8021q_8queue_tt_tc_map[] = {
> + [IEEE8021Q_TT_BK] = 0,
> + [IEEE8021Q_TT_BE] = 1,
> + [IEEE8021Q_TT_EE] = 2,
> + [IEEE8021Q_TT_CA] = 3,
> + [IEEE8021Q_TT_VI] = 4,
> + [IEEE8021Q_TT_VO] = 5,
> + [IEEE8021Q_TT_IC] = 6,
> + [IEEE8021Q_TT_NC] = 7,
> +};
> +
> +static const u8 ieee8021q_7queue_tt_tc_map[] = {
> + [IEEE8021Q_TT_BK] = 0,
> + [IEEE8021Q_TT_BE] = 1,
> + [IEEE8021Q_TT_EE] = 2,
> + [IEEE8021Q_TT_CA] = 3,
> + [IEEE8021Q_TT_VI] = 4, [IEEE8021Q_TT_VO] = 4,
> + [IEEE8021Q_TT_IC] = 5,
> + [IEEE8021Q_TT_NC] = 6,
> +};
> +
> +static const u8 ieee8021q_6queue_tt_tc_map[] = {
> + [IEEE8021Q_TT_BK] = 0,
> + [IEEE8021Q_TT_BE] = 1,
> + [IEEE8021Q_TT_EE] = 2, [IEEE8021Q_TT_CA] = 2,
> + [IEEE8021Q_TT_VI] = 3, [IEEE8021Q_TT_VO] = 3,
> + [IEEE8021Q_TT_IC] = 4,
> + [IEEE8021Q_TT_NC] = 5,
> +};
> +
> +static const u8 ieee8021q_5queue_tt_tc_map[] = {
> + [IEEE8021Q_TT_BK] = 0, [IEEE8021Q_TT_BE] = 0,
> + [IEEE8021Q_TT_EE] = 1, [IEEE8021Q_TT_CA] = 1,
> + [IEEE8021Q_TT_VI] = 2, [IEEE8021Q_TT_VO] = 2,
> + [IEEE8021Q_TT_IC] = 3,
> + [IEEE8021Q_TT_NC] = 4,
> +};
> +
> +static const u8 ieee8021q_4queue_tt_tc_map[] = {
> + [IEEE8021Q_TT_BK] = 0, [IEEE8021Q_TT_BE] = 0,
> + [IEEE8021Q_TT_EE] = 1, [IEEE8021Q_TT_CA] = 1,
> + [IEEE8021Q_TT_VI] = 2, [IEEE8021Q_TT_VO] = 2,
> + [IEEE8021Q_TT_IC] = 3, [IEEE8021Q_TT_NC] = 3,
> +};
> +
> +static const u8 ieee8021q_3queue_tt_tc_map[] = {
> + [IEEE8021Q_TT_BK] = 0, [IEEE8021Q_TT_BE] = 0,
> + [IEEE8021Q_TT_EE] = 0, [IEEE8021Q_TT_CA] = 0,
> + [IEEE8021Q_TT_VI] = 1, [IEEE8021Q_TT_VO] = 1,
> + [IEEE8021Q_TT_IC] = 2, [IEEE8021Q_TT_NC] = 2,
> +};
> +
> +static const u8 ieee8021q_2queue_tt_tc_map[] = {
> + [IEEE8021Q_TT_BK] = 0, [IEEE8021Q_TT_BE] = 0,
> + [IEEE8021Q_TT_EE] = 0, [IEEE8021Q_TT_CA] = 0,
> + [IEEE8021Q_TT_VI] = 1, [IEEE8021Q_TT_VO] = 1,
> + [IEEE8021Q_TT_IC] = 1, [IEEE8021Q_TT_NC] = 1,
> +};
> +
> +static const u8 ieee8021q_1queue_tt_tc_map[] = {
> + [IEEE8021Q_TT_BK] = 0, [IEEE8021Q_TT_BE] = 0,
> + [IEEE8021Q_TT_EE] = 0, [IEEE8021Q_TT_CA] = 0,
> + [IEEE8021Q_TT_VI] = 0, [IEEE8021Q_TT_VO] = 0,
> + [IEEE8021Q_TT_IC] = 0, [IEEE8021Q_TT_NC] = 0,
> +};
> +
> +/**
> + * ieee8021q_tt_to_tc - Map IEEE 802.1Q Traffic Type to Traffic Class
> + * @tt: IEEE 802.1Q Traffic Type
> + * @num_queues: Number of queues
> + *
> + * This function maps an IEEE 802.1Q Traffic Type to a Traffic Class (TC) based
> + * on the number of queues configured on the switch. The mapping is based on the

s/switch/NIC/, ideally it should be useful beyond switches :)

> + * example provided by IEEE 802.1Q-2022 in Annex I "I.3 Traffic type to traffic
> + * class mapping" and Table I-1 "Traffic type to traffic class mapping".
> + *
> + * Return: Traffic Class corresponding to the given Traffic Type or -EINVAL if
> + * the number of queues is not supported. -EINVAL is used to differentiate from

Needs to also describe the other error case, or be less specific.

> + * -EOPNOTSUPP which is used to indicate that this library function is not
> + * compiled in.
> + */
> +int ieee8021q_tt_to_tc(unsigned int tt, unsigned int num_queues)

Can the enum ieee8021q_traffic_type be used instead of unsigned int?

> +{
> + if (tt >= IEEE8021Q_TT_MAX) {
> + pr_err("Requested Traffic Type (%d) is out of range (%d)\n", tt,
> + IEEE8021Q_TT_MAX);
> + return -EINVAL;
> + }
> +
> + switch (num_queues) {
> + case 8:
> + compiletime_assert(ARRAY_SIZE(ieee8021q_8queue_tt_tc_map) !=
> + IEEE8021Q_TT_MAX - 1,
> + "ieee8021q_8queue_tt_tc_map != max - 1");
> + return ieee8021q_8queue_tt_tc_map[tt];
> + case 7:
> + compiletime_assert(ARRAY_SIZE(ieee8021q_7queue_tt_tc_map) !=
> + IEEE8021Q_TT_MAX - 1,
> + "ieee8021q_7queue_tt_tc_map != max - 1");
> +
> + return ieee8021q_7queue_tt_tc_map[tt];
> + case 6:
> + compiletime_assert(ARRAY_SIZE(ieee8021q_6queue_tt_tc_map) !=
> + IEEE8021Q_TT_MAX - 1,
> + "ieee8021q_6queue_tt_tc_map != max - 1");
> +
> + return ieee8021q_6queue_tt_tc_map[tt];
> + case 5:
> + compiletime_assert(ARRAY_SIZE(ieee8021q_5queue_tt_tc_map) !=
> + IEEE8021Q_TT_MAX - 1,
> + "ieee8021q_5queue_tt_tc_map != max - 1");
> +
> + return ieee8021q_5queue_tt_tc_map[tt];
> + case 4:
> + compiletime_assert(ARRAY_SIZE(ieee8021q_4queue_tt_tc_map) !=
> + IEEE8021Q_TT_MAX - 1,
> + "ieee8021q_4queue_tt_tc_map != max - 1");
> +
> + return ieee8021q_4queue_tt_tc_map[tt];
> + case 3:
> + compiletime_assert(ARRAY_SIZE(ieee8021q_3queue_tt_tc_map) !=
> + IEEE8021Q_TT_MAX - 1,
> + "ieee8021q_3queue_tt_tc_map != max - 1");
> +
> + return ieee8021q_3queue_tt_tc_map[tt];
> + case 2:
> + compiletime_assert(ARRAY_SIZE(ieee8021q_2queue_tt_tc_map) !=
> + IEEE8021Q_TT_MAX - 1,
> + "ieee8021q_2queue_tt_tc_map != max - 1");
> +
> + return ieee8021q_2queue_tt_tc_map[tt];
> + case 1:
> + compiletime_assert(ARRAY_SIZE(ieee8021q_1queue_tt_tc_map) !=
> + IEEE8021Q_TT_MAX - 1,
> + "ieee8021q_1queue_tt_tc_map != max - 1");
> +
> + return ieee8021q_1queue_tt_tc_map[tt];
> + }
> +
> + pr_err("Invalid number of queues %d\n", num_queues);
> +
> + return -EINVAL;
> +}
> +EXPORT_SYMBOL_GPL(ieee8021q_tt_to_tc);
> +
> +/**
> + * ietf_dscp_to_ieee8021q_tt - Map IETF DSCP to IEEE 802.1Q Traffic Type
> + * @dscp: IETF DSCP value
> + *
> + * This function maps an IETF DSCP value to an IEEE 802.1Q Traffic Type (TT).
> + * Since there is no corresponding mapping between DSCP and IEEE 802.1Q Traffic
> + * Type, this function is inspired by the RFC8325 documentation which describe
> + * the mapping between DSCP and 802.11 User Priority (UP) values.
> + *
> + * Return: IEEE 802.1Q Traffic Type corresponding to the given DSCP value
> + */
> +int ietf_dscp_to_ieee8021q_tt(u8 dscp)
> +{
> + switch (dscp) {
> + case DSCP_CS0:
> + case DSCP_AF11:
> + case DSCP_AF12:
> + case DSCP_AF13:

Is it correct for AF11, AF12, AF13 to be classified together with CS0
rather than with CS1? It looks strange when their upper 3 bits are the
same as CS1.

> + return IEEE8021Q_TT_BE;
> + case DSCP_CS1:
> + return IEEE8021Q_TT_BK;
> + case DSCP_CS2:
> + case DSCP_AF21:
> + case DSCP_AF22:
> + case DSCP_AF23:
> + return IEEE8021Q_TT_EE;
> + case DSCP_CS3:
> + case DSCP_AF31:
> + case DSCP_AF32:
> + case DSCP_AF33:
> + return IEEE8021Q_TT_CA;
> + case DSCP_CS4:
> + case DSCP_AF41:
> + case DSCP_AF42:
> + case DSCP_AF43:
> + return IEEE8021Q_TT_VI;
> + case DSCP_CS5:
> + case DSCP_EF:
> + case DSCP_VOICE_ADMIT:
> + return IEEE8021Q_TT_VO;
> + case DSCP_CS6:
> + return IEEE8021Q_TT_IC;
> + case DSCP_CS7:
> + return IEEE8021Q_TT_NC;
> + }
> +
> + return SIMPLE_IETF_DSCP_TO_IEEE8021Q_TT(dscp);
> +}
> +EXPORT_SYMBOL_GPL(ietf_dscp_to_ieee8021q_tt);
> --
> 2.39.2
>