From: <Daniel.Machon@microchip.com>
To: <petrm@nvidia.com>
Cc: <netdev@vger.kernel.org>, <dsahern@kernel.org>,
<stephen@networkplumber.org>, <UNGLinuxDriver@microchip.com>
Subject: Re: [PATCH iproute2-next 6/9] dcb: rewr: add new dcb-rewr subcommand
Date: Wed, 24 May 2023 06:51:21 +0000 [thread overview]
Message-ID: <ZG2z6XlAEUu3B4yg@DEN-LT-70577> (raw)
In-Reply-To: <874jo3575i.fsf@nvidia.com>
> Daniel Machon <daniel.machon@microchip.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.
Good :)
>
> > Signed-off-by: Daniel Machon <daniel.machon@microchip.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 :)
I introduced it for PCP back then, so why not for DSCP also. Will update
use in dcb-app also.
>
> >
> > 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.
Sure.
>
> > + "\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.
Ack.
>
> > +}
> > +
> > +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;
> > + }
> > +}
>
next prev parent reply other threads:[~2023-05-24 6:51 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-22 18:41 [PATCH iproute2-next 0/9] Introduce new dcb-rewr subcommand Daniel Machon
2023-05-22 18:41 ` [PATCH iproute2-next 1/9] dcb: app: expose dcb-app functions in new header Daniel Machon
2023-05-23 11:18 ` Petr Machata
2023-05-24 6:39 ` Daniel.Machon
2023-05-24 9:28 ` Petr Machata
2023-05-22 18:41 ` [PATCH iproute2-next 2/9] dcb: app: add new dcbnl attribute field Daniel Machon
2023-05-22 18:41 ` [PATCH iproute2-next 3/9] dcb: app: modify dcb-app print functions for dcb-rewr reuse Daniel Machon
2023-05-22 21:33 ` Stephen Hemminger
2023-05-25 7:20 ` Daniel Machon
2023-05-23 13:23 ` Petr Machata
2023-05-24 6:47 ` Daniel.Machon
2023-05-24 9:37 ` Petr Machata
2023-05-22 18:41 ` [PATCH iproute2-next 4/9] dcb: app: modify dcb_app_table_remove_replaced() " Daniel Machon
2023-05-23 14:42 ` Petr Machata
2023-05-22 18:41 ` [PATCH iproute2-next 5/9] dcb: app: modify dcb_app_parse_mapping_cb " Daniel Machon
2023-05-23 16:29 ` Petr Machata
2023-05-22 18:41 ` [PATCH iproute2-next 6/9] dcb: rewr: add new dcb-rewr subcommand Daniel Machon
2023-05-23 16:35 ` Petr Machata
2023-05-24 6:51 ` Daniel.Machon [this message]
2023-05-22 18:41 ` [PATCH iproute2-next 7/9] man: dcb-rewr: add new manpage for dcb-rewr Daniel Machon
2023-05-23 16:56 ` Petr Machata
2023-05-22 18:41 ` [PATCH iproute2-next 8/9] man: dcb: add additional references under 'SEE ALSO' Daniel Machon
2023-05-22 18:41 ` [PATCH iproute2-next 9/9] man: dcb-app: clean up a few mistakes Daniel Machon
2023-05-23 16:49 ` Petr Machata
2023-05-24 6:56 ` Daniel.Machon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZG2z6XlAEUu3B4yg@DEN-LT-70577 \
--to=daniel.machon@microchip.com \
--cc=UNGLinuxDriver@microchip.com \
--cc=dsahern@kernel.org \
--cc=netdev@vger.kernel.org \
--cc=petrm@nvidia.com \
--cc=stephen@networkplumber.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox