[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v5 1/8] mm: Place unscrubbed pages at the end of pagelist
On 07/31/2017 10:45 AM, Jan Beulich wrote: >>>> Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> 07/23/17 4:01 AM >>> >> On 06/27/2017 01:06 PM, Jan Beulich wrote: >>>>>> Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> 06/22/17 8:55 PM >>> >>>> + { >>>> + if ( pg < first_dirty_pg ) >>>> + first_dirty = (first_dirty_pg - pg) / sizeof(*pg); >>> Pointer subtraction already includes the involved division. >> >> Yes, this was a mistake. >> >>> Otoh I wonder >>> if you couldn't get away without pointer comparison/subtraction here >>> altogether. >> >> Without comparison I can only assume that first_dirty is zero (i.e. the >> whole buddy is potentially dirty). Is there something else I could do? > I was thinking of tracking indexes instead of pointers. But maybe that > would more hamper readability of the overall result than help it. I'll try to see how it looks. > >>>> + else >>>> + i = 0; >>>> + >>>> + for ( ; i < (1 << cur_order); i++ ) >>>> + if ( test_bit(_PGC_need_scrub, >>>> + &cur_head[i].count_info) ) >>>> + { >>>> + first_dirty = i; >>>> + break; >>>> + } >>> Perhaps worth having ASSERT(first_dirty != INVALID_DIRTY_IDX) here? Or are >>> there cases where ->u.free.first_dirty of a page may be wrong? >> >> When we merge in free_heap_pages we don't clear first_dirty of the >> successor buddy (at some point I did have this done but you questioned >> whether it was needed and I dropped it). > Hmm, this indeed answers my question, but doesn't help (me) understanding > whether the suggested ASSERT() could be wrong. Oh, I see what you were asking --- ASSERT() *after* the loop, to make sure we indeed found the first dirty page. Yes, I will add it. > >>>> --- a/xen/include/asm-x86/mm.h >>>> +++ b/xen/include/asm-x86/mm.h >>>> @@ -88,7 +88,15 @@ struct page_info >>>> /* Page is on a free list: ((count_info & PGC_count_mask) == 0). >>>> */ >>>> struct { >>>> /* Do TLBs need flushing for safety before next page use? */ >>>> - bool_t need_tlbflush; >>>> + unsigned long need_tlbflush:1; >>>> + >>>> + /* >>>> + * Index of the first *possibly* unscrubbed page in the buddy. >>>> + * One more than maximum possible order (MAX_ORDER+1) to >>> Why +1 here and hence ... >> Don't we have MAX_ORDER+1 orders? > So here there might be a simple misunderstanding: I understand the > parenthesized MAX_ORDER+1 to represent "maximum possible > order", i.e. excluding the "one more than", not the least because of > the ... > >>> + * accommodate INVALID_DIRTY_IDX. >>> + */ >>> +#define INVALID_DIRTY_IDX (-1UL & (((1UL<<MAX_ORDER) + 2) - 1)) >>> + unsigned long first_dirty:MAX_ORDER + 2; > +2 here. > >>> ... why +2 instead of +1? And isn't the expression INVALID_DIRTY_IDX wrongly >>> parenthesized (apart from lacking blanks around the shift operator)? I'd >>> expect you want a value with MAX_ORDER+1 set bits, i.e. >>> (1UL << (MAX_ORDER + 1)) - 1. ANDing with -1UL seems quite pointless too. >> Yes to parentheses and AND. Should be (1UL << (MAX_ORDER + 2)) - 1 > I.e. I would still expect it to be (1UL << (MAX_ORDER + 1)) - 1 > here. Sorry, I still don't get it. Say, MAX_ORDER is 1. Since this implies that indexes 0, 1, 2 and 3 are all valid (because we can have up to 2^(MAX_ORDER+1) pages), don't we need 3 bits to indicate an invalid index? -boris _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |