Date: Tue, 23 May 2023 18:35:35 +0200
From: Petr Machata <petrm@...dia.com>
To: Daniel Machon <daniel.machon@...rochip.com>
CC: <netdev@...r.kernel.org>, <dsahern@...nel.org>,
	<stephen@...workplumber.org>, <petrm@...dia.com>,
	<UNGLinuxDriver@...rochip.com>
Subject: Re: [PATCH iproute2-next 6/9] dcb: rewr: add new dcb-rewr subcommand


Daniel Machon <daniel.machon@...rochip.com> writes:

> Add a new subcommand 'rewr' for configuring the in-kernel DCB rewrite
> table. The rewr-table of the kernel is similar to the APP-table, and so
> is this new subcommand. Therefore, much of the existing bookkeeping code
> from dcb-app, can be reused in the dcb-rewr implementation.
>
> Initially, only support for configuring PCP and DSCP-based rewrite has
> been added.

That's reasonable.

> Signed-off-by: Daniel Machon <daniel.machon@...rochip.com>
> ---
>  dcb/Makefile   |   3 +-
>  dcb/dcb.c      |   4 +-
>  dcb/dcb.h      |   3 +
>  dcb/dcb_app.h  |   1 +
>  dcb/dcb_rewr.c | 332 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 341 insertions(+), 2 deletions(-)
>
> diff --git a/dcb/Makefile b/dcb/Makefile
> index dd41a559a0c8..10794c9dc19f 100644
> --- a/dcb/Makefile
> +++ b/dcb/Makefile
> @@ -8,7 +8,8 @@ DCBOBJ = dcb.o \
>           dcb_ets.o \
>           dcb_maxrate.o \
>           dcb_pfc.o \
> -         dcb_apptrust.o
> +         dcb_apptrust.o \
> +         dcb_rewr.o
>  TARGETS += dcb
>  LDLIBS += -lm
>  
> diff --git a/dcb/dcb.c b/dcb/dcb.c
> index 9b996abac529..fe0a0f04143d 100644
> --- a/dcb/dcb.c
> +++ b/dcb/dcb.c
> @@ -470,7 +470,7 @@ static void dcb_help(void)
>  	fprintf(stderr,
>  		"Usage: dcb [ OPTIONS ] OBJECT { COMMAND | help }\n"
>  		"       dcb [ -f | --force ] { -b | --batch } filename [ -n | --netns ] netnsname\n"
> -		"where  OBJECT := { app | apptrust | buffer | dcbx | ets | maxrate | pfc }\n"
> +		"where  OBJECT := { app | apptrust | buffer | dcbx | ets | maxrate | pfc | rewr }\n"
>  		"       OPTIONS := [ -V | --Version | -i | --iec | -j | --json\n"
>  		"                  | -N | --Numeric | -p | --pretty\n"
>  		"                  | -s | --statistics | -v | --verbose]\n");
> @@ -485,6 +485,8 @@ static int dcb_cmd(struct dcb *dcb, int argc, char **argv)
>  		return dcb_cmd_app(dcb, argc - 1, argv + 1);
>  	} else if (strcmp(*argv, "apptrust") == 0) {
>  		return dcb_cmd_apptrust(dcb, argc - 1, argv + 1);
> +	} else if (strcmp(*argv, "rewr") == 0) {
> +		return dcb_cmd_rewr(dcb, argc - 1, argv + 1);
>  	} else if (matches(*argv, "buffer") == 0) {
>  		return dcb_cmd_buffer(dcb, argc - 1, argv + 1);
>  	} else if (matches(*argv, "dcbx") == 0) {
> diff --git a/dcb/dcb.h b/dcb/dcb.h
> index 4c8a4aa25e0c..39a04f1c59df 100644
> --- a/dcb/dcb.h
> +++ b/dcb/dcb.h
> @@ -56,6 +56,9 @@ void dcb_print_array_on_off(const __u8 *array, size_t size);
>  void dcb_print_array_kw(const __u8 *array, size_t array_size,
>  			const char *const kw[], size_t kw_size);
>  
> +/* dcb_rewr.c */
> +int dcb_cmd_rewr(struct dcb *dcb, int argc, char **argv);
> +
>  /* dcb_apptrust.c */
>  
>  int dcb_cmd_apptrust(struct dcb *dcb, int argc, char **argv);
> diff --git a/dcb/dcb_app.h b/dcb/dcb_app.h
> index 8f048605c3a8..02c9eb03f6c2 100644
> --- a/dcb/dcb_app.h
> +++ b/dcb/dcb_app.h
> @@ -22,6 +22,7 @@ struct dcb_app_parse_mapping {
>  };
>  
>  #define DCB_APP_PCP_MAX 15
> +#define DCB_APP_DSCP_MAX 63

It would be nice to have dcb_app_parse_mapping_dscp_prio() use that
define now that it exists. Back then I figured the value 63 in the
context that mentions DSCP is clear enough, and the value itself being
grounded in IEEE won't change, but... um, yeah, if the define exists,
let's use it :)

>  
>  int dcb_cmd_app(struct dcb *dcb, int argc, char **argv);
>  
> diff --git a/dcb/dcb_rewr.c b/dcb/dcb_rewr.c
> new file mode 100644
> index 000000000000..731ba78977e2
> --- /dev/null
> +++ b/dcb/dcb_rewr.c
> @@ -0,0 +1,332 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +#include <errno.h>
> +#include <linux/dcbnl.h>
> +#include <stdio.h>
> +
> +#include "dcb.h"
> +#include "utils.h"
> +
> +static void dcb_rewr_help_add(void)
> +{
> +	fprintf(stderr,
> +		"Usage: dcb rewr { add | del | replace } dev STRING\n"
> +		"           [ prio-pcp PRIO:PCP   ]\n"
> +		"           [ prio-dscp PRIO:DSCP ]\n"
> +		"\n"
> +		" where PRIO := { 0 .. 7               }\n"
> +		"       PCP  := { 0(nd/de) .. 7(nd/de) }\n"
> +		"       DSCP := { 0 .. 63              }\n"

I was wondering if you had done something with this instance of 63 ;)

Can you also drop all those extra spaces? Here and elsewhere. These
tabular layouts only ever lead to later reformatting as longer lines
break it.

> +		"\n"
> +	);
> +}
> +
> +static void dcb_rewr_help_show_flush(void)
> +{
> +	fprintf(stderr,
> +		"Usage: dcb rewr { show | flush } dev STRING\n"
> +		"           [ prio-pcp  ]\n"
> +		"           [ prio-dscp ]\n"
> +		"\n"
> +	);
> +}
> +
> +static void dcb_rewr_help(void)
> +{
> +	fprintf(stderr,
> +		"Usage: dcb rewr help\n"
> +		"\n"
> +	);
> +	dcb_rewr_help_show_flush();
> +	dcb_rewr_help_add();
> +}
> +
> +static int dcb_rewr_parse_mapping_prio_pcp(__u32 key, char *value, void *data)
> +{
> +	__u32 pcp;
> +
> +	if (dcb_app_parse_pcp(&pcp, value))
> +		return -EINVAL;
> +
> +	return dcb_parse_mapping("PRIO", key, IEEE_8021QAZ_MAX_TCS - 1, "PCP",
> +				 pcp, DCB_APP_PCP_MAX, dcb_app_parse_mapping_cb,
> +				 data);

See, the way it's formatted in app makes it clear what's what. Consider:

	return dcb_parse_mapping("PRIO", key, IEEE_8021QAZ_MAX_TCS - 1,
				 "PCP", pcp, DCB_APP_PCP_MAX,
				 dcb_app_parse_mapping_cb, data);

PRIO has proposed value of "key" and goes up to IEEE_8021QAZ_MAX_TCS - 1,
PCP has "pcp", goes up to DCB_APP_PCP_MAX, please use this callback and
invoke it with "data".

The expression in this patch takes the same amount of space, but it's
much less clear what is what.

The same applies below.

> +}
> +
> +static int dcb_rewr_parse_mapping_prio_dscp(__u32 key, char *value, void *data)
> +{
> +	__u32 dscp;
> +
> +	if (dcb_app_parse_dscp(&dscp, value))
> +		return -EINVAL;
> +
> +	return dcb_parse_mapping("PRIO", key, IEEE_8021QAZ_MAX_TCS - 1, "DSCP",
> +				 dscp, DCB_APP_DSCP_MAX,
> +				 dcb_app_parse_mapping_cb, data);
> +}
> +
> +static void dcb_rewr_print_prio_pcp(const struct dcb *dcb,
> +				    const struct dcb_app_table *tab)
> +{
> +	dcb_app_print_filtered(tab, dcb_app_is_pcp,
> +			       dcb->numeric ? dcb_app_print_pid_dec :
> +					      dcb_app_print_pid_pcp,
> +			       "prio_pcp", "prio-pcp");
> +}
> +
> +static void dcb_rewr_print_prio_dscp(const struct dcb *dcb,
> +				     const struct dcb_app_table *tab)
> +{
> +	dcb_app_print_filtered(tab, dcb_app_is_dscp,
> +			       dcb->numeric ? dcb_app_print_pid_dec :
> +					      dcb_app_print_pid_dscp,
> +			       "prio_dscp", "prio-dscp");
> +}
> +
> +static void dcb_rewr_print(const struct dcb *dcb,
> +			   const struct dcb_app_table *tab)
> +{
> +	dcb_rewr_print_prio_pcp(dcb, tab);
> +	dcb_rewr_print_prio_dscp(dcb, tab);
> +}
> +
> +static int dcb_cmd_rewr_parse_add_del(struct dcb *dcb, const char *dev,
> +				      int argc, char **argv,
> +				      struct dcb_app_table *tab)
> +{
> +	struct dcb_app_parse_mapping pm = { .tab = tab };
> +	int ret;
> +
> +	if (!argc) {
> +		dcb_rewr_help_add();
> +		return 0;
> +	}
> +
> +	do {
> +		if (strcmp(*argv, "help") == 0) {
> +			dcb_rewr_help_add();
> +			return 0;
> +		} else if (strcmp(*argv, "prio-pcp") == 0) {
> +			NEXT_ARG();
> +			pm.selector = DCB_APP_SEL_PCP;
> +			ret = parse_mapping(&argc, &argv, false,
> +					    &dcb_rewr_parse_mapping_prio_pcp,
> +					    &pm);
> +		} else if (strcmp(*argv, "prio-dscp") == 0) {
> +			NEXT_ARG();
> +			pm.selector = IEEE_8021QAZ_APP_SEL_DSCP;
> +			ret = parse_mapping(&argc, &argv, false,
> +					    &dcb_rewr_parse_mapping_prio_dscp,
> +					    &pm);
> +		} else {
> +			fprintf(stderr, "What is \"%s\"?\n", *argv);
> +			dcb_rewr_help_add();
> +			return -EINVAL;
> +		}
> +
> +		if (ret != 0) {
> +			fprintf(stderr, "Invalid mapping %s\n", *argv);
> +			return ret;
> +		}
> +		if (pm.err)
> +			return pm.err;
> +	} while (argc > 0);
> +
> +	return 0;
> +}
> +
> +static int dcb_cmd_rewr_add(struct dcb *dcb, const char *dev, int argc,
> +			    char **argv)
> +{
> +	struct dcb_app_table tab = { .attr = DCB_ATTR_DCB_REWR_TABLE };
> +	int ret;
> +
> +	ret = dcb_cmd_rewr_parse_add_del(dcb, dev, argc, argv, &tab);
> +	if (ret != 0)
> +		return ret;
> +
> +	ret = dcb_app_add_del(dcb, dev, DCB_CMD_IEEE_SET, &tab, NULL);
> +	dcb_app_table_fini(&tab);
> +	return ret;
> +}
> +
> +static int dcb_cmd_rewr_del(struct dcb *dcb, const char *dev, int argc,
> +			    char **argv)
> +{
> +	struct dcb_app_table tab = { .attr = DCB_ATTR_DCB_REWR_TABLE };
> +	int ret;
> +
> +	ret = dcb_cmd_rewr_parse_add_del(dcb, dev, argc, argv, &tab);
> +	if (ret != 0)
> +		return ret;
> +
> +	ret = dcb_app_add_del(dcb, dev, DCB_CMD_IEEE_DEL, &tab, NULL);
> +	dcb_app_table_fini(&tab);
> +	return ret;
> +}
> +
> +static int dcb_cmd_rewr_replace(struct dcb *dcb, const char *dev, int argc,
> +				char **argv)
> +{
> +	struct dcb_app_table orig = { .attr = DCB_ATTR_DCB_REWR_TABLE };
> +	struct dcb_app_table tab = { .attr = DCB_ATTR_DCB_REWR_TABLE };
> +	struct dcb_app_table new = { .attr = DCB_ATTR_DCB_REWR_TABLE };
> +	int ret;
> +
> +	ret = dcb_app_get(dcb, dev, &orig);
> +	if (ret != 0)
> +		return ret;
> +
> +	ret = dcb_cmd_rewr_parse_add_del(dcb, dev, argc, argv, &tab);
> +	if (ret != 0)
> +		goto out;
> +
> +	/* Attempts to add an existing entry would be rejected, so drop
> +	 * these entries from tab.
> +	 */
> +	ret = dcb_app_table_copy(&new, &tab);
> +	if (ret != 0)
> +		goto out;
> +	dcb_app_table_remove_existing(&new, &orig);
> +
> +	ret = dcb_app_add_del(dcb, dev, DCB_CMD_IEEE_SET, &new, NULL);
> +	if (ret != 0) {
> +		fprintf(stderr, "Could not add new rewrite entries\n");
> +		goto out;
> +	}
> +
> +	/* Remove the obsolete entries. */
> +	dcb_app_table_remove_replaced(&orig, &tab);
> +	ret = dcb_app_add_del(dcb, dev, DCB_CMD_IEEE_DEL, &orig, NULL);
> +	if (ret != 0) {
> +		fprintf(stderr, "Could not remove replaced rewrite entries\n");
> +		goto out;
> +	}
> +
> +out:
> +	dcb_app_table_fini(&new);
> +	dcb_app_table_fini(&tab);
> +	dcb_app_table_fini(&orig);
> +	return 0;
> +}
> +
> +
> +static int dcb_cmd_rewr_show(struct dcb *dcb, const char *dev, int argc,
> +			     char **argv)
> +{
> +	struct dcb_app_table tab = { .attr = DCB_ATTR_DCB_REWR_TABLE };
> +	int ret;
> +
> +	ret = dcb_app_get(dcb, dev, &tab);
> +	if (ret != 0)
> +		return ret;
> +
> +	dcb_app_table_sort(&tab);
> +
> +	open_json_object(NULL);
> +
> +	if (!argc) {
> +		dcb_rewr_print(dcb, &tab);
> +		goto out;
> +	}
> +
> +	do {
> +		if (strcmp(*argv, "help") == 0) {
> +			dcb_rewr_help_show_flush();
> +			goto out;
> +		} else if (strcmp(*argv, "prio-pcp") == 0) {
> +			dcb_rewr_print_prio_pcp(dcb, &tab);
> +		} else if (strcmp(*argv, "prio-dscp") == 0) {
> +			dcb_rewr_print_prio_dscp(dcb, &tab);
> +		} else {
> +			fprintf(stderr, "What is \"%s\"?\n", *argv);
> +			dcb_rewr_help_show_flush();
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +
> +		NEXT_ARG_FWD();
> +	} while (argc > 0);
> +
> +out:
> +	close_json_object();
> +	dcb_app_table_fini(&tab);
> +	return ret;
> +}
> +
> +static int dcb_cmd_rewr_flush(struct dcb *dcb, const char *dev, int argc,
> +			      char **argv)
> +{
> +	struct dcb_app_table tab = { .attr = DCB_ATTR_DCB_REWR_TABLE };
> +	int ret;
> +
> +	ret = dcb_app_get(dcb, dev, &tab);
> +	if (ret != 0)
> +		return ret;
> +
> +	if (!argc) {
> +		ret = dcb_app_add_del(dcb, dev, DCB_CMD_IEEE_DEL, &tab,
> +				      NULL);
> +		goto out;
> +	}
> +
> +	do {
> +		if (strcmp(*argv, "help") == 0) {
> +			dcb_rewr_help_show_flush();
> +			goto out;
> +		} else if (strcmp(*argv, "prio-pcp") == 0) {
> +			ret = dcb_app_add_del(dcb, dev, DCB_CMD_IEEE_DEL, &tab,
> +					      &dcb_app_is_pcp);
> +			if (ret != 0)
> +				goto out;
> +		} else if (strcmp(*argv, "prio-dscp") == 0) {
> +			ret = dcb_app_add_del(dcb, dev, DCB_CMD_IEEE_DEL, &tab,
> +					      &dcb_app_is_dscp);
> +			if (ret != 0)
> +				goto out;
> +		} else {
> +			fprintf(stderr, "What is \"%s\"?\n", *argv);
> +			dcb_rewr_help_show_flush();
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +
> +		NEXT_ARG_FWD();
> +	} while (argc > 0);
> +
> +out:
> +	dcb_app_table_fini(&tab);
> +	return ret;
> +}
> +
> +int dcb_cmd_rewr(struct dcb *dcb, int argc, char **argv)
> +{
> +	if (!argc || strcmp(*argv, "help") == 0) {
> +		dcb_rewr_help();
> +		return 0;
> +	} else if (strcmp(*argv, "show") == 0) {
> +		NEXT_ARG_FWD();
> +		return dcb_cmd_parse_dev(dcb, argc, argv, dcb_cmd_rewr_show,
> +					 dcb_rewr_help_show_flush);
> +	} else if (strcmp(*argv, "flush") == 0) {
> +		NEXT_ARG_FWD();
> +		return dcb_cmd_parse_dev(dcb, argc, argv, dcb_cmd_rewr_flush,
> +					 dcb_rewr_help_show_flush);
> +	} else if (strcmp(*argv, "add") == 0) {
> +		NEXT_ARG_FWD();
> +		return dcb_cmd_parse_dev(dcb, argc, argv, dcb_cmd_rewr_add,
> +					 dcb_rewr_help_add);
> +	} else if (strcmp(*argv, "del") == 0) {
> +		NEXT_ARG_FWD();
> +		return dcb_cmd_parse_dev(dcb, argc, argv, dcb_cmd_rewr_del,
> +					 dcb_rewr_help_add);
> +	} else if (strcmp(*argv, "replace") == 0) {
> +		NEXT_ARG_FWD();
> +		return dcb_cmd_parse_dev(dcb, argc, argv, dcb_cmd_rewr_replace,
> +					 dcb_rewr_help_add);
> +	} else {
> +		fprintf(stderr, "What is \"%s\"?\n", *argv);
> +		dcb_rewr_help();
> +		return -EINVAL;
> +	}
> +}

Powered by blists - more mailing lists