[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH] xen/mm: page_alloc fix duplicated order shift operation in the loop
On 23.04.2022 21:35, Paran Lee wrote: > It doesn't seem necessary to do that > duplicate calculation of order shift 2^@order in the loop. Once again I'm not convinced. As in the other patch the compiler could do this transformation via its CSE pass if it sees fit. Also (applicable as well to the other patch) I think "fix" in the subject is misleading: There's nothing wrong with the original code. > In addition, I fixed type of total_avail_pages from long > to unsigned long. because when total_avail_pages static variable > substitute in functions of page alloc local variable, > type of local variables is unsigned long. You've done more changes, some of which are questionable. > @@ -922,8 +922,9 @@ static struct page_info *alloc_heap_pages( > struct domain *d) > { > nodeid_t node; > - unsigned int i, buddy_order, zone, first_dirty; > - unsigned long request = 1UL << order; > + unsigned int buddy_order, zone, first_dirty; > + unsigned int buddy_request; > + unsigned long i, request = 1UL << order; E.g. it's not clear why these both need to be unsigned long when the largest chunk which can be allocated in one go is 1GiB (MAX_ORDER). At least on x86 operations on 32-bit quantities are generally slightly more efficient than such on 64-bit values. If we wanted to cater for architectures setting MAX_ORDER to 32 or higher, I think the type used should become a typedef picking "unsigned int" at least on x86. > @@ -975,16 +976,17 @@ static struct page_info *alloc_heap_pages( > while ( buddy_order != order ) > { > buddy_order--; > + buddy_request = 1U << buddy_order; Such a local variable would better have narrowest possible scope. > @@ -1490,7 +1493,7 @@ static void free_heap_pages( > /* Update predecessor's first_dirty if necessary. */ > if ( predecessor->u.free.first_dirty == INVALID_DIRTY_IDX && > pg->u.free.first_dirty != INVALID_DIRTY_IDX ) > - predecessor->u.free.first_dirty = (1U << order) + > + predecessor->u.free.first_dirty = mask + > pg->u.free.first_dirty; > > pg = predecessor; > @@ -1511,7 +1514,7 @@ static void free_heap_pages( > /* Update pg's first_dirty if necessary. */ > if ( pg->u.free.first_dirty == INVALID_DIRTY_IDX && > successor->u.free.first_dirty != INVALID_DIRTY_IDX ) > - pg->u.free.first_dirty = (1U << order) + > + pg->u.free.first_dirty = mask + > successor->u.free.first_dirty; This re-use of an existing local variable looks reasonable to me. It would be nice though if the variable's scope was shrunk and its type was also adjusted to unsigned int. Jan
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |