Re: [Xen-devel] [PATCHv6 5/5] gnttab: use per-VCPU maptrack free lists
>>> On 22.04.15 at 18:00, wrote:
> static inline int
> get_maptrack_handle(
> struct grant_table *lgt)
> {
> +struct vcpu *v = current;
> int i;
> grant_handle_thandle;
> struct grant_mapping *new_mt;
> unsigned int new_mt_limit, nr_frames;
>
> -spin_lock(&lgt->maptrack_lock);
> -
> -while ( unlikely((handle = __get_maptrack_handle(lgt)) == -1) )
> -{
> -nr_frames = nr_maptrack_frames(lgt);
> -if ( nr_frames >= max_maptrack_frames )
> -break;
> -
> -new_mt = alloc_xenheap_page();
> -if ( !new_mt )
> -break;
> +handle = __get_maptrack_handle(lgt, v);
> +if ( likely(handle != -1) )
> +return handle;
>
> -clear_page(new_mt);
> +nr_frames = nr_vcpu_maptrack_frames(v);
> +if ( nr_frames >= max_maptrack_frames )
> +return -1;
>
> -new_mt_limit = lgt->maptrack_limit + MAPTRACK_PER_PAGE;
> +new_mt = alloc_xenheap_page();
> +if ( !new_mt )
> +return -1;
>
> -for ( i = 1; i < MAPTRACK_PER_PAGE; i++ )
> -new_mt[i - 1].ref = lgt->maptrack_limit + i;
> -new_mt[i - 1].ref = lgt->maptrack_head;
> -lgt->maptrack_head = lgt->maptrack_limit;
> +clear_page(new_mt);
>
> -lgt->maptrack[nr_frames] = new_mt;
> -smp_wmb();
> -lgt->maptrack_limit = new_mt_limit;
> +new_mt_limit = v->maptrack_limit + MAPTRACK_PER_PAGE;
>
> -gdprintk(XENLOG_INFO, "Increased maptrack size to %u frames\n",
> - nr_frames + 1);
> +for ( i = 1; i < MAPTRACK_PER_PAGE; i++ ){
> +new_mt[i - 1].ref = (lgt->maptrack_pages * MAPTRACK_PER_PAGE) + i;
> +new_mt[i - 1].vcpu = v->vcpu_id;
> +}
> +/* Set last entry vcpu and ref */
> +new_mt[i - 1].ref = v->maptrack_head;
> +new_mt[i - 1].vcpu = v->vcpu_id;
> +v->maptrack_head = lgt->maptrack_pages * MAPTRACK_PER_PAGE;
> +if (v->maptrack_tail == MAPTRACK_TAIL)
> +{
> +v->maptrack_tail = (lgt->maptrack_pages * MAPTRACK_PER_PAGE)
> ++ MAPTRACK_PER_PAGE - 1;
> +new_mt[i - 1].ref = MAPTRACK_TAIL;
> }
>
> +spin_lock(&lgt->maptrack_lock);
> +lgt->maptrack[lgt->maptrack_pages++] = new_mt;
> spin_unlock(&lgt->maptrack_lock);
The uses of ->maptrack_pages ahead of taking the lock can race
with updates inside the lock. And with locking elsewhere dropped
by the previous patch it looks like you can't update ->maptrack[]
like you do (you'd need a barrier between the pointer store and
the increment, and with that I think the lock would become
superfluous if it was needed only for this update).
Also note the coding style issues in the changes above^(and also
in code further down).
> -return handle;
> +v->maptrack_limit = new_mt_limit;
> +
> +return __get_maptrack_handle(lgt, v);
With the lock dropped, nothing guarantees this to succeed, which it
ought to unless the table size reached its allowed maximum.
> @@ -1422,6 +1448,17 @@ gnttab_setup_table(
> }
> gt->maptrack_pages = 0;
>
> +/* Tracking of mapped foreign frames table */
> +if ( (gt->maptrack = xzalloc_array(struct grant_mapping *,
> + max_maptrack_frames * d->max_vcpus))
> == NULL )
> +goto out2;
See the comments on the similar misplaced hunk in the previous
patch.
> --- a/xen/include/xen/grant_table.h
> +++ b/xen/include/xen/grant_table.h
> @@ -60,6 +60,8 @@ struct grant_mapping {
> u32 ref; /* grant ref */
> u16 flags; /* 0-4: GNTMAP_* ; 5-15: unused */
> domid_t domid; /* granting domain */
> +u32 vcpu; /* vcpu which created the grant mapping */
> +u16 pad[2];
> };
What is this pad[] good for?
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCHv6 5/5] gnttab: use per-VCPU maptrack free lists
On 23/04/15 17:11, Jan Beulich wrote:
On 22.04.15 at 18:00, wrote:
>>
>> +v->maptrack_head = lgt->maptrack_pages * MAPTRACK_PER_PAGE;
>> +if (v->maptrack_tail == MAPTRACK_TAIL)
>> +{
>> +v->maptrack_tail = (lgt->maptrack_pages * MAPTRACK_PER_PAGE)
>> ++ MAPTRACK_PER_PAGE - 1;
>> +new_mt[i - 1].ref = MAPTRACK_TAIL;
>> }
>>
>> +spin_lock(&lgt->maptrack_lock);
>> +lgt->maptrack[lgt->maptrack_pages++] = new_mt;
>> spin_unlock(&lgt->maptrack_lock);
>
> The uses of ->maptrack_pages ahead of taking the lock can race
> with updates inside the lock. And with locking elsewhere dropped
> by the previous patch it looks like you can't update ->maptrack[]
> like you do (you'd need a barrier between the pointer store and
> the increment, and with that I think the lock would become
> superfluous if it was needed only for this update).
This was my mistake. Malcolm's original patch had correct locking here.
> Also note the coding style issues in the changes above^(and also
> in code further down).
>
>> -return handle;
>> +v->maptrack_limit = new_mt_limit;
>> +
>> +return __get_maptrack_handle(lgt, v);
>
> With the lock dropped, nothing guarantees this to succeed, which it
> ought to unless the table size reached its allowed maximum.
We've just added a new page of handles for this VCPU. This will succeed.
David
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCHv6 5/5] gnttab: use per-VCPU maptrack free lists
>>> On 23.04.15 at 18:29, wrote: > On 23/04/15 17:11, Jan Beulich wrote: > On 22.04.15 at 18:00, wrote: >>> -return handle; >>> +v->maptrack_limit = new_mt_limit; >>> + >>> +return __get_maptrack_handle(lgt, v); >> >> With the lock dropped, nothing guarantees this to succeed, which it >> ought to unless the table size reached its allowed maximum. > > We've just added a new page of handles for this VCPU. This will succeed. Ah, right, this is per-vCPU now. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCHv6 5/5] gnttab: use per-VCPU maptrack free lists
On 23/04/15 17:11, Jan Beulich wrote:
On 22.04.15 at 18:00, wrote:
>> static inline int
>> get_maptrack_handle(
>> struct grant_table *lgt)
>> {
>> +struct vcpu *v = current;
>> int i;
>> grant_handle_thandle;
>> struct grant_mapping *new_mt;
>> unsigned int new_mt_limit, nr_frames;
>>
>> -spin_lock(&lgt->maptrack_lock);
>> -
>> -while ( unlikely((handle = __get_maptrack_handle(lgt)) == -1) )
>> -{
>> -nr_frames = nr_maptrack_frames(lgt);
>> -if ( nr_frames >= max_maptrack_frames )
>> -break;
>> -
>> -new_mt = alloc_xenheap_page();
>> -if ( !new_mt )
>> -break;
>> +handle = __get_maptrack_handle(lgt, v);
>> +if ( likely(handle != -1) )
>> +return handle;
>>
>> -clear_page(new_mt);
>> +nr_frames = nr_vcpu_maptrack_frames(v);
>> +if ( nr_frames >= max_maptrack_frames )
>> +return -1;
>>
>> -new_mt_limit = lgt->maptrack_limit + MAPTRACK_PER_PAGE;
>> +new_mt = alloc_xenheap_page();
>> +if ( !new_mt )
>> +return -1;
>>
>> -for ( i = 1; i < MAPTRACK_PER_PAGE; i++ )
>> -new_mt[i - 1].ref = lgt->maptrack_limit + i;
>> -new_mt[i - 1].ref = lgt->maptrack_head;
>> -lgt->maptrack_head = lgt->maptrack_limit;
>> +clear_page(new_mt);
>>
>> -lgt->maptrack[nr_frames] = new_mt;
>> -smp_wmb();
>> -lgt->maptrack_limit = new_mt_limit;
>> +new_mt_limit = v->maptrack_limit + MAPTRACK_PER_PAGE;
>>
>> -gdprintk(XENLOG_INFO, "Increased maptrack size to %u frames\n",
>> - nr_frames + 1);
>> +for ( i = 1; i < MAPTRACK_PER_PAGE; i++ ){
>> +new_mt[i - 1].ref = (lgt->maptrack_pages * MAPTRACK_PER_PAGE) + i;
>> +new_mt[i - 1].vcpu = v->vcpu_id;
>> +}
>> +/* Set last entry vcpu and ref */
>> +new_mt[i - 1].ref = v->maptrack_head;
>> +new_mt[i - 1].vcpu = v->vcpu_id;
>> +v->maptrack_head = lgt->maptrack_pages * MAPTRACK_PER_PAGE;
>> +if (v->maptrack_tail == MAPTRACK_TAIL)
>> +{
>> +v->maptrack_tail = (lgt->maptrack_pages * MAPTRACK_PER_PAGE)
>> ++ MAPTRACK_PER_PAGE - 1;
>> +new_mt[i - 1].ref = MAPTRACK_TAIL;
>> }
>>
>> +spin_lock(&lgt->maptrack_lock);
>> +lgt->maptrack[lgt->maptrack_pages++] = new_mt;
>> spin_unlock(&lgt->maptrack_lock);
>
> The uses of ->maptrack_pages ahead of taking the lock can race
> with updates inside the lock. And with locking elsewhere dropped
> by the previous patch it looks like you can't update ->maptrack[]
> like you do (you'd need a barrier between the pointer store and
> the increment, and with that I think the lock would become
> superfluous if it was needed only for this update).
>
> Also note the coding style issues in the changes above^(and also
> in code further down).
This was a last minute optimisation, this isn't on the hot patch so
we'll expand the spin_lock to cover all users of maptrack_pages.
Sorry about the coding style problems.
>
>> -return handle;
>> +v->maptrack_limit = new_mt_limit;
>> +
>> +return __get_maptrack_handle(lgt, v);
>
> With the lock dropped, nothing guarantees this to succeed, which it
> ought to unless the table size reached its allowed maximum.
>
>> @@ -1422,6 +1448,17 @@ gnttab_setup_table(
>> }
>> gt->maptrack_pages = 0;
>>
>> +/* Tracking of mapped foreign frames table */
>> +if ( (gt->maptrack = xzalloc_array(struct grant_mapping *,
>> + max_maptrack_frames * d->max_vcpus))
>> == NULL )
>> +goto out2;
>
> See the comments on the similar misplaced hunk in the previous
> patch.
>
>> --- a/xen/include/xen/grant_table.h
>> +++ b/xen/include/xen/grant_table.h
>> @@ -60,6 +60,8 @@ struct grant_mapping {
>> u32 ref; /* grant ref */
>> u16 flags; /* 0-4: GNTMAP_* ; 5-15: unused */
>> domid_t domid; /* granting domain */
>> +u32 vcpu; /* vcpu which created the grant mapping */
>> +u16 pad[2];
>> };
>
> What is this pad[] good for?
The pad is to keep the struct power of 2 sized because this allows the
compiler to optimise these macro's to right and left shifts:
#define MAPTRACK_PER_PAGE (PAGE_SIZE / sizeof(struct grant_mapping))
#define maptrack_entry(t, e) \
((t)->maptrack[(e)/MAPTRACK_PER_PAGE][(e)%MAPTRACK_PER_PAGE])
Malcolm
>
> Jan
>
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCHv6 5/5] gnttab: use per-VCPU maptrack free lists
>>> On 24.04.15 at 11:09, wrote:
> On 23/04/15 17:11, Jan Beulich wrote:
> On 22.04.15 at 18:00, wrote:
>>> --- a/xen/include/xen/grant_table.h
>>> +++ b/xen/include/xen/grant_table.h
>>> @@ -60,6 +60,8 @@ struct grant_mapping {
>>> u32 ref; /* grant ref */
>>> u16 flags; /* 0-4: GNTMAP_* ; 5-15: unused */
>>> domid_t domid; /* granting domain */
>>> +u32 vcpu; /* vcpu which created the grant mapping */
>>> +u16 pad[2];
>>> };
>>
>> What is this pad[] good for?
>
> The pad is to keep the struct power of 2 sized because this allows the
> compiler to optimise these macro's to right and left shifts:
>
> #define MAPTRACK_PER_PAGE (PAGE_SIZE / sizeof(struct grant_mapping))
> #define maptrack_entry(t, e) \
> ((t)->maptrack[(e)/MAPTRACK_PER_PAGE][(e)%MAPTRACK_PER_PAGE])
Okay, then why u16[2] instead of u32? And please add a brief
comment explaining the reason.
Apart from that I wonder whether fitting vcpu in the 10 unused
flags bits (not 11, as the comment on the field suggests) would be
an option. That would require limiting vCPU count to 4k, which I
don't think would really be a problem for anyone.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCHv6 5/5] gnttab: use per-VCPU maptrack free lists
On 24/04/15 10:50, Jan Beulich wrote:
On 24.04.15 at 11:09, wrote:
>> On 23/04/15 17:11, Jan Beulich wrote:
>> On 22.04.15 at 18:00, wrote:
--- a/xen/include/xen/grant_table.h
+++ b/xen/include/xen/grant_table.h
@@ -60,6 +60,8 @@ struct grant_mapping {
u32 ref; /* grant ref */
u16 flags; /* 0-4: GNTMAP_* ; 5-15: unused */
domid_t domid; /* granting domain */
+u32 vcpu; /* vcpu which created the grant mapping */
+u16 pad[2];
};
>>> What is this pad[] good for?
>> The pad is to keep the struct power of 2 sized because this allows the
>> compiler to optimise these macro's to right and left shifts:
>>
>> #define MAPTRACK_PER_PAGE (PAGE_SIZE / sizeof(struct grant_mapping))
>> #define maptrack_entry(t, e) \
>> ((t)->maptrack[(e)/MAPTRACK_PER_PAGE][(e)%MAPTRACK_PER_PAGE])
> Okay, then why u16[2] instead of u32? And please add a brief
> comment explaining the reason.
Most likely because vcpu was a u16 in the first draft, and got bumped
during internal review.
>
> Apart from that I wonder whether fitting vcpu in the 10 unused
> flags bits (not 11, as the comment on the field suggests) would be
> an option. That would require limiting vCPU count to 4k, which I
> don't think would really be a problem for anyone.
8k VCPU PV guests do function, and are very good at finding scalability
limits.
It would be nice not to break this.
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCHv6 5/5] gnttab: use per-VCPU maptrack free lists
On 24/04/15 10:50, Jan Beulich wrote:
On 24.04.15 at 11:09, wrote:
>> On 23/04/15 17:11, Jan Beulich wrote:
>> On 22.04.15 at 18:00, wrote:
--- a/xen/include/xen/grant_table.h
+++ b/xen/include/xen/grant_table.h
@@ -60,6 +60,8 @@ struct grant_mapping {
u32 ref; /* grant ref */
u16 flags; /* 0-4: GNTMAP_* ; 5-15: unused */
domid_t domid; /* granting domain */
+u32 vcpu; /* vcpu which created the grant mapping */
+u16 pad[2];
};
>>>
>>> What is this pad[] good for?
>>
>> The pad is to keep the struct power of 2 sized because this allows the
>> compiler to optimise these macro's to right and left shifts:
>>
>> #define MAPTRACK_PER_PAGE (PAGE_SIZE / sizeof(struct grant_mapping))
>> #define maptrack_entry(t, e) \
>> ((t)->maptrack[(e)/MAPTRACK_PER_PAGE][(e)%MAPTRACK_PER_PAGE])
>
> Okay, then why u16[2] instead of u32? And please add a brief
> comment explaining the reason.
>
> Apart from that I wonder whether fitting vcpu in the 10 unused
> flags bits (not 11, as the comment on the field suggests) would be
> an option. That would require limiting vCPU count to 4k, which I
> don't think would really be a problem for anyone.
I'd like to not have the overhead of bit operations on a hot path.
We're going from 512 grant entries per page to 256 grant entries per
page. This is taking the Xen memory overhead from 0.2% to 0.4% for grant
mapped pages.
Is the extra 0.2% memory overhead that concerning?
Malcolm
>
> Jan
>
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCHv6 5/5] gnttab: use per-VCPU maptrack free lists
>>> On 24.04.15 at 12:21, wrote:
> On 24/04/15 10:50, Jan Beulich wrote:
> On 24.04.15 at 11:09, wrote:
>>> On 23/04/15 17:11, Jan Beulich wrote:
>>> On 22.04.15 at 18:00, wrote:
> --- a/xen/include/xen/grant_table.h
> +++ b/xen/include/xen/grant_table.h
> @@ -60,6 +60,8 @@ struct grant_mapping {
> u32 ref; /* grant ref */
> u16 flags; /* 0-4: GNTMAP_* ; 5-15: unused */
> domid_t domid; /* granting domain */
> +u32 vcpu; /* vcpu which created the grant mapping */
> +u16 pad[2];
> };
What is this pad[] good for?
>>>
>>> The pad is to keep the struct power of 2 sized because this allows the
>>> compiler to optimise these macro's to right and left shifts:
>>>
>>> #define MAPTRACK_PER_PAGE (PAGE_SIZE / sizeof(struct grant_mapping))
>>> #define maptrack_entry(t, e) \
>>> ((t)->maptrack[(e)/MAPTRACK_PER_PAGE][(e)%MAPTRACK_PER_PAGE])
>>
>> Okay, then why u16[2] instead of u32? And please add a brief
>> comment explaining the reason.
>>
>> Apart from that I wonder whether fitting vcpu in the 10 unused
>> flags bits (not 11, as the comment on the field suggests) would be
>> an option. That would require limiting vCPU count to 4k, which I
>> don't think would really be a problem for anyone.
>
> I'd like to not have the overhead of bit operations on a hot path.
>
> We're going from 512 grant entries per page to 256 grant entries per
> page. This is taking the Xen memory overhead from 0.2% to 0.4% for grant
> mapped pages.
>
> Is the extra 0.2% memory overhead that concerning?
Perhaps not so much the memory overhead but the doubled
cache bandwidth needed. But anyway, this was only a
question, and with both you and Andrew being opposed let's
just stay with what you submitted.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
