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

[Paolo Abeni <pabeni@redhat.com>]

On Mon, 2019-06-03 at 22:27 +0000, Saeed Mahameed wrote:
> 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@redhat.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;

I experimented a bit with gcc 8.3.1 and some BP hint variations:

* with current macros we have single test for fN and an incresing
number of conditional jumps and tests for the following functions, as
the generated code looks somehow alike:

	cmp f4, function_ptr
	jne test_f3
	call f4
post_call:
	// ...

	// ...
test_f3:
	cmp f3, function_ptr
	jne test_f2
	call f3
	jmp post_call

test_f2:
	cmp f2, function_ptr
	//...

* keeping 'likely' only on INDIRECT_CALL_1 we get a conditinal jump for
fN and the number of conditional jumps and tests grows for the next
functions, as the generated code looks somehow alike:

	cmp f4, function_ptr
	je call_f4
	cmp f3, function_ptr
	je call_f3
	//...
	cmp f1, function_ptr
	jne indirect_call
	call f1
post_call:
	// ...

	// ...
call_f4:
	call f4
	jmp post_call
call_f3:
	call f3
	jmp post_call
	// ...


* without any BP hints, is quite alike the above, except for the last
test, the indirect call don't need an additional jump: 

	cmp f4, function_ptr
	je call_f4
	cmp f3, function_ptr
	je call_f3
	//...
	cmp f1, function_ptr
	je call_f1
	call retpoline_helper

I think the first option should be overall better then the 2nd. The 3rd
one is the worse.

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

Sure! will do in next iteration, as soon as the above topic is settled.

Thanks!

Paolo