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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1472559628.14381.283.camel@edumazet-glaptop3.roam.corp.google.com>
Date:   Tue, 30 Aug 2016 05:20:28 -0700
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Jamal Hadi Salim <jhs@...atatu.com>
Cc:     davem@...emloft.net, netdev@...r.kernel.org, daniel@...earbox.net,
        xiyou.wangcong@...il.com
Subject: Re: [PATCH v3 net-next 1/1] net_sched: Introduce skbmod action

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.