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