Re: [PATCH net-next v2 1/3] net/sched: act_vlan: Fix modify to allow 0

On Wed, May 26, 2021 at 02:45:53PM +0300, Boris Sukholitko wrote:

Hi Davide,

On Tue, May 25, 2021 at 10:35:50PM +0200, Davide Caratti wrote:

quoted

On Tue, May 25, 2021 at 06:35:59PM +0300, Boris Sukholitko wrote:

hello Boris,

[...]

quoted

quoted

@@ -189,7 +190,8 @@ static int tcf_vlan_init(struct net *net, struct nlattr *nla,
 			push_proto = htons(ETH_P_8021Q);
 		}
 
-		if (tb[TCA_VLAN_PUSH_VLAN_PRIORITY])
+		push_prio_exists = !!tb[TCA_VLAN_PUSH_VLAN_PRIORITY];

when the VLAN tag is pushed, not modified, the value of 'push_prio' is
used in the traffic path regardless of the true/false value of
'push_prio_exists'. See below:

 50         case TCA_VLAN_ACT_PUSH:
 51                 err = skb_vlan_push(skb, p->tcfv_push_proto, p->tcfv_push_vid |
 52                                     (p->tcfv_push_prio << VLAN_PRIO_SHIFT));

Yes, the tcfv_push_prio is 0 here by default.

quoted

So, I think that 'p->push_prio_exists' should be identically set to
true when 'v_action' is TCA_VLAN_ACT_PUSH. That would allow a better
display of rules once patch 2 of your series is applied: otherwise,
2 rules configuring the same TCA_VLAN_ACT_PUSH rule would be displayed
differently, depending on whether userspace provided or not the
TCA_VLAN_PUSH_VLAN_PRIORITY attribute set to 0.

Sorry for being obtuse, but I was under impression that we want to
display priority if and only if it has been set by the userspace.

don't get me wrong, I don't have strong opinions on this (I don't have
strong opinions at all :) ). In my understanding, the patch was adding
'push_prio_exists' to allow using 0 in 'p->tcfv_push_prio' in the
traffic path, instead of assuming that '0' implies no configuration by
the user.

My suggestion was just to simplify the end-user dump experience, so
that the value of 'p->tcfv_push_prio' is dumped always in case of
TCA_VLAN_ACT_PUSH. In this way, rules with equal "behavior" in the
traffic path are always dumped in the same way. IOW,

# tc action add action vlan push id 42 prio 0 index 1

and

# tc action add action vlan push id 42 index 1

do exactly the same thing in the traffic path, so there is no need to
dump them differently. On the contrary, these 2 rules:

# tc action add action vlan modify id 42 prio 0 index 1

and

# tc action add action vlan modify id 42 index 1

don't do the same thing, because packet hitting the first rule will have
their priority identically set to 0, while the second one will leave the
VLAN priority unmodified. So, I think it makes sense to have different
dumps here (that was my comment to your v1).

Another small nit - forgive me, I didn't spot it in the first review:

not 100% sure, but I think that in tcf_vlan_get_fill_size() we need
to avoid accounting for TCA_VLAN_PUSH_VLAN_PRIORITY in case the rule
has 'push_prio_exists' equal to false. Otherwise we allocate an
extra u8 netlink attribute in case of batch dump. Correct?

thanks!
-- 
davide