Re: [PATCH v2 iproute2-next 2/2] taprio: support dumping and setting per-tc max SDU

[David Ahern <dsahern@kernel.org>]

On 10/4/22 6:00 AM, Vladimir Oltean wrote:
> diff --git a/tc/q_taprio.c b/tc/q_taprio.c
> index e3af3f3fa047..45f82be1f50a 100644
> --- a/tc/q_taprio.c
> +++ b/tc/q_taprio.c
> @@ -151,13 +151,32 @@ static struct sched_entry *create_entry(uint32_t gatemask, uint32_t interval, ui
>  	return e;
>  }
>  
> +static void add_tc_entries(struct nlmsghdr *n,
> +			   __u32 max_sdu[TC_QOPT_MAX_QUEUE])
> +{
> +	struct rtattr *l;
> +	__u32 tc;
> +
> +	for (tc = 0; tc < TC_QOPT_MAX_QUEUE; tc++) {
> +		l = addattr_nest(n, 1024, TCA_TAPRIO_ATTR_TC_ENTRY | NLA_F_NESTED);
> +
> +		addattr_l(n, 1024, TCA_TAPRIO_TC_ENTRY_INDEX, &tc, sizeof(tc));
> +		addattr_l(n, 1024, TCA_TAPRIO_TC_ENTRY_MAX_SDU,
> +			  &max_sdu[tc], sizeof(max_sdu[tc]));
> +
> +		addattr_nest_end(n, l);


Why the full TC_QOPT_MAX_QUEUE? the parse_opt knows the index of the
last entry used.

> +	}
> +}
> +
>  static int taprio_parse_opt(struct qdisc_util *qu, int argc,
>  			    char **argv, struct nlmsghdr *n, const char *dev)
>  {
> +	__u32 max_sdu[TC_QOPT_MAX_QUEUE] = { };
>  	__s32 clockid = CLOCKID_INVALID;
>  	struct tc_mqprio_qopt opt = { };
>  	__s64 cycle_time_extension = 0;
>  	struct list_head sched_entries;
> +	bool have_tc_entries = false;
>  	struct rtattr *tail, *l;
>  	__u32 taprio_flags = 0;
>  	__u32 txtime_delay = 0;
> @@ -211,6 +230,18 @@ static int taprio_parse_opt(struct qdisc_util *qu, int argc,
>  				free(tmp);
>  				idx++;
>  			}
> +		} else if (strcmp(*argv, "max-sdu") == 0) {
> +			while (idx < TC_QOPT_MAX_QUEUE && NEXT_ARG_OK()) {
> +				NEXT_ARG();
> +				if (get_u32(&max_sdu[idx], *argv, 10)) {
> +					PREV_ARG();
> +					break;
> +				}
> +				idx++;
> +			}
> +			for ( ; idx < TC_QOPT_MAX_QUEUE; idx++)
> +				max_sdu[idx] = 0;

max_sdu is initialized to 0 and you have "have_tc_entries" to detect
multiple options on the command line.

> +			have_tc_entries = true;
>  		} else if (strcmp(*argv, "sched-entry") == 0) {
>  			uint32_t mask, interval;
>  			struct sched_entry *e;
> @@ -341,6 +372,9 @@ static int taprio_parse_opt(struct qdisc_util *qu, int argc,
>  		addattr_l(n, 1024, TCA_TAPRIO_ATTR_SCHED_CYCLE_TIME_EXTENSION,
>  			  &cycle_time_extension, sizeof(cycle_time_extension));
>  
> +	if (have_tc_entries)
> +		add_tc_entries(n, max_sdu);
> +
>  	l = addattr_nest(n, 1024, TCA_TAPRIO_ATTR_SCHED_ENTRY_LIST | NLA_F_NESTED);
>  
>  	err = add_sched_list(&sched_entries, n);
> @@ -430,6 +464,59 @@ static int print_schedule(FILE *f, struct rtattr **tb)
>  	return 0;
>  }
>  
> +static void dump_tc_entry(__u32 max_sdu[TC_QOPT_MAX_QUEUE],
> +			  struct rtattr *item, bool *have_tc_entries)
> +{
> +	struct rtattr *tb[TCA_TAPRIO_TC_ENTRY_MAX + 1];
> +	__u32 tc, val = 0;
> +
> +	parse_rtattr_nested(tb, TCA_TAPRIO_TC_ENTRY_MAX, item);
> +
> +	if (!tb[TCA_TAPRIO_TC_ENTRY_INDEX]) {
> +		fprintf(stderr, "Missing tc entry index\n");
> +		return;
> +	}
> +
> +	tc = rta_getattr_u32(tb[TCA_TAPRIO_TC_ENTRY_INDEX]);
> +
> +	if (tb[TCA_TAPRIO_TC_ENTRY_MAX_SDU])
> +		val = rta_getattr_u32(tb[TCA_TAPRIO_TC_ENTRY_MAX_SDU]);
> +
> +	max_sdu[tc] = val;
> +
> +	*have_tc_entries = true;
> +}
> +
> +static void dump_tc_entries(FILE *f, struct rtattr *opt)
> +{
> +	__u32 max_sdu[TC_QOPT_MAX_QUEUE] = {};
> +	bool have_tc_entries = false;
> +	struct rtattr *i;
> +	int tc, rem;
> +
> +	for (tc = 0; tc < TC_QOPT_MAX_QUEUE; tc++)
> +		max_sdu[tc] = 0;

max_sdu is initialized to 0 above when it is declared.

> +
> +	rem = RTA_PAYLOAD(opt);
> +
> +	for (i = RTA_DATA(opt); RTA_OK(i, rem); i = RTA_NEXT(i, rem)) {
> +		if (i->rta_type != (TCA_TAPRIO_ATTR_TC_ENTRY | NLA_F_NESTED))
> +			continue;
> +
> +		dump_tc_entry(max_sdu, i, &have_tc_entries);
> +	}
> +
> +	if (!have_tc_entries)
> +		return;
> +
> +	open_json_array(PRINT_ANY, "max-sdu");
> +	for (tc = 0; tc < TC_QOPT_MAX_QUEUE; tc++)

you can know the max index so why not use it here?