Re: [PATCH v7 1/2] xen/pci: introduce PF<->VF links

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 11/15/24 02:55, Jan Beulich wrote:
> On 14.11.2024 19:50, Stewart Hildebrand wrote:
>> On 11/14/24 05:34, Jan Beulich wrote:
>>> On 12.11.2024 21:53, Stewart Hildebrand wrote:
>>>> +            pdev->pf_pdev = pf_pdev;
>>>> +            list_for_each_entry(vf_pdev, &pf_pdev->vf_list, vf_list)
>>>> +            {
>>>> +                if ( vf_pdev == pdev )
>>>> +                {
>>>> +                    already_added = true;
>>>> +                    break;
>>>> +                }
>>>> +            }
>>>> +            if ( !already_added )
>>>> +                list_add(&pdev->vf_list, &pf_pdev->vf_list);
>>>> +        }
>>>>      }
>>>
>>> Personally, as I have a dislike for excess variables, I'd have gotten away
>>> without "already_added". Instead of setting it to true, vf_pdev could be
>>> set to NULL. Others may, however, consider this "obfuscation" or alike.
>>
>> This relies on vf_pdev being set to non-NULL when the list is empty and
>> after the last iteration if the list doesn't contain the element. I had
>> to look up the definitions of list_for_each_entry, INIT_LIST_HEAD, and
>> list_{add,del,entry} to verify that vf_pdev would be non-NULL in those
>> cases.
>>
>> Perhaps a better approach would be to introduce list_add_unique() in
>> Xen's list library? Then we can also get rid of the vf_pdev variable.
>>
>> static inline bool list_contains(struct list_head *entry,
>>                                  struct list_head *head)
>> {
>>    struct list_head *ptr;
>>
>>    list_for_each(ptr, head)
>>    {
>>        if ( ptr == entry )
>>            return true;
>>    }
>>
>>    return false;
>> }
>>
>> static inline void list_add_unique(struct list_head *new,
>>                                    struct list_head *head)
>> {
>>     if ( !list_contains(new, head) )
>>         list_add(new, head);
>> }
> 
> I'm uncertain of this kind of an addition. For long lists one would need to
> be careful with whether to actually use list_contains(). It being a simple
> library function would make this easy to overlook.

It occurs to me I could simply check if pdev->pf_pdev has been initialized:

            if ( !pdev->pf_pdev )
                list_add(&pdev->vf_list, &pf_pdev->vf_list);
            pdev->pf_pdev = pf_pdev;