netdev - Re: [PATCH net-next 2/3] indirect call wrappers: add helpers for 3 and 4 ways switch

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: <10e134ce6b8c0e2060cecf57527cc52a99d4d6a5.camel@mellanox.com>
Date:   Mon, 3 Jun 2019 22:27:59 +0000
From:   Saeed Mahameed <saeedm@...lanox.com>
To:     "edumazet@...gle.com" <edumazet@...gle.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "pabeni@...hat.com" <pabeni@...hat.com>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "leon@...nel.org" <leon@...nel.org>
Subject: Re: [PATCH net-next 2/3] indirect call wrappers: add helpers for 3
 and 4 ways switch

On Mon, 2019-06-03 at 11:51 +0200, Paolo Abeni wrote:
> On Fri, 2019-05-31 at 18:30 +0000, Saeed Mahameed wrote:
> > On Fri, 2019-05-31 at 14:53 +0200, Paolo Abeni wrote:
> > > Experimental results[1] has shown that resorting to several
> > > branches
> > > and a direct-call is faster than indirect call via retpoline,
> > > even
> > > when the number of added branches go up 5.
> > > 
> > > This change adds two additional helpers, to cope with indirect
> > > calls
> > > with up to 4 available direct call option. We will use them
> > > in the next patch.
> > > 
> > > [1] 
> > > https://linuxplumbersconf.org/event/2/contributions/99/attachments/98/117/lpc18_paper_af_xdp_perf-v2.pdf
> > > 
> > > Signed-off-by: Paolo Abeni <pabeni@...hat.com>
> > > ---
> > >  include/linux/indirect_call_wrapper.h | 12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > > 
> > > diff --git a/include/linux/indirect_call_wrapper.h
> > > b/include/linux/indirect_call_wrapper.h
> > > index 00d7e8e919c6..7c4cac87eaf7 100644
> > > --- a/include/linux/indirect_call_wrapper.h
> > > +++ b/include/linux/indirect_call_wrapper.h
> > > @@ -23,6 +23,16 @@
> > >  		likely(f == f2) ? f2(__VA_ARGS__) :			
> > > \
> > >  				  INDIRECT_CALL_1(f, f1, __VA_ARGS__);	
> > > \
> > >  	})
> > > +#define INDIRECT_CALL_3(f, f3, f2, f1, ...)			
> > > 	
> > > \
> > > +	({								
> > > \
> > > +		likely(f == f3) ? f3(__VA_ARGS__) :			
> > > \
> > > +				  INDIRECT_CALL_2(f, f2, f1,
> > > __VA_ARGS__); \
> > > +	})
> > > +#define INDIRECT_CALL_4(f, f4, f3, f2, f1, ...)			
> > > 	\
> > > +	({								
> > > \
> > > +		likely(f == f4) ? f4(__VA_ARGS__) :		
> > 
> > do we really want "likely" here ? in our cases there is no
> > preference
> > on whuch fN is going to have the top priority, all of them are
> > equally
> > important and statically configured and guranteed to not change on
> > data
> > path .. 
> 
> I was a little undecided about that, too. 'likely()' is there mainly
> for simmetry with the already existing _1 and _2 variants. In such
> macros the branch prediction hint represent a real priority of the
> available choices.
> 

For macro _1 it make sense to have the likely keyword but for _2 it
doesn't, by looking at most of the usecases of INDIRECT_CALL_2, they
seem to be all around improving tcp/udp related indirection calls in
the protocol stack, and they seem to prefer tcp over udp. But IMHO at
least for the above usecase I think the likely keyword is being misused
here and should be remove from all INDIRECT_CALL_N where N > 1;

Eric, what do you think ?

> To avoid the branch prediction, a new set of macros should be
> defined,
> but that also sounds redundant.
> 
> If you have strong opinion against the breanch prediction hint, I
> could
> either drop this patch and the next one or resort to custom macros in
> the mlx code.
> 
> Any [alternative] suggestions more than welcome!
> 	\

custom macros can work, but in case you don't want to introduce such
macros in a vendor specific driver, then i think your patches are still
an improvement after all..

In any case, just make sure to use the order i suggested in next patch
with: MLX5_RX_INDIRECT_CALL_LIST

> > > +				  INDIRECT_CALL_3(f, f3, f2, f1,
> > > __VA_ARGS__); \
> > > +	})
> > >  
> > 
> > Oh the RETPOLINE!
> > 
> > On which (N) where INDIRECT_CALL_N(f, fN, fN-1, ..., f1,...) ,
> > calling
> > the indirection function pointer directly is going to be actually
> > better than this whole INDIRECT_CALL_N wrapper "if else" dance ?
> 
> In commit ce02ef06fcf7a399a6276adb83f37373d10cbbe1, it's measured a
> relevant gain even with more than 5 options. I personally would avoid
> adding much more options than the above.
> 
> Thanks,
> 
> Paolo
> 
>