Re: [RFC PATCH v2 net-next 0/3] flow_offload: Re-add per-action statistics

[Vlad Buslov <vladbu@mellanox.com>]

On Tue 21 May 2019 at 16:23, Vlad Buslov <vladbu@mellanox.com> wrote:
> On Tue 21 May 2019 at 15:46, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>> On 2019-05-20 5:12 p.m., Jamal Hadi Salim wrote:
>>> On 2019-05-20 2:36 p.m., Edward Cree wrote:
>>>> On 20/05/2019 17:29, Jamal Hadi Salim wrote:
>>
>>> Ok, so the "get" does it. Will try to reproduce when i get some
>>> cycles. Meantime CCing Cong and Vlad.
>>>
>>
>>
>> I have reproduced it in a simpler setup. See attached. Vlad this is
>> likely from your changes. Sorry no cycles to dig more.
>
> Jamal, thanks for minimizing the reproduction. I'll look into it.
>
>> Lucas, can we add this to the testcases?
>>
>>
>> cheers,
>> jamal
>>
>> sudo tc qdisc del dev lo ingress
>> sudo tc qdisc add dev lo ingress
>>
>> sudo tc filter add dev lo parent ffff: protocol ip prio 8 u32 \
>> match ip dst 127.0.0.8/32 flowid 1:10 \
>> action vlan push id 100 protocol 802.1q \
>> action drop index 104
>>
>> sudo tc filter add dev lo parent ffff: protocol ip prio 8 u32 \
>> match ip dst 127.0.0.10/32 flowid 1:10 \
>> action vlan push id 101 protocol 802.1q \
>> action drop index 104
>>
>> #
>> sudo tc -s filter ls dev lo parent ffff: protocol ip
>>
>> #this will now delete action gact index 104(drop) from display
>> sudo tc -s actions get action drop index 104
>>
>> sudo tc -s filter ls dev lo parent ffff: protocol ip
>>
>> #But you can still see it if you do this:
>> sudo tc -s actions get action drop index 104

It seems that culprit in this case is tc_action->order field. It is used
as nla attrtype when dumping actions. Initially it is set according to
ordering of actions of filter that creates them. However, it is
overwritten in tca_action_gd() each time action is dumped through action
API (according to action position in tb array) and when new filter is
attached to shared action (according to action order on the filter).
With this we have another way to reproduce the bug:

sudo tc qdisc add dev lo ingress

#shared action is the first action (order 1)
sudo tc filter add dev lo parent ffff: protocol ip prio 8 u32 \
match ip dst 127.0.0.8/32 flowid 1:10 \
action drop index 104 \
action vlan push id 100 protocol 802.1q

#shared action is the the second action (order 2)
sudo tc filter add dev lo parent ffff: protocol ip prio 8 u32 \
match ip dst 127.0.0.10/32 flowid 1:10 \
action vlan push id 101 protocol 802.1q \
action drop index 104

# Now action is only visible on one filter
sudo tc -s filter ls dev lo parent ffff: protocol ip

The usage of tc_action->order is inherently incorrect for shared actions
and I don't really understand the reason for using it in first place.
I'm sending RFC patch that fixes the issue by just using action position
in tb array for attrtype instead of order field, and it seems to solve
both issues for me. Please check it out to verify that I'm not breaking
something. Also, please advise on "fixes" tag since this problem doesn't
seem to be directly caused by my act API refactoring.