Re: [PATCHv8 1/3] gnttab: Introduce rwlock to protect updates to grant table state

[David Vrabel <david.vrabel@citrix.com>]

On 19/05/15 09:02, Jan Beulich wrote:
> 
> Which then of course raises the question - is taking the read lock
> here and in several other places really sufficient? The thing that the
> original spin lock appears to protect here is not only the grant table
> structure itself, but also the active entry. I.e. relaxing to a read
> lock would seem possible only after having put per-active-entry
> locking in place.

Yes, this series was split the wrong way round.  I could:

a) squash the first two patches back together;
b) refactor the first two patches into
   - introduce active entry locks
   - introduce maptrack lock
   - convert grant table lock to rw lock.

Which approach is preferred (obviously (a) is a lot less work)?

>> @@ -64,6 +64,11 @@ struct grant_mapping {
>>  
>>  /* Per-domain grant information. */
>>  struct grant_table {
>> +    /* 
>> +     * Lock protecting updates to grant table state (version, active
>> +     * entry list, etc.)
>> +     */
>> +    rwlock_t              lock;
>>      /* Table size. Number of frames shared with guest */
>>      unsigned int          nr_grant_frames;
>>      /* Shared grant table (see include/public/grant_table.h). */
>> @@ -82,8 +87,8 @@ struct grant_table {
>>      struct grant_mapping **maptrack;
>>      unsigned int          maptrack_head;
>>      unsigned int          maptrack_limit;
>> -    /* Lock protecting updates to active and shared grant tables. */
>> -    spinlock_t            lock;
>> +    /* Lock protecting the maptrack page list, head, and limit */
>> +    spinlock_t            maptrack_lock;
>>      /* The defined versions are 1 and 2.  Set to 0 if we don't know
>>         what version to use yet. */
>>      unsigned              gt_version;
> 
> So in order for fields protected by one of the locks to be as likely
> as possible on the same cache line as the lock itself, I think
> gt_version should also be moved up in the structure. We may
> even want/need to add some separator between basic and
> maptrack data to ensure they end up on different cache lines.

I think this sort of structure field shuffling should be a follow on patch.

David