[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Julien Grall <julien@xxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Mon, 6 Sep 2021 15:58:42 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version; bh=NbkbrBJpOYpfUoH/UPnckwv83IdFu2ZkE2MCOi9Y3VM=; b=LcfZQPEIjveA2wV8SuJFoXoXOTqnZsbfYKCcp8qmggsRYlFe+QntvsRYM83YnWImj+kgqcVWfBL/Z58FEzzce5ym1fpXneVxzQS4xQtwOb5oNAARzr8Wwj6USSs/SbvJ3kKRmE4ALiynacV6TXkBkdqvEX1KEf3UCCt+DYMYKT0tnpEtr5gPTuRog+Q8edCs5c23IM+mRT54/6PfInnmVt/Vur4xBu3H0xy/8b/n5HEpTR7kI9ruGm7cO+DJuGUIYhc3VzoVOwVUKtlz0L249s2B5lWE1y+iIGRh6xDvUrFgWD9OUkivCGor69eVANoAyCKJENV2Kbhg7zzNTKLiXA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=W/98m7EE/vaNsWFlRjnTFXQoozQBocGC/8V4/k1orNkAwjibJQRUp1lDNnNb2sM52VEyzBFGnzyx3QmloZCpQ2FFL9p0YfyB3R/Lr/bAJ8SaS3AiUYagria4SHQDKFcH47deVEs9B+8djyjpj7pFxFDkDx64jOYz7vOQiuuprJozH/Q25MwlqCRGfy3rQziH+eEtNuapug0S92GLuSZYEL8M+llktfVwleylnaNUcIyclb3kV13Ep43hHrdsqnTSAklUl5bKCJEl/fjcnal9kdH38HZNQqyZesAXKBSIAX02esRoDIVqrCMKQeFeEWJZ30PyAlRaQ5Y4W2r6H7XaPg==
  • Authentication-results: lists.xenproject.org; dkim=none (message not signed) header.d=none;lists.xenproject.org; dmarc=none action=none header.from=suse.com;
  • Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Ian Jackson <iwj@xxxxxxxxxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Mon, 06 Sep 2021 13:58:54 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.