Re: [PATCH v2 11/11] static_call: Remove DEFINE_STATIC_CALL_RET0()

[Josh Poimboeuf]

On Wed, Mar 22, 2023 at 03:04:31PM +0000, Christophe Leroy wrote:
> Le 22/03/2023 à 05:00, Josh Poimboeuf a écrit :
> > NULL and RET0 static calls are both slightly different ways of nopping a
> > static call.  A not-insignificant amount of code and complexity is spent
> > maintaining them separately.  It's also somewhat tricky for the user who
> > has to try to remember to use the correct one for the given function
> > type.
> > 
> > Simplify things all around by just combining them, such that NULL static
> > calls always return 0.
> > 
> > While it doesn't necessarily make sense for void-return functions to
> > return 0, it's pretty much harmless.  The return value register is
> > already callee-clobbered, and an extra "xor %eax, %eax" shouldn't affect
> > performance (knock on wood).
> 
> In the case of powerpc, which implements out-of-line static calls for 
> now, it is more than just an extra instruction. It requires a jump to 
> the couple instructions that clear ret reg and rets. For the 8xx it also 
> means cache miss as the cache lines are 16 bytes. So what was just one 
> cycle return instruction becomes a 3 cycles + 1 cache miss. It is not a 
> show-stopper for that change, but I think it was worth mentioning.

Good point.  I should mention that (if we keep the patch).

> > This "do nothing return 0" default should work for the vast majority of
> > NULL cases.  Otherwise it can be easily overridden with a user-specified
> > function which panics or returns 0xdeadbeef or does whatever one wants.
> > 
> > This simplifies the static call code and also tends to help simplify
> > users' code as well.
> 
> I'd have expected DEFINE_STATIC_CALL_RET0() to remain, to make it clear 
> that it returns 0. As you explained, it doesn't matter what NULL 
> returns, but returning 0 is vital four RET0 cases. So I would have 
> dropped DEFINE_STATIC_CALL_NULL() and retained DEFINE_STATIC_CALL_RET0().

The issue is static_call_update().  It takes NULL as an input, which
comes in handy for many static call users.  So it makes sense to have
NULL mean "nop (and return 0 if needed)".

IMO it becomes more confusing to have two interfaces (NULL and RET0)
meaning the same thing.

-- 
Josh