[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v1 06/13] xen/arm: assign shared memory to owner when host address not provided
On 09/01/2023 07:49, Penny Zheng wrote:
Hi Julien
Hi Penny,
Happy new year~~~~
Happy new year too!
-----Original Message-----
From: Julien Grall <julien@xxxxxxx>
Sent: Sunday, January 8, 2023 8:53 PM
To: Penny Zheng <Penny.Zheng@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
Cc: Wei Chen <Wei.Chen@xxxxxxx>; Stefano Stabellini
<sstabellini@xxxxxxxxxx>; Bertrand Marquis <Bertrand.Marquis@xxxxxxx>;
Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
Subject: Re: [PATCH v1 06/13] xen/arm: assign shared memory to owner
when host address not provided
Hi,
On 15/11/2022 02:52, Penny Zheng wrote:
@@ -922,33 +927,82 @@ static mfn_t __init
acquire_shared_memory_bank(struct domain *d,
d->max_pages += nr_pfns;
smfn = maddr_to_mfn(pbase);
- res = acquire_domstatic_pages(d, smfn, nr_pfns, 0);
- if ( res )
+ page = mfn_to_page(smfn);
+ /*
+ * If page is allocated from heap as static shared memory, then we just
+ * assign it to the owner domain
+ */
+ if ( page->count_info == (PGC_state_inuse | PGC_static) )
I am a bit confused how this can help differentiating becaPGC_state_inuse is
0. So effectively, you are checking that count_info is equal to PGC_static.
When host address is provided, the host address range defined in
xen,static-mem will be stored as a "struct membank" with type
of "MEMBANK_STATIC_DOMAIN" in "bootinfo.reserved_mem"
Then it will be initialized as static memory through "init_staticmem_pages"
So here its page->count_info is PGC_state_free |PGC_static.
For pages allocated from heap, its page state is different, being
PGC_state_inuse.
So actually, we are checking page state to tell the difference
.
Ok. This is definitely not obvious from the code. But I think this is a
very fragile assumption.
Instead, it would be better if we allocate the memory in
acquire_shared_memory_bank() when the host address is not provided.
But as I wrote in a previous patch, I don't think you should convert
{xen,dom}heap pages to a static pages.
I agree that taking reference could also prevent giving these pages back to
heap.
But may I ask what is your concern on converting {xen,dom}heap pages to a
static pages?
A few reasons:
1) I consider them as two distinct allocators. So far they have the
same behavior, but in the future this may change.
2) If the page is freed you really don't want the domain to be able to
re-use the page for a different purpose.
I realize that 2) is already a problem today with static pages. So I
think the best is to ensure that pages allocated for shared memory never
reach the any of the allocators.
[...]
+static int __init assign_shared_memory(struct domain *d,
+ struct shm_membank *shm_membank,
+ paddr_t gbase) {
+ int ret = 0;
+ unsigned long nr_pages, nr_borrowers;
+ struct page_info *page;
+ unsigned int i;
+ struct meminfo *meminfo;
+
+ /* Host address is not provided in "xen,shared-mem" */
+ if ( shm_membank->mem.banks.meminfo )
+ {
+ meminfo = shm_membank->mem.banks.meminfo;
+ for ( i = 0; i < meminfo->nr_banks; i++ )
+ {
+ ret = acquire_shared_memory(d,
+ meminfo->bank[i].start,
+ meminfo->bank[i].size,
+ gbase);
+ if ( ret )
+ return ret;
+
+ gbase += meminfo->bank[i].size;
+ }
+ }
+ else
+ {
+ ret = acquire_shared_memory(d,
+ shm_membank->mem.bank->start,
+ shm_membank->mem.bank->size, gbase);
+ if ( ret )
+ return ret;
+ }
Looking at this change and...
+
/*
* Get the right amount of references per page, which is the number of
* borrower domains.
@@ -984,23 +1076,37 @@ static int __init assign_shared_memory(struct
domain *d,
* So if the borrower is created first, it will cause adding pages
* in the P2M without reference.
*/
- page = mfn_to_page(smfn);
- for ( i = 0; i < nr_pages; i++ )
+ if ( shm_membank->mem.banks.meminfo )
{
- if ( !get_page_nr(page + i, d, nr_borrowers) )
+ meminfo = shm_membank->mem.banks.meminfo;
+ for ( i = 0; i < meminfo->nr_banks; i++ )
{
- printk(XENLOG_ERR
- "Failed to add %lu references to page %"PRI_mfn".\n",
- nr_borrowers, mfn_x(smfn) + i);
- goto fail;
+ page = mfn_to_page(maddr_to_mfn(meminfo->bank[i].start));
+ nr_pages = PFN_DOWN(meminfo->bank[i].size);
+ ret = add_shared_memory_ref(d, page, nr_pages, nr_borrowers);
+ if ( ret )
+ goto fail;
}
}
+ else
+ {
+ page = mfn_to_page(
+ maddr_to_mfn(shm_membank->mem.bank->start));
+ nr_pages = shm_membank->mem.bank->size >> PAGE_SHIFT;
+ ret = add_shared_memory_ref(d, page, nr_pages, nr_borrowers);
+ if ( ret )
+ return ret;
+ }
... this one. The code to deal with a bank is exactly the same. But you need
the duplication because you special case "one bank".
As I wrote in a previous patch, I don't think we should special case it.
If the concern is memory usage, then we should look at reworking meminfo
instead (or using a different structure).
A few concerns explained why I didn't choose "struct meminfo" over two
pointers "struct membank*" and "struct meminfo*".
1) memory usage is the main reason.
If we use "struct meminfo" over the current "struct membank*" and "struct
meminfo*",
"struct shm_meminfo" will become a array of 256 "struct shm_membank", with
"struct shm_membank" being also an 256-item array, that is 256 * 256, too big
for
a structure and If I remembered clearly, it will lead to "more than PAGE_SIZE"
compiling error.
I am not aware of any place where we would restrict the size of kinfo in
upstream. Can you give me a pointer?
FWIT, either reworking meminfo or using a different structure, are both leading
to sizing down
the array, hmmm, I don't know which size is suitable. That's why I prefer
pointer
and dynamic allocation.
I would expect that in most cases, you will need only one bank when the
host address is not provided. So I think this is a bit odd to me to
impose a "large" allocation for them.
2) If we use "struct meminfo*" over the current "struct membank*" and "struct
meminfo*",
we will need a new flag to differentiate two scenarios(host address provided or
not), As
the special case "struct membank*" is already helping us differentiate.
And since when host address is provided, the related "struct membank" also
needs to be
reserved in "bootinfo.reserved_mem". That's why I used pointer " struct
membank*" to
avoid memory waste.
I am confused... Today you are defining as:
struct {
struct membank *;
struct {
struct meminfo *;
unsigned long total_size;
}
}
So on arm64 host, you would use 24 bytes. If we were using an union
instead. We would use 16 bytes. Adding a 32-bit fields, would bring to
20 bytes (assuming we can re-use a padding).
Therefore, there is no memory waste here with a flag...
Cheers,
--
Julien Grall
|
