Re: [PATCH v2 02/11] xen/gnttab: Rework resource acquisition

From: Andrew Cooper <amc96@cam.ac.uk>
To: paul@xen.org, 'Xen-devel' <xen-devel@lists.xenproject.org>
Cc: "'George Dunlap'" <George.Dunlap@eu.citrix.com>,
	"'Ian Jackson'" <iwj@xenproject.org>,
	"'Jan Beulich'" <JBeulich@suse.com>,
	"'Stefano Stabellini'" <sstabellini@kernel.org>,
	"'Wei Liu'" <wl@xen.org>, "'Julien Grall'" <julien@xen.org>,
	"'Michał Leszczyński'" <michal.leszczynski@cert.pl>,
	"'Hubert Jasudowicz'" <hubert.jasudowicz@cert.pl>,
	"'Tamas K Lengyel'" <tamas@tklengyel.com>,
	"Andrew Cooper" <amc96@cam.ac.uk>
Subject: Re: [PATCH v2 02/11] xen/gnttab: Rework resource acquisition
Date: Mon, 11 Jan 2021 21:22:39 +0000	[thread overview]
Message-ID: <835893e1-fcd0-efa1-86ce-02af70dd1101@cam.ac.uk> (raw)
In-Reply-To: <001601d69258$4d6a7c90$e83f75b0$@xen.org>

On 24/09/2020 10:51, Paul Durrant wrote:
>> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
>> index a5d3ed8bda..912f07be47 100644
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -4013,6 +4013,81 @@ static int gnttab_get_shared_frame_mfn(struct domain *d,
>>      return 0;
>>  }
>>
>> +int gnttab_acquire_resource(
>> +    struct domain *d, unsigned int id, unsigned long frame,
>> +    unsigned int nr_frames, xen_pfn_t mfn_list[])
>> +{
>> +    struct grant_table *gt = d->grant_table;
>> +    unsigned int i = nr_frames, tot_frames;
>> +    mfn_t tmp;
>> +    void **vaddrs = NULL;
>> +    int rc;
>> +
>> +    /* Input sanity. */
> Nit: inconsistency with full stops on single line comments.

The whole point of relaxing the style was because feedback over minutia
such as this was deemed detrimental to the community.

If I ever see feedback like this, I will commit commit the patch there
and then.  This is the only way upstream Xen is going to turn into a
less toxic environment for contributors.

>> +            rc = -EINVAL;
>> +            break;
>> +        }
>> +        rc = gnttab_get_status_frame_mfn(d, tot_frames - 1, &tmp);
>> +        break;
>> +    }
>> +
> I think you could drop the write lock here...
>
>> +    /* Any errors from growing the table? */
>> +    if ( rc )
>> +        goto out;
>> +
> ...and acquire it read here, since we know the table cannot shrink. You'd need to re-check the gt_version for safety though.

And you've correctly identified why I didn't.  If we had a
relax-write-to-read lock operation, that would also be fine, but we don't.

Fundamentally, this is an operation made once during VM construction, to
map one single frame.  It is not a hotpath in need of microptimising its
locking pattern, and absolutely not something worth introducing a safety
hazard for.

~Andrew