[PATCH net-next] net: sched: implement action-specific terse dump

[PATCH net-next] net: sched: implement action-specific terse dump

[Vlad Buslov]

Allow user to request action terse dump with new flag value
TCA_FLAG_TERSE_DUMP. Only output essential action info in terse dump (kind,
stats and index). This is different from filter terse dump where index is
excluded (filter can be identified by its own handle) and cookie is
included.

Move tcf_action_dump_terse() function to the beginning of source file in
order to call it from tcf_dump_walker().

Signed-off-by: Vlad Buslov <vlad@buslov.dev>
Suggested-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 include/uapi/linux/rtnetlink.h |  4 ++
 net/sched/act_api.c            | 69 ++++++++++++++++++----------------
 2 files changed, 41 insertions(+), 32 deletions(-)

diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
index fdd408f6a5d2..d1325ffb0060 100644
--- a/include/uapi/linux/rtnetlink.h
+++ b/include/uapi/linux/rtnetlink.h
@@ -770,8 +770,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/net/sched/act_api.c b/net/sched/act_api.c
index f66417d5d2c3..107e8ce3a57c 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -215,6 +215,36 @@ static size_t tcf_action_fill_size(const struct tc_action *act)
 	return sz;
 }
 
+static int
+tcf_action_dump_terse(struct sk_buff *skb, struct tc_action *a, bool from_act)
+{
+	unsigned char *b = skb_tail_pointer(skb);
+	struct tc_cookie *cookie;
+
+	if (nla_put_string(skb, TCA_KIND, a->ops->kind))
+		goto nla_put_failure;
+	if (tcf_action_copy_stats(skb, a, 0))
+		goto nla_put_failure;
+	if (from_act && nla_put_u32(skb, TCA_ACT_INDEX, a->tcfa_index))
+		goto nla_put_failure;
+
+	rcu_read_lock();
+	cookie = rcu_dereference(a->act_cookie);
+	if (cookie && !from_act) {
+		if (nla_put(skb, TCA_ACT_COOKIE, cookie->len, cookie->data)) {
+			rcu_read_unlock();
+			goto nla_put_failure;
+		}
+	}
+	rcu_read_unlock();
+
+	return 0;
+
+nla_put_failure:
+	nlmsg_trim(skb, b);
+	return -1;
+}
+
 static int tcf_dump_walker(struct tcf_idrinfo *idrinfo, struct sk_buff *skb,
 			   struct netlink_callback *cb)
 {
@@ -248,7 +278,9 @@ static int tcf_dump_walker(struct tcf_idrinfo *idrinfo, struct sk_buff *skb,
 			index--;
 			goto nla_put_failure;
 		}
-		err = tcf_action_dump_1(skb, p, 0, 0);
+		err = (act_flags & TCA_FLAG_TERSE_DUMP) ?
+			tcf_action_dump_terse(skb, p, true) :
+			tcf_action_dump_1(skb, p, 0, 0);
 		if (err < 0) {
 			index--;
 			nlmsg_trim(skb, nest);
@@ -752,34 +784,6 @@ tcf_action_dump_old(struct sk_buff *skb, struct tc_action *a, int bind, int ref)
 	return a->ops->dump(skb, a, bind, ref);
 }
 
-static int
-tcf_action_dump_terse(struct sk_buff *skb, struct tc_action *a)
-{
-	unsigned char *b = skb_tail_pointer(skb);
-	struct tc_cookie *cookie;
-
-	if (nla_put_string(skb, TCA_KIND, a->ops->kind))
-		goto nla_put_failure;
-	if (tcf_action_copy_stats(skb, a, 0))
-		goto nla_put_failure;
-
-	rcu_read_lock();
-	cookie = rcu_dereference(a->act_cookie);
-	if (cookie) {
-		if (nla_put(skb, TCA_ACT_COOKIE, cookie->len, cookie->data)) {
-			rcu_read_unlock();
-			goto nla_put_failure;
-		}
-	}
-	rcu_read_unlock();
-
-	return 0;
-
-nla_put_failure:
-	nlmsg_trim(skb, b);
-	return -1;
-}
-
 int
 tcf_action_dump_1(struct sk_buff *skb, struct tc_action *a, int bind, int ref)
 {
@@ -787,7 +791,7 @@ tcf_action_dump_1(struct sk_buff *skb, struct tc_action *a, int bind, int ref)
 	unsigned char *b = skb_tail_pointer(skb);
 	struct nlattr *nest;
 
-	if (tcf_action_dump_terse(skb, a))
+	if (tcf_action_dump_terse(skb, a, false))
 		goto nla_put_failure;
 
 	if (a->hw_stats != TCA_ACT_HW_STATS_ANY &&
@@ -832,7 +836,7 @@ int tcf_action_dump(struct sk_buff *skb, struct tc_action *actions[],
 		nest = nla_nest_start_noflag(skb, i + 1);
 		if (nest == NULL)
 			goto nla_put_failure;
-		err = terse ? tcf_action_dump_terse(skb, a) :
+		err = terse ? tcf_action_dump_terse(skb, a, false) :
 			tcf_action_dump_1(skb, a, bind, ref);
 		if (err < 0)
 			goto errout;
@@ -1469,7 +1473,8 @@ static int tcf_action_add(struct net *net, struct nlattr *nla,
 }
 
 static const struct nla_policy tcaa_policy[TCA_ROOT_MAX + 1] = {
-	[TCA_ROOT_FLAGS] = NLA_POLICY_BITFIELD32(TCA_FLAG_LARGE_DUMP_ON),
+	[TCA_ROOT_FLAGS] = NLA_POLICY_BITFIELD32(TCA_FLAG_LARGE_DUMP_ON |
+						 TCA_FLAG_TERSE_DUMP),
 	[TCA_ROOT_TIME_DELTA]      = { .type = NLA_U32 },
 };
 
-- 
2.29.1

[PATCH iproute2-next] tc: implement support for action terse dump

[Vlad Buslov]

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(-)

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;
-- 
2.29.1

Re: [PATCH iproute2-next] tc: implement support for action terse dump

[David Ahern]

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.

Re: [PATCH iproute2-next] tc: implement support for action terse dump

[Vlad Buslov]

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.

Re: [PATCH iproute2-next] tc: implement support for action terse dump

[Jamal Hadi Salim]

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

Re: [PATCH iproute2-next] tc: implement support for action terse dump

[Vlad Buslov]

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

Re: [PATCH iproute2-next] tc: implement support for action terse dump

[David Ahern]

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.

Re: [PATCH iproute2-next] tc: implement support for action terse dump

[Jakub Kicinski]

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.

Re: [PATCH net-next] net: sched: implement action-specific terse dump

[Jamal Hadi Salim]

Thanks Vlad. Ive run the basic test and it looks good.
One thing i discovered while testing is that if the
cookie is set, we also want it in the dump. Your earlier
comment that it only costs if it was set is on point.

So please remove that check below:


 > +	if (cookie && !from_act) {
 > +		if (nla_put(skb, TCA_ACT_COOKIE, cookie->len, cookie->data)) {
 > +			rcu_read_unlock();
 > +			goto nla_put_failure;
 > +		}


cheers,
jamal

Re: [PATCH net-next] net: sched: implement action-specific terse dump

[Vlad Buslov]

On Mon 02 Nov 2020 at 14:26, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> Thanks Vlad. Ive run the basic test and it looks good.
> One thing i discovered while testing is that if the
> cookie is set, we also want it in the dump. Your earlier
> comment that it only costs if it was set is on point.
>
> So please remove that check below:

Okay. Will send V2 shortly.

>
>
>> +	if (cookie && !from_act) {
>> +		if (nla_put(skb, TCA_ACT_COOKIE, cookie->len, cookie->data)) {
>> +			rcu_read_unlock();
>> +			goto nla_put_failure;
>> +		}
>
>
> cheers,
> jamal