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

Josh Poimboeuf
2023-03-22T18:50:48Z

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