| Message ID | 20201031202522.247924-1-vlad@buslov.dev (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Stephen Hemminger |
| Headers | show |
| Series | [iproute2-next] tc: implement support for action terse dump | expand |
On 10/31/20 2:25 PM, Vlad Buslov wrote: > diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h > index 5ad84e663d01..b486f52900f0 100644 > --- a/include/uapi/linux/rtnetlink.h > +++ b/include/uapi/linux/rtnetlink.h > @@ -768,8 +768,12 @@ enum { > * actions in a dump. All dump responses will contain the number of actions > * being dumped stored in for user app's consumption in TCA_ROOT_COUNT > * > + * TCA_FLAG_TERSE_DUMP user->kernel to request terse (brief) dump that only > + * includes essential action info (kind, index, etc.) > + * > */ > #define TCA_FLAG_LARGE_DUMP_ON (1 << 0) > +#define TCA_FLAG_TERSE_DUMP (1 << 1) > there is an existing TCA_DUMP_FLAGS_TERSE. How does this differ and if it really is needed please make it different enough and documented to avoid confusion.
On Tue 03 Nov 2020 at 03:48, David Ahern <dsahern@gmail.com> wrote: > On 10/31/20 2:25 PM, Vlad Buslov wrote: >> diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h >> index 5ad84e663d01..b486f52900f0 100644 >> --- a/include/uapi/linux/rtnetlink.h >> +++ b/include/uapi/linux/rtnetlink.h >> @@ -768,8 +768,12 @@ enum { >> * actions in a dump. All dump responses will contain the number of actions >> * being dumped stored in for user app's consumption in TCA_ROOT_COUNT >> * >> + * TCA_FLAG_TERSE_DUMP user->kernel to request terse (brief) dump that only >> + * includes essential action info (kind, index, etc.) >> + * >> */ >> #define TCA_FLAG_LARGE_DUMP_ON (1 << 0) >> +#define TCA_FLAG_TERSE_DUMP (1 << 1) >> > > there is an existing TCA_DUMP_FLAGS_TERSE. How does this differ and if > it really is needed please make it different enough and documented to > avoid confusion. TCA_FLAG_TERSE_DUMP is a bit in TCA_ROOT_FLAGS tlv which is basically "action flags". TCA_DUMP_FLAGS_TERSE is a bit in TCA_DUMP_FLAGS tlv which is dedicated flags attribute for filter dump. We can't just reuse existing filter dump constant because its value "1" is already taken by TCA_FLAG_LARGE_DUMP_ON. This might look confusing, but what do you suggest? Those are two unrelated tlv's. I can rename the constant name to TCA_FLAG_ACTION_TERSE_DUMP to signify that the flag is action specific, but that would make the naming inconsistent with existing TCA_FLAG_LARGE_DUMP_ON.
On 2020-11-03 10:07 a.m., Vlad Buslov wrote: > > On Tue 03 Nov 2020 at 03:48, David Ahern <dsahern@gmail.com> wrote: >> On 10/31/20 2:25 PM, Vlad Buslov wrote: >>> diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h >>> index 5ad84e663d01..b486f52900f0 100644 >>> --- a/include/uapi/linux/rtnetlink.h >>> +++ b/include/uapi/linux/rtnetlink.h >>> @@ -768,8 +768,12 @@ enum { >>> * actions in a dump. All dump responses will contain the number of actions >>> * being dumped stored in for user app's consumption in TCA_ROOT_COUNT >>> * >>> + * TCA_FLAG_TERSE_DUMP user->kernel to request terse (brief) dump that only >>> + * includes essential action info (kind, index, etc.) >>> + * >>> */ >>> #define TCA_FLAG_LARGE_DUMP_ON (1 << 0) >>> +#define TCA_FLAG_TERSE_DUMP (1 << 1) >>> >> >> there is an existing TCA_DUMP_FLAGS_TERSE. How does this differ and if >> it really is needed please make it different enough and documented to >> avoid confusion. > > TCA_FLAG_TERSE_DUMP is a bit in TCA_ROOT_FLAGS tlv which is basically > "action flags". TCA_DUMP_FLAGS_TERSE is a bit in TCA_DUMP_FLAGS tlv > which is dedicated flags attribute for filter dump. We can't just reuse > existing filter dump constant because its value "1" is already taken by > TCA_FLAG_LARGE_DUMP_ON. This might look confusing, but what do you > suggest? Those are two unrelated tlv's. I can rename the constant name > to TCA_FLAG_ACTION_TERSE_DUMP to signify that the flag is action > specific, but that would make the naming inconsistent with existing > TCA_FLAG_LARGE_DUMP_ON. > Its unfortunate that the TCA_ prefix ended being used for both filters and actions. Since we only have a couple of flags maybe it is not too late to have a prefix TCAA_ ? For existing flags something like a #define TCAA_FLAG_LARGE_DUMP_ON TCA_FLAG_LARGE_DUMP_ON in the uapi header will help. Of course that would be a separate patch which will require conversion code in both the kernel and user space. FWIW, the patch is good for what i tested. So even if you do send an update with a name change please add: Acked-by: Jamal Hadi Salim <jhs@mojatatu.com> cheers, jamal
On Tue 03 Nov 2020 at 23:59, Jamal Hadi Salim <jhs@mojatatu.com> wrote: > On 2020-11-03 10:07 a.m., Vlad Buslov wrote: >> >> On Tue 03 Nov 2020 at 03:48, David Ahern <dsahern@gmail.com> wrote: >>> On 10/31/20 2:25 PM, Vlad Buslov wrote: >>>> diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h >>>> index 5ad84e663d01..b486f52900f0 100644 >>>> --- a/include/uapi/linux/rtnetlink.h >>>> +++ b/include/uapi/linux/rtnetlink.h >>>> @@ -768,8 +768,12 @@ enum { >>>> * actions in a dump. All dump responses will contain the number of actions >>>> * being dumped stored in for user app's consumption in TCA_ROOT_COUNT >>>> * >>>> + * TCA_FLAG_TERSE_DUMP user->kernel to request terse (brief) dump that only >>>> + * includes essential action info (kind, index, etc.) >>>> + * >>>> */ >>>> #define TCA_FLAG_LARGE_DUMP_ON (1 << 0) >>>> +#define TCA_FLAG_TERSE_DUMP (1 << 1) >>>> >>> >>> there is an existing TCA_DUMP_FLAGS_TERSE. How does this differ and if >>> it really is needed please make it different enough and documented to >>> avoid confusion. >> >> TCA_FLAG_TERSE_DUMP is a bit in TCA_ROOT_FLAGS tlv which is basically >> "action flags". TCA_DUMP_FLAGS_TERSE is a bit in TCA_DUMP_FLAGS tlv >> which is dedicated flags attribute for filter dump. We can't just reuse >> existing filter dump constant because its value "1" is already taken by >> TCA_FLAG_LARGE_DUMP_ON. This might look confusing, but what do you >> suggest? Those are two unrelated tlv's. I can rename the constant name >> to TCA_FLAG_ACTION_TERSE_DUMP to signify that the flag is action >> specific, but that would make the naming inconsistent with existing >> TCA_FLAG_LARGE_DUMP_ON. >> > > Its unfortunate that the TCA_ prefix ended being used for both filters > and actions. Since we only have a couple of flags maybe it is not too > late to have a prefix TCAA_ ? For existing flags something like a > #define TCAA_FLAG_LARGE_DUMP_ON TCA_FLAG_LARGE_DUMP_ON > in the uapi header will help. Of course that would be a separate > patch which will require conversion code in both the kernel and user > space. I can send a followup patch, assuming David is satisfied with proposed change. > > FWIW, the patch is good for what i tested. So even if you do send an > update with a name change please add: > > Acked-by: Jamal Hadi Salim <jhs@mojatatu.com> > > cheers, > jamal
On 11/6/20 12:16 PM, Vlad Buslov wrote: >>> >> >> Its unfortunate that the TCA_ prefix ended being used for both filters >> and actions. Since we only have a couple of flags maybe it is not too >> late to have a prefix TCAA_ ? For existing flags something like a >> #define TCAA_FLAG_LARGE_DUMP_ON TCA_FLAG_LARGE_DUMP_ON >> in the uapi header will help. Of course that would be a separate >> patch which will require conversion code in both the kernel and user >> space. > > I can send a followup patch, assuming David is satisfied with proposed > change. > fine with me.
On Fri, 6 Nov 2020 13:25:19 -0700 David Ahern wrote: > On 11/6/20 12:16 PM, Vlad Buslov wrote: > >>> > >> > >> Its unfortunate that the TCA_ prefix ended being used for both filters > >> and actions. Since we only have a couple of flags maybe it is not too > >> late to have a prefix TCAA_ ? For existing flags something like a > >> #define TCAA_FLAG_LARGE_DUMP_ON TCA_FLAG_LARGE_DUMP_ON > >> in the uapi header will help. Of course that would be a separate > >> patch which will require conversion code in both the kernel and user > >> space. > > > > I can send a followup patch, assuming David is satisfied with proposed > > change. > > fine with me. In some ways it helps in some ways it adds to a mix of confusing acronyms in which TC truly excels. Are we saying that all new action attrs/defines going forward should be prefixed with TCAA, and all new filters with TCFA? Otherwise, if it's a one off, I'd vote for including _ACT in the name instead.
diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h index 5ad84e663d01..b486f52900f0 100644 --- a/include/uapi/linux/rtnetlink.h +++ b/include/uapi/linux/rtnetlink.h @@ -768,8 +768,12 @@ enum { * actions in a dump. All dump responses will contain the number of actions * being dumped stored in for user app's consumption in TCA_ROOT_COUNT * + * TCA_FLAG_TERSE_DUMP user->kernel to request terse (brief) dump that only + * includes essential action info (kind, index, etc.) + * */ #define TCA_FLAG_LARGE_DUMP_ON (1 << 0) +#define TCA_FLAG_TERSE_DUMP (1 << 1) /* New extended info filters for IFLA_EXT_MASK */ #define RTEXT_FILTER_VF (1 << 0) diff --git a/man/man8/tc.8 b/man/man8/tc.8 index e8622053df65..4338572a36f3 100644 --- a/man/man8/tc.8 +++ b/man/man8/tc.8 @@ -858,7 +858,7 @@ alias. .BR "\-br" , " \-brief" Print only essential data needed to identify the filter and action (handle, cookie, etc.) and stats. This option is currently only supported by -.BR "tc filter show " command. +.BR "tc filter show " and " tc actions ls " commands. .SH "EXAMPLES" .PP diff --git a/tc/m_action.c b/tc/m_action.c index 66e672453c25..b640b3c88b7b 100644 --- a/tc/m_action.c +++ b/tc/m_action.c @@ -374,6 +374,11 @@ static int tc_print_one_action(FILE *f, struct rtattr *arg) if (err < 0) return err; + if (brief && tb[TCA_ACT_INDEX]) { + print_uint(PRINT_ANY, "index", "\t index %u", + rta_getattr_u32(tb[TCA_ACT_INDEX])); + print_nl(); + } if (show_stats && tb[TCA_ACT_STATS]) { print_string(PRINT_FP, NULL, "\tAction statistics:", NULL); print_nl(); @@ -737,6 +742,10 @@ static int tc_act_list_or_flush(int *argc_p, char ***argv_p, int event) tail3 = NLMSG_TAIL(&req.n); flag_select.value |= TCA_FLAG_LARGE_DUMP_ON; flag_select.selector |= TCA_FLAG_LARGE_DUMP_ON; + if (brief) { + flag_select.value |= TCA_FLAG_TERSE_DUMP; + flag_select.selector |= TCA_FLAG_TERSE_DUMP; + } addattr_l(&req.n, MAX_MSG, TCA_ROOT_FLAGS, &flag_select, sizeof(struct nla_bitfield32)); tail3->rta_len = (void *) NLMSG_TAIL(&req.n) - (void *) tail3;
Implement support for action terse dump using new TCA_FLAG_TERSE_DUMP value of TCA_ROOT_FLAGS tlv. Set the flag when user requested it with following example CLI (-br for 'brief'): $ tc -s -br actions ls action tunnel_key total acts 2 action order 0: tunnel_key index 1 Action statistics: Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0) backlog 0b 0p requeues 0 action order 1: tunnel_key index 2 Action statistics: Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0) backlog 0b 0p requeues 0 In terse mode dump only outputs essential data needed to identify the action (kind, index) and stats, if requested by the user. Signed-off-by: Vlad Buslov <vlad@buslov.dev> Suggested-by: Jamal Hadi Salim <jhs@mojatatu.com> --- include/uapi/linux/rtnetlink.h | 4 ++++ man/man8/tc.8 | 2 +- tc/m_action.c | 9 +++++++++ 3 files changed, 14 insertions(+), 1 deletion(-)