Re: [PATCH v4 01/11] xen: Implement xen/alternative-call.h for use in common code

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 06/09/2021 16:52, Jan Beulich wrote:
> On 03.09.2021 21:06, Daniel P. Smith wrote:
>> --- /dev/null
>> +++ b/xen/include/xen/alternative-call.h
>> @@ -0,0 +1,63 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef XEN_ALTERNATIVE_CALL
>> +#define XEN_ALTERNATIVE_CALL
>> +
>> +/*
>> + * Some subsystems in Xen may have multiple implementions, which can be
>> + * resolved to a single implementation at boot time.  By default, this will
>> + * result in the use of function pointers.
>> + *
>> + * Some architectures may have mechanisms for dynamically modifying .text.
>> + * Using this mechnaism, function pointers can be converted to direct calls
>> + * which are typically more efficient at runtime.
>> + *
>> + * For architectures to support:
>> + *
>> + * - Implement alternative_{,v}call() in asm/alternative.h.  Code generation
>> + *   requirements are to emit a function pointer call at build time, and 
>> stash
>> + *   enough metadata to simplify the call at boot once the implementation 
>> has
>> + *   been resolved.
>> + * - Select ALTERNATIVE_CALL in Kconfig.
>> + *
>> + * To use:
>> + *
>> + * Consider the following simplified example.
>> + *
>> + *  1) struct foo_ops __alt_call_maybe_initdata ops;
>> + *
>> + *  2) const struct foo_ops __initconst foo_a_ops = { ... };
>> + *     const struct foo_ops __initconst foo_b_ops = { ... };
>> + *
>> + *     void foo_init(void)
>> + *     {
>> + *         ...
>> + *         if ( use_impl_a )
>> + *             ops = *foo_a_ops;
>> + *         else if ( use_impl_b )
>> + *             ops = *foo_b_ops;
>> + *         ...
>> + *     }
>> + *
>> + *  3) alternative_call(ops.bar, ...);
>> + *
>> + * There needs to a single ops object (1) which will eventually contain the
>> + * function pointers.  This should be populated in foo's init() function (2)
>> + * by one of the available implementations.  To call functions, use
>> + * alternative_{,v}call() referencing the main ops object (3).
>> + */
>> +
>> +#ifdef CONFIG_ALTERNATIVE_CALL
>> +
>> +#include <asm/alternative.h>
>> +
>> +#define __alt_call_maybe_initdata __initdata
> My v3 comment here was:
>
> "I think it wants (needs) clarifying that this may only be used if
>  the ops object is used exclusively in alternative_{,v}call()
>  instances (besides the original assignments to it, of course)."
>
> I realize this was slightly too strict, as other uses from .init.*
> are of course also okay, but I continue to think that - in
> particular with the example using it - there should be a warning
> about this possible pitfall. Or am I merely unable to spot the
> wording change somewhere in the comment?

Such a comment is utterly pointless.  initdata has a well known meaning,
and a comment warning about the effects of it is just teaching
developers to suck eggs[1]

~Andrew

[1] For the non-english speakers,
https://en.wikipedia.org/wiki/Teaching_grandmother_to_suck_eggs