Re: [Patch net] sched, cls: check if we could overwrite actions when changing a filter

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

On 04/18/14 13:18, Cong Wang wrote:

> In this case, all the statements inside if (icmp) {} are actions, right?
> Sorry, I still fail to see why not allowing to change them *together*?
>

Yes, thats what i am saying as well. Maybe we were agreeing all along.

> IOW, what's wrong with changing if (icmp) { A } to if (icmp) { B } ?
> where A and B could be any complex combination of actions.
> RTNL lock guarantees this is transactional.
>

RTNL is one dimension. The other is the datapath processing.
You need to make sure that packets still flow correctly during the
change over.
My suggestion was you add the new rule first with a lower priority
so that it is never used as long as the current one is in place.
You then do a delete of the old one. RCU grace period passes
where current packets are processed then the new rule takes effect.
You dont have to follow that suggestion as long as you achieve the
goal.

> Users are responsible to ensure the logic of A or B is correct, not
> the kernel. Kernel should allow even a wrong combination,
> since there is no way to check the correctness in kernel.
>

I'd be happy with that.

> I never mean to only add or remove one of them inside, although
> my specific case is just for appending, my patch should allow to
> overwrite all the actions together.
>

Well - then go nuts and put out a patch.
Replace _all or none_ is a reasonable approach.


> It is not a corner case, it is a very basic functionality we need:
>
> We mirror icmp packets to every vethX device, when one of them
> is gone, we just remove the action; when a new one comes up,
> we append an action. So simple...
>

This is where my problem was Cong - you have a simple use case and i was
hoping that you dont base that for a generic solution. If you only
have one action, then no problem in deleting/adding it. But if you
have a group of actions then you delete/add the whole group.

cheers,
jamal