On Sun, Aug 28, 2016 at 10:04:21PM -0700, Cong Wang wrote:
On Fri, Aug 26, 2016 at 12:16 PM, Eric Dumazet [off-list ref] wrote:
quoted
On Fri, 2016-08-26 at 11:26 -0700, Cong Wang wrote:
quoted
1) Currently there are only a few actions using lockless, and they are
questionable, as we already discussed before, there could be some
race condition when you modify an existing action.
There is no fundamental issue with a race condition.
For mirred action, maybe. As we already discussed, the more
complex an action is, the harder to make it lockless in your
way (that is, not using RCU)
quoted
Sure, there are races, but they have no serious effect.
Feel free to send a fix if you really have time to spare.
It's because the code is written by you?
I am surprised how you try to hide your own problem in
such a way...
quoted
quoted
2) We need to change the tc action API in order to fully support RCU,
which is what I have been working on these days. I should come up
with something next Monday (if not this weekend).
So for this patchset, using spinlock is fine, just as many other actions.
I will take care of it later.
This is _not_ fine.
OK, so where are your patches to make the rest actions
lockless?
quoted
We are in 2016, not in 1995 anymore.
Fair enough, sounds like all actions are already lockless in
fast path now in 2016, you know this is not true...
quoted
We are not adding a spinlock in a hot path unless absolutely needed.
If it is bug-free, yes, I am totally with you. I care about corretness
more than any performance.
quoted
With multi queue NIC, this spinlock is going to hurt performance so much
that this action wont be used by any serious user.
We have used mirred action even before you make it lockless.
quoted
Here, it is absolutely trivial to use RCU and/or percpu counters.
Sounds like we don't need any API change, why not go ahead
and try it? Please do teach me how to modify an existing
action in a lockless way without changing any API (and of course
needs to be bug-free), I am very happy to learn your "trivial" way
to fix this, since I don't have any trivial fix.
Please, stop bullsh*t, show me your trivial code.
Regarding the specific action in this patchset, correct me if I'm wrong,
but I think that the lock could be removed safely.
When the action is modified during traffic, an existing tcf_enc_metadata
is not changed, but a new metadata is allocated and the pointer is
replaced to point to the new one.
I just need to make sure that when changing an action from 'release'
into 'set' - tcf_enc_metadata will be set before the action type is
changed - change the order of operations and add a memory barrier.
Here is a pseudo code to explain:
metadata_new = new allocated metadata
metadata_old = t->tcft_enc_metadata
t->tcft_action = encapdecap
/* make sure the compiler won't swap the setting of tcft_action with
* tcft_enc_metadata
*/
wmb()
t->tcft_enc_metadata = metadata_new
release metadata_old
This way, no need for lock between the init() and act() operations.
Please let me know if you see a problem with this approach.
I will also change the stats to be percpu.
Thanks,
Amir