Re: [PATCH v3 net-next 1/1] net_sched: Introduce skbmod action

[Eric Dumazet <eric.dumazet@gmail.com>]

On Tue, 2016-08-30 at 07:12 -0400, Jamal Hadi Salim wrote:
> On 16-08-29 02:20 PM, Eric Dumazet wrote:
> 
> >
> > You would need to store everything in an object, managed by rcu.
> >
> > struct my_rcu_safe_struct {
> >      u32 flags;
> >      u8   eth_dst[ETH_ALEN];
> >      u8   eth_src[ETH_ALEN];
> >      __be16 eth_type;
> > };
> >
> > And then allocate a new one when you need to update the infos (from
> > tcf_skbmod_init(())
> >
> > RCU : Read Copy Update.
> >
> > Then in the reader you would use
> >
> > rcu_read_lock();
> > myptr = rcu_dereference(d->ptr);
> > if (myptr) {
> >         if (myptr->flags & SKBMOD_F_DMAC)
> >                  ether_addr_copy(eth_hdr(skb)->h_dest, myptr->eth_dst);
> >          if (myptr->flags & SKBMOD_F_SMAC)
> >                  ether_addr_copy(eth_hdr(skb)->h_source, myptr->eth_src);
> >          if (myptr->flags & SKBMOD_F_ETYPE)
> >                  eth_hdr(skb)->h_proto = myptr->eth_type;
> > }
> > rcu_read_unlock();
> 
> Thanks Eric.
> This is the approach i thought Cong was going to take but in a way that
> it applies to all actions.
> It requires I do this extra allocation per update/create - I am not sure 
> how much of a big deal that is (we take pride in our update rate).
> Let me work and post something simple that captures these ideas
> then wait to see what Cong has in mind for the general approach.


The percpu stats infra is already there, you have to change
tcf_hash_create() last parameter to 'true'.

For example you have to add in struct my_rcu_safe_struct a 'struct
rcu_head rcu;'  field to be able to use kfree_rcu() instead of
synchronize_rcu()

Some very simple actions do not even need rcu_dereference()

RCU have many different variants.

Sometimes READ_ONCE() and WRITE_ONCE() are enough, when all the 'flags'
can be in a single integer, and this avoids an extra dereference and
possible cache miss.