Re: [PATCH net-next V3 4/4] net/sched: Introduce act_tunnel_key

[Eric Dumazet]

On Tue, 2016-08-30 at 16:17 +0300, Amir Vadai wrote:
> On Tue, Aug 30, 2016 at 08:05:03AM -0400, Jamal Hadi Salim wrote:
> > 
> > old = rtnl_dereference(mykey->p);
> > if (ovr)
> > spin_lock_bh(>tcf_lock);
> Thanks for the detailed example :)
> 
> what are we protecting with this spin lock here? isn't concurrent init()
> calls are protected by the rtnl lock?

Right. RTNL should be enough here for the write exclusion.

Re: [PATCH net-next V3 4/4] net/sched: Introduce act_tunnel_key

[Amir Vadai]

On Tue, Aug 30, 2016 at 08:05:03AM -0400, Jamal Hadi Salim wrote:
> On 16-08-30 07:03 AM, Amir Vadai wrote:
> > On Sun, Aug 28, 2016 at 10:04:21PM -0700, Cong Wang wrote:
> > > On Fri, Aug 26, 2016 at 12:16 PM, Eric Dumazet  
> > > wrote:
> > > > On Fri, 2016-08-26 at 11:26 -0700, Cong Wang wrote:
> 
> 
> > Regarding the specific action in this patchset, correct me if I'm wrong,
> > but I think that the lock could be removed safely.
> > 
> 
> From what Eric suggested (refer to my posting on skbmod),
> this becomes:
> 
> +struct tcf_tunnel_key_p {
> + int tcft_action;
> + struct metadata_dst *tcft_enc_metadata;
> +};
> 
> /* rcu protected */
> +struct tcf_tunnel_key {
> + struct tc_actioncommon;
> +   struct tcf_tunnel_key_p *p;
> +};
> 
> At init() - always alloc struct tcf_tunnel_key_p, new
> 
> old = rtnl_dereference(mykey->p);
> if (ovr)
> spin_lock_bh(>tcf_lock);
Thanks for the detailed example :)

what are we protecting with this spin lock here? isn't concurrent init()
calls are protected by the rtnl lock?


> ... update all params here ..
> rcu_assign_pointer(mykey->p, new);
> if (ovr) {
>  spin_unlock_bh(>tcf_lock);
>  synchronize_rcu();
> }
> 
> kfree(old);
> 
> at act():
> 
> rcu_read_lock();
> struct tcf_tunnel_key_p *p = rcu_dereference(mykey->p);
> ... use p here ...
> rcu_read_unlock();
> 
> Cong was looking to do something more generic for all actions.
> 
> cheers,
> jamal

Re: [PATCH net-next V3 4/4] net/sched: Introduce act_tunnel_key

[Eric Dumazet]

On Tue, 2016-08-30 at 14:39 +0300, Amir Vadai wrote:
> On Tue, Aug 30, 2016 at 02:03:08PM +0300, Amir Vadai wrote:

> > Regarding the specific action in this patchset, correct me if I'm wrong,
> > but I think that the lock could be removed safely.

Sure ;)

> > 
> > 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
> > 
> 
> Oh - I had a typo here:
> Need to set the metadata and only after that, set the action:
> 
> t->tcft_enc_metadata = metadata_new
> wmb()

rcu_assign_pointer() is your friend, it auto documents the thing.


Note that you probably need to store in the allocated object :

