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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.05.15 at 15:20, <david.vrabel@xxxxxxxxxx> wrote:
> 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)?

Unless it results in a very hard to review patch, I guess I'd be fine
with (a). Another alternative I would see is to have the current
first patch use write_lock() in most places, and convert them to
read_lock() as the active entry locks get introduced (which I'd
expect to still be less work than (b)).

>>> @@ -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.

Sure, if you so prefer (with "lock" being moved I personally
wouldn't mind gt_version to also move).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel