Re: [PATCH 1/9] gnttab: defer allocation of maptrack frames table

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.09.2021 15:33, Julien Grall wrote:
> On 06/09/2021 14:29, Jan Beulich wrote:
>> On 06.09.2021 15:13, Julien Grall wrote:
>>> On 26/08/2021 11:09, Jan Beulich wrote:
>>>> --- a/xen/common/grant_table.c
>>>> +++ b/xen/common/grant_table.c
>>>> @@ -633,6 +633,34 @@ get_maptrack_handle(
>>>>        if ( likely(handle != INVALID_MAPTRACK_HANDLE) )
>>>>            return handle;
>>>>    
>>>> +    if ( unlikely(!read_atomic(&lgt->maptrack)) )
>>>> +    {
>>>> +        struct grant_mapping **maptrack = NULL;
>>>> +
>>>> +        if ( lgt->max_maptrack_frames )
>>>> +            maptrack = vzalloc(lgt->max_maptrack_frames * 
>>>> sizeof(*maptrack));
>>>
>>> While I understand that allocating with a lock is bad idea, I don't like
>>> the fact that multiple vCPUs racing each other would result to
>>> temporarily allocate more memory.
>>>
>>> If moving the call within the lock is not a solution, would using a loop
>>> with a cmpxchg() a solution to block the other vCPU?
>>
>> As with any such loop the question then is for how long to retry. No matter
>> what (static) loop bound you choose, if you exceed it you would return an
>> error to the caller for no reason.
> 
> I was thinking to have an unbound loop. This would be no better no worth 
> than the current implementation because of the existing lock.

Not quite: Ticket locks grant access to the locked region in FIFO manner.
Such an open-coded loop wouldn't, i.e. there would be a risk of a loop
becoming (close to) infinite. Granted this is largely a theoretical risk,
but still ...

>> As to the value to store by cmpxchg() - were you thinking of some "alloc in
>> progress" marker?
> 
> Yes.
> 
>> You obviously can't store the result of the allocation
>> before actually doing the allocation, yet it is unnecessary allocations
>> that you want to avoid (i.e. to achieve your goal the allocation would need
>> to come after the cmpxchg()). A marker would further complicate the other
>> code here, even if (maybe) just slightly ...
> 
> Right, the code will be a bit more complicated (although it would not be 
> that bad if moved in a separate function...) but I feel it is better 
> than the multiple vzalloc().

It's the other way around here - to me it feels better the way I've coded
it. I don't think the risk of an actual race is overly high, the more that
this is a one time event for every domain.

Jan