dst,
tcf_action (a copy of it, read in tunnel_key_act()
tcft_action ( a copy of it, read in tunnel_key_act())
rcu_head rcu for kfree_rcu()

> t->tcft_action = encapdecap
> 
> > 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.

Right, check tcf_hash_create() last argument. (false -> true)

Thanks.

Re: [PATCH net-next V3 4/4] net/sched: Introduce act_tunnel_key

[Jamal Hadi Salim]

On 16-08-30 07:03 AM, Amir Vadai wrote:

On Sun, Aug 28, 2016 at 10:04:21PM -0700, Cong Wang wrote:

On Fri, Aug 26, 2016 at 12:16 PM, Eric Dumazet  wrote:

On Fri, 2016-08-26 at 11:26 -0700, Cong Wang wrote:




Regarding the specific action in this patchset, correct me if I'm wrong,
but I think that the lock could be removed safely.



From what Eric suggested (refer to my posting on skbmod),
this becomes:

+struct tcf_tunnel_key_p {
+   int tcft_action;
+   struct metadata_dst *tcft_enc_metadata;
+};

/* rcu protected */
+struct tcf_tunnel_key {
+   struct tc_actioncommon;
+   struct tcf_tunnel_key_p *p;
+};

At init() - always alloc struct tcf_tunnel_key_p, new

old = rtnl_dereference(mykey->p);
if (ovr)
spin_lock_bh(>tcf_lock);
... update all params here ..
rcu_assign_pointer(mykey->p, new);
if (ovr) {
 spin_unlock_bh(>tcf_lock);
 synchronize_rcu();
}

kfree(old);

at act():

rcu_read_lock();
struct tcf_tunnel_key_p *p = rcu_dereference(mykey->p);
... use p here ...
rcu_read_unlock();

Cong was looking to do something more generic for all actions.

cheers,
jamal

Re: [PATCH net-next V3 4/4] net/sched: Introduce act_tunnel_key

[Amir Vadai]

On Tue, Aug 30, 2016 at 02:03:08PM +0300, Amir Vadai wrote:
> On Sun, Aug 28, 2016 at 10:04:21PM -0700, Cong Wang wrote:
> > On Fri, Aug 26, 2016 at 12:16 PM, Eric Dumazet  
> > wrote:
> > > On Fri, 2016-08-26 at 11:26 -0700, Cong Wang wrote:
> > >> 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)
> > 
> > >
> > > 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...
> > 
> > 
> > >
> > >>
> > >> 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?
> > 
> > 
> > >
> > > 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...
> > 
> > 
> > > 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.
> > 
> > 
> > >
> > > 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.
> > 
> > 
> > >
> > > 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
> 

Oh - I had a typo here:
Need to set the metadata and only after that, set the action:

t->tcft_enc_metadata = metadata_new
wmb()
t->tcft_action = encapdecap

> 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
> 

Re: [PATCH net-next V3 4/4] net/sched: Introduce act_tunnel_key

[Amir Vadai]

On Sun, Aug 28, 2016 at 10:04:21PM -0700, Cong Wang wrote:
> On Fri, Aug 26, 2016 at 12:16 PM, Eric Dumazet  wrote:
> > On Fri, 2016-08-26 at 11:26 -0700, Cong Wang wrote:
> >> 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)
> 
> >
> > 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...
> 
> 
> >
> >>
> >> 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?
> 
> 
> >
> > 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...
> 
> 
> > 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.
> 
> 
> >
> > 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.
> 
> 
> >
> > 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

Re: [PATCH net-next V3 4/4] net/sched: Introduce act_tunnel_key

[Cong Wang]

On Fri, Aug 26, 2016 at 12:16 PM, Eric Dumazet  wrote:
> On Fri, 2016-08-26 at 11:26 -0700, Cong Wang wrote:
>> 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)

>
> 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...


>
>>
>> 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?


>
> 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...


> 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.


>
> 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.


>
> 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.

Re: [PATCH net-next V3 4/4] net/sched: Introduce act_tunnel_key

[Eric Dumazet]

On Fri, 2016-08-26 at 11:26 -0700, Cong Wang wrote:
> On Thu, Aug 25, 2016 at 10:48 AM, Eric Dumazet  wrote:
> >
> > Please find a better way than using a spinlock in this hot path.
> >
> > Maybe looking at
> > 2ee22a90c7afac265bb6f7abea610b938195e2b8 net_sched: act_mirred: remove 
> > spinlock in fast path
> > 56e5d1ca183d8616fab377d7d466c244b4dbb3b9 net_sched: act_gact: remove 
> > spinlock in fast path
> 
> This is not necessary at the moment, because:
> 
> 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.

Sure, there are races, but they have no serious effect.

Feel free to send a fix if you really have time to spare.

> 
> 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.

We are in 2016, not in 1995 anymore.

We are not adding a spinlock in a hot path unless absolutely needed.

