Re: [PATCH net-next 00/13] Control action percpu counters allocation by netlink flag

[Vlad Buslov <vladbu@mellanox.com>]

On Thu 24 Oct 2019 at 19:12, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> On 2019-10-24 11:23 a.m., Vlad Buslov wrote:
>> On Thu 24 Oct 2019 at 10:35, Jiri Pirko <jiri@resnulli.us> wrote:
>>> Wed, Oct 23, 2019 at 04:21:51PM CEST, jhs@mojatatu.com wrote:
>>>> On 2019-10-23 9:04 a.m., Vlad Buslov wrote:
>>>>>
>
> [..]
>>>> Jiri complains constantly about all these new per-action TLVs
>>>> which are generic. He promised to "fix it all" someday. Jiri i notice
>>>> your ack here, what happened? ;->
>>>
>>> Correct, it would be great. However not sure how exactly to do that now.
>>> Do you have some ideas.
>>>
>>>
>>> But basically this patchset does what was done many many times in the
>>> past. I think it was a mistake in the original design not to have some
>>> "common attrs" :/ Lesson learned for next interfaces.
>>
>
> Jiri, we have a high level TLV space which can be applied to all
> actions. See discussion below with Vlad. At least for this specific
> change we can get away from repeating that mistake.
>
>> Jamal, can we reach some conclusion? Do you still suggest to refactor
>> the patches to use TCA_ROOT_FLAGS and parse it in act API instead of
>> individual actions?
>>
>
> IMO this would certainly help us walk away from having
> every action replicate the same attribute with common semantics.
> refactoring ->init() to take an extra attribute may look ugly but
> is cleaner from a uapi pov. Actions that dont implement the feature
> can ignore the extra parameter(s). It doesnt have to be TCA_ROOT_FLAGS
> but certainly that high level TLV space is the right place to put it.
> WDYT?
>
> cheers,
> jamal

Hi Jamal,

I've been trying to re-design this change to use high level TLV, but I
don't understand how to make it backward compatible. If I just put new
attribute before nested action attributes, it will fail to parse when
older client send netlink packet without it and I don't see
straightforward way to distinguish between the new attribute and
following nested action attribute (which might accidentally have same
length and fall into allowed attribute range). I don't have much
experience working with netlink, so I might be missing something obvious
here. However, extending existing action attributes (which are already
conveniently parsed in tcf_action_init_1() function) with new optional
'flags' value seems like straightforward and backward compatible
solution.

Rough outline in code:

2 files changed, 6 insertions(+), 2 deletions(-)
include/uapi/linux/pkt_cls.h | 1 +
net/sched/act_api.c          | 7 +++++--

modified   include/uapi/linux/pkt_cls.h
@@ -16,6 +16,7 @@ enum {
 	TCA_ACT_STATS,
 	TCA_ACT_PAD,
 	TCA_ACT_COOKIE,
+	TCA_ACT_FLAGS,
 	__TCA_ACT_MAX
 };
 
modified   net/sched/act_api.c
@@ -852,6 +852,7 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
 	char act_name[IFNAMSIZ];
 	struct nlattr *tb[TCA_ACT_MAX + 1];
 	struct nlattr *kind;
+	u32 flags = 0;
 	int err;
 
 	if (name == NULL) {
@@ -877,6 +878,8 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
 				goto err_out;
 			}
 		}
+		if (tb[TCA_ACT_FLAGS])
+			flags = nla_get_bitfield32(tb[TCA_ACT_FLAGS]).value;
 	} else {
 		if (strlcpy(act_name, name, IFNAMSIZ) >= IFNAMSIZ) {
 			NL_SET_ERR_MSG(extack, "TC action name too long");
@@ -915,10 +918,10 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
 	/* backward compatibility for policer */
 	if (name == NULL)
 		err = a_o->init(net, tb[TCA_ACT_OPTIONS], est, &a, ovr, bind,
-				rtnl_held, tp, extack);
+				rtnl_held, tp, flags, extack);
 	else
 		err = a_o->init(net, nla, est, &a, ovr, bind, rtnl_held,
-				tp, extack);
+				tp, flags, extack);
 	if (err < 0)
 		goto err_mod;


WDYT?

Regards,
Vlad