Re: [PATCH net-next v4 3/8] net_sched: mirred: remove action when the target device is gone

[Jamal Hadi Salim <jhs@mojatatu.com>]

[-- Attachment #1: Type: text/plain, Size: 1773 bytes --]



I am sorry Cong - I will still object to this change. I dont want
to even bother testing it.
You are making some serious policy decisions in the kernel.
Such policy decisions should be made by user space not the kernel.
Whoever made the idiotic decision of removing the device should
modify or delete the flow rule - at minimal they
may deserve some warning. Deleting the action is wrong. It is simple
graph theory.
Slightly related is this - look at the attached test (I went out of
my way to annotate it) and imagine you have mirred where the policers
are ....

cheers,
jamal

On 12/18/13 16:42, Cong Wang wrote:
> On Wed, Dec 18, 2013 at 11:50 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>> On 12/18/13 13:36, Cong Wang wrote:
>>>
>>> On Wed, Dec 18, 2013 at 6:31 AM, Jamal Hadi Salim <jhs@mojatatu.com>
>>> wrote:
>>>>
>>>> On 12/15/13 23:15, Cong Wang wrote:
>>>>>
>>>>>
>>>>> When the target device is removed, the mirred action is
>>>>> still there but with the dev pointer setting to NULL.
>>>>> This makes the output from 'tc filter' ugly. There is no
>>>>> reason to keep it.
>>>>>
>>>>
>>>> Sorry - this one i have problems with.
>>>> actions may be referenced from multiple filters,
>>>> you cant just delete it (that would leave other users
>>>> pointing to it dangling). Destroying would eventually
>>>> delete it when the refcount goes to 0.
>>>
>>>
>>> How? tcf_action_init() always allocates a new action,
>>> it doesn't even look for an existing one.
>>>
>>
>>
>> tc action blah index 123
>> tc action filter goo action blah index 123
>> tc action filter gah action blah index 123
>>
>> Very useful for example for multiple flows to
>> share the same policer.
>>
>
> Ah, I see. I will create a test case for this
> and rework on the mirred patch.
>


[-- Attachment #2: shared-act-tst1 --]
[-- Type: text/plain, Size: 1346 bytes --]

TC="/sbin/tc"
#
sudo ifconfig dummy0 up
sudo ifconfig dummy1 up
DEV1="eth0"
DEV2="eth1"
#
#
#
sudo $TC qdisc del dev $DEV1 ingress
sudo $TC qdisc add dev $DEV1 ingress
sudo $TC qdisc del dev $DEV2 ingress
sudo $TC qdisc add dev $DEV2 ingress
#Note - older tc did not pipe for skbedit (add "pipe" if this is rejected)
sudo $TC filter add dev $DEV1 parent ffff: protocol ip pref 10 \
u32 match ip protocol 1 0xff flowid 1:10 \
action skbedit mark 11 \
action police rate 10kbit burst 10k pipe index 1 \
action skbedit mark 12 \
action police rate 20kbit burst 20k pipe index 2 \
action action mirred egress mirror dev dummy0
#note sharing of the two policers by two flows
#across multiple devices
sudo $TC filter add dev $DEV2 parent ffff: protocol ip pref 10 \
u32 match ip protocol 1 0xff flowid 1:10 \
action skbedit mark 21 \
action police rate 10kbit burst 10k pipe index 1 \
action skbedit mark 22 \
action police rate 20kbit burst 20k pipe index 2 \
action action mirred egress mirror dev dummy1
#
#
# now do a ping -f to somewhere, for more exciting results
# do a -s 2048 or so
# sudo ping -f 192.168.1.100 -c 10000
# sudo ping -f 127.0.0.1 -c 10000
#
sudo tc -s filter ls dev $DEV1 parent ffff: protocol ip 
sudo tc -s filter ls dev $DEV2 parent ffff: protocol ip 
sudo tc -s actions ls action police
sudo tc -s actions ls action skbedit