With multi queue NIC, this spinlock is going to hurt performance so much
that this action wont be used by any serious user.

Here, it is absolutely trivial to use RCU and/or percpu counters.

Re: [PATCH net-next V3 4/4] net/sched: Introduce act_tunnel_key

[Cong Wang]

On Thu, Aug 25, 2016 at 10:48 AM, Eric Dumazet  wrote:
>
> Please find a better way than using a spinlock in this hot path.
>
> Maybe looking at
> 2ee22a90c7afac265bb6f7abea610b938195e2b8 net_sched: act_mirred: remove 
> spinlock in fast path
> 56e5d1ca183d8616fab377d7d466c244b4dbb3b9 net_sched: act_gact: remove spinlock 
> in fast path

This is not necessary at the moment, because:

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.

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.

Re: [PATCH net-next V3 4/4] net/sched: Introduce act_tunnel_key

[Jiri Benc]

On Thu, 25 Aug 2016 19:13:47 +0300, Hadar Hen Zion wrote:
> +static int tunnel_key_act(struct sk_buff *skb, const struct tc_action *a,
> +   struct tcf_result *res)
> +{
> + struct tcf_tunnel_key *t = to_tunnel_key(a);
> + int action;
> +
> + spin_lock(>tcf_lock);
> + tcf_lastuse_update(>tcf_tm);
> + bstats_update(>tcf_bstats, skb);
> + action = t->tcf_action;
> +
> + switch (t->tcft_action) {
> + case TCA_TUNNEL_KEY_ACT_RELEASE:
> + skb_dst_set_noref(skb, NULL);
> + break;

You're leaking dst here.

> + case TCA_TUNNEL_KEY_ACT_SET:
> + skb_dst_set_noref(skb, >tcft_enc_metadata->dst);
> + break;

And here, too. Also, what protects the tcft_enc_metadata->dst from
being freed if there's a skb queued and the action is removed? Seems
that tunnel_key_release just happily frees it. You probably need to
take a reference here.

> + if (tb[TCA_TUNNEL_KEY_ENC_IPV4_SRC] &&
> + tb[TCA_TUNNEL_KEY_ENC_IPV4_DST]) {
> + __be32 saddr;
> + __be32 daddr;
> +
> + saddr = nla_get_be32(tb[TCA_TUNNEL_KEY_ENC_IPV4_SRC]);
> + daddr = nla_get_be32(tb[TCA_TUNNEL_KEY_ENC_IPV4_DST]);

Use nla_get_in_addr, please.

> + if (family == AF_INET) {
> + __be32 saddr = info->key.u.ipv4.src;
> + __be32 daddr = info->key.u.ipv4.dst;
> +
> + if (!nla_put_be32(skb, TCA_TUNNEL_KEY_ENC_IPV4_SRC, saddr) &&
> + !nla_put_be32(skb, TCA_TUNNEL_KEY_ENC_IPV4_DST, daddr))

nla_put_in_addr

Thanks!

 Jiri

Re: [PATCH net-next V3 4/4] net/sched: Introduce act_tunnel_key

[Or Gerlitz]

On Thu, Aug 25, 2016 at 8:48 PM, Eric Dumazet  wrote:
> On Thu, 2016-08-25 at 19:13 +0300, Hadar Hen Zion wrote:
>> From: Amir Vadai 

>> +static int tunnel_key_act(struct sk_buff *skb, const struct tc_action *a,
>> +   struct tcf_result *res)
>> +{
>> + struct tcf_tunnel_key *t = to_tunnel_key(a);
>> + int action;
>> +
>> + spin_lock(>tcf_lock);
>> + tcf_lastuse_update(>tcf_tm);
>> + bstats_update(>tcf_bstats, skb);
>> + action = t->tcf_action;
>> +
>> + switch (t->tcft_action) {
>> + case TCA_TUNNEL_KEY_ACT_RELEASE:
>> + skb_dst_set_noref(skb, NULL);
>> + break;
>> + case TCA_TUNNEL_KEY_ACT_SET:
>> + skb_dst_set_noref(skb, >tcft_enc_metadata->dst);
>> + break;
>> + default:
>> + WARN_ONCE(1, "Bad tunnel_key action.\n");
>> + break;
>> + }
>> +
>> + spin_unlock(>tcf_lock);
>> + return action;

> Please find a better way than using a spinlock in this hot path.
> Maybe looking at
> 2ee22a90c7afac265bb6f7abea610b938195e2b8 net_sched: act_mirred: remove 
> spinlock in fast path
[...]

okay, thanks for the heads up, will look there

Re: [PATCH net-next V3 4/4] net/sched: Introduce act_tunnel_key

[Eric Dumazet]

On Thu, 2016-08-25 at 19:13 +0300, Hadar Hen Zion wrote:
> From: Amir Vadai 
> 
> This action could be used before redirecting packets to a shared tunnel
> device, or when redirecting packets arriving from a such a device.
> 
> The action will release the metadata created by the tunnel device
> (decap), or set the metadata with the specified values for encap
> operation.
> 
> For example, the following flower filter will forward all ICMP packets
> destined to 11.11.11.2 through the shared vxlan device 'vxlan0'. Before
> redirecting, a metadata for the vxlan tunnel is created using the
> tunnel_key action and it's arguments:



> +
> +static int tunnel_key_act(struct sk_buff *skb, const struct tc_action *a,
> +   struct tcf_result *res)
> +{
> + struct tcf_tunnel_key *t = to_tunnel_key(a);
> + int action;
> +
> + spin_lock(>tcf_lock);
> + tcf_lastuse_update(>tcf_tm);
> + bstats_update(>tcf_bstats, skb);
> + action = t->tcf_action;
> +
> + switch (t->tcft_action) {
> + case TCA_TUNNEL_KEY_ACT_RELEASE:
> + skb_dst_set_noref(skb, NULL);
> + break;
> + case TCA_TUNNEL_KEY_ACT_SET:
> + skb_dst_set_noref(skb, >tcft_enc_metadata->dst);
> + break;
> + default:
> + WARN_ONCE(1, "Bad tunnel_key action.\n");
> + break;
> + }
> +
> + spin_unlock(>tcf_lock);
> + return action;


Please find a better way than using a spinlock in this hot path.

Maybe looking at 
2ee22a90c7afac265bb6f7abea610b938195e2b8 net_sched: act_mirred: remove spinlock 
in fast path
56e5d1ca183d8616fab377d7d466c244b4dbb3b9 net_sched: act_gact: remove spinlock 
in fast path
8f2ae965b7ef4f4ddab6110f06388e270723d694 net_sched: act_gact: read tcfg_ptype 
once
cc6510a9504fd3c03d76bd68d99653148342eecc net_sched: act_gact: use a separate 
packet counters for gact_determ()
cef5ecf96b28dc91c4e9f398a336c578fb9e1a0c net_sched: act_gact: make tcfg_pval 
non zero
519c818e8fb646eef1e8bfedd18519bec47bc9a9 net: sched: add percpu stats to actions

Re: [PATCH net-next V3 4/4] net/sched: Introduce act_tunnel_key

[Shmulik Ladkani]

Hi,

On Thu, 25 Aug 2016 19:13:47 +0300 Hadar Hen Zion  wrote:
> +static int tunnel_key_act(struct sk_buff *skb, const struct tc_action *a,
> +   struct tcf_result *res)
> +{
> + struct tcf_tunnel_key *t = to_tunnel_key(a);
> + int action;
> +
> + spin_lock(>tcf_lock);
> + tcf_lastuse_update(>tcf_tm);
> + bstats_update(>tcf_bstats, skb);
> + action = t->tcf_action;
> +
> + switch (t->tcft_action) {
> + case TCA_TUNNEL_KEY_ACT_RELEASE:
> + skb_dst_set_noref(skb, NULL);
> + break;
> + case TCA_TUNNEL_KEY_ACT_SET:
> + skb_dst_set_noref(skb, >tcft_enc_metadata->dst);
> + break;

Two additional questions:
 - No need to perform a 'skb_dst_drop(skb)' prior the dst set calls?
 - Why there's no need to take a reference on tcft_enc_metadata->dst?

Besides that,

Reviewed-by: Shmulik Ladkani 

Thanks!