[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v6 1/8] mm: Place unscrubbed pages at the end of pagelist


On 07/08/17 15:46, Boris Ostrovsky wrote:
On 08/07/2017 06:45 AM, Julien Grall wrote:
Hi Boris,

I would have appreciated to be CCed as maintainer of the ARM bits...
Please use scripts/get_maintainers.pl in the future.

Ugh, sorry about that. (I did test builds for both ARM64 and ARM32, if
this make my transgression any less serious ;-))

On 04/08/17 18:05, Boris Ostrovsky wrote:
. so that it's easy to find pages that need to be scrubbed (those
pages are

Pointless .

diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
index ef84b72..d26b232 100644
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -44,7 +44,16 @@ 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;

You've turned need_tlbflush from bool to unsigned long : 1. But some
of the users use true/false or may rely on the boolean property.  So
it sounds like to me you want to use bool bitfields here (and in the
x86 part).

This is what we have (with this series applied):

root@ovs104> git grep need_tlbflush .
common/memory.c:    bool need_tlbflush = false;
accumulate_tlbflush(&need_tlbflush, &page[j],
common/memory.c:    if ( need_tlbflush )
common/page_alloc.c:    bool need_tlbflush = false;
common/page_alloc.c:            accumulate_tlbflush(&need_tlbflush, &pg[i],
common/page_alloc.c:    if ( need_tlbflush )
* common/page_alloc.c:        pg[i].u.free.need_tlbflush =
(page_get_owner(&pg[i]) != NULL);
common/page_alloc.c:        if ( pg[i].u.free.need_tlbflush )
include/asm-arm/mm.h:                unsigned long need_tlbflush:1;
include/asm-x86/mm.h:           unsigned long need_tlbflush:1;
include/xen/mm.h:static inline void accumulate_tlbflush(bool *need_tlbflush,
include/xen/mm.h:    if ( page->u.free.need_tlbflush &&
include/xen/mm.h:         (!*need_tlbflush ||
include/xen/mm.h:        *need_tlbflush = true;

The only possibly boolean-style use is '*' and event that I think is

Stand-alone need_tlbflush variables above have nothing to do with this

If you look at it, all of them use bool semantic. Now you mix bool and int. We are trying to remove that in the new code, so please don't introduce new one.

+            /*
+             * Index of the first *possibly* unscrubbed page in the
+             * One more than maximum possible order to accommodate
+             * INVALID_DIRTY_IDX.
+             */
+#define INVALID_DIRTY_IDX ((1UL << (MAX_ORDER + 1)) - 1)
+            unsigned long first_dirty:MAX_ORDER + 1;

We need to make sure that this union will not be bigger than unsigned
long. Otherwise this will limit lower down the maximum amount of
memory we support.
So this likely means a BUILD_BUG_ON(....).

Are you concerned that (MAX_ORDER+1) will be larger than sizeof(unsigned
long)? If yes, the compiler should complain anyway, shouldn't it?

I am more concerned about the sizeof of the union u to be larger than unsigned long.

first_dirty should not be greater than 63 bits (or 31 bits for 32-bits architecture). Otherwise likely the compiler will add a padding between need_tlbflush and first_dirty. This would increase the page_info by 4/8 byte.

The BUILD_BUG_ON(...) would be here to catch such error.


Julien Grall

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.