Re: [PATCH 3/4] xen/virtual-region: Link the list build time

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.03.2024 12:04, Andrew Cooper wrote:
> Given 3 statically initialised objects, its easy to link the list at build
> time.  There's no need to do it during runtime at boot (and with IRQs-off,
> even).

Hmm, technically that's correct, but isn't the overall result more fragile,
in being more error prone if going forward someone found a need to alter
things? Kind of supporting that view is also ...

> ---
>  xen/common/virtual_region.c | 45 ++++++++++++++++++++++++-------------
>  1 file changed, 30 insertions(+), 15 deletions(-)

... the diffstat of the change. It's perhaps also for a reason that ...

> --- a/xen/common/virtual_region.c
> +++ b/xen/common/virtual_region.c
> @@ -15,8 +15,19 @@ extern const struct bug_frame
>      __start_bug_frames_2[], __stop_bug_frames_2[],
>      __start_bug_frames_3[], __stop_bug_frames_3[];
>  
> +/*
> + * For the built-in regions, the double linked list can be constructed at
> + * build time.  Forward-declare the elements.
> + */
> +static struct list_head virtual_region_list;
> +static struct virtual_region core, core_init;
> +
>  static struct virtual_region core = {
> -    .list = LIST_HEAD_INIT(core.list),
> +    .list = {
> +        .next = &core_init.list,
> +        .prev = &virtual_region_list,
> +    },
> +
>      .text_start = _stext,
>      .text_end = _etext,
>      .rodata_start = _srodata,
> @@ -32,7 +43,11 @@ static struct virtual_region core = {
>  
>  /* Becomes irrelevant when __init sections are cleared. */
>  static struct virtual_region core_init __initdata = {
> -    .list = LIST_HEAD_INIT(core_init.list),
> +    .list = {
> +        .next = &virtual_region_list,
> +        .prev = &core.list,
> +    },
> +
>      .text_start = _sinittext,
>      .text_end = _einittext,
>  
> @@ -50,7 +65,10 @@ static struct virtual_region core_init __initdata = {
>   *
>   * All readers of virtual_region_list MUST use list_for_each_entry_rcu.
>   */
> -static LIST_HEAD(virtual_region_list);
> +static struct list_head virtual_region_list = {
> +    .next = &core.list,
> +    .prev = &core_init.list,
> +};

... there's no pre-cooked construct to avoid any open-coding at least
here.

To clarify up front: I'm willing to be convinced otherwise, and I therefore
might subsequently provide an ack. I'm also specifically not meaning this
to be treated as "pending objection"; if another maintainer provides an ack,
that's okay(ish) with me.

Jan