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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.03.2024 14:54, Andrew Cooper wrote:
> On 18/03/2024 1:25 pm, Jan Beulich wrote:
>> 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.
> 
> I think it's a very small price to pay in order to allow patch 4 to exist.
> 
> If you can think of a nice way to express this with a pre-cooked
> construct then suggestions welcome, but it's a really complicated piece
> of metaprogramming to express in a nice way.

I don't see any suitable pre-cooked construct, but something custom just for
this file might be to have

/*
 * 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;
#define ENTRY1() { .next = &core_init.list, .prev = &virtual_region_list }
#define ENTRY2() { .next = &virtual_region_list, .prev = &core.list }
#define ENTRY3() { .next = &core.list, .prev = &core_init.list }

such that they're all close together and hence the list arrangement can be
easily seen. Sure, that'll still require each of the macros to be used
exactly once. Maybe instead of numeric suffixes the name of the struct the
macro is to be used in might help:

#define ENTRY_HEAD() { .next = &core.list, .prev = &core_init.list }
#define ENTRY_CORE() { .next = &core_init.list, .prev = &virtual_region_list }
#define ENTRY_INIT() { .next = &virtual_region_list, .prev = &core.list }

This way entries also come in list order.

Jan