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

Re: [PATCH v2 5/5] xen/memory, tools: Make init-dom0less consume XEN_DOMCTL_get_mem_map


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Henry Wang <xin.wang2@xxxxxxx>
  • Date: Tue, 2 Apr 2024 17:03:45 +0800
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass (sender ip is 165.204.84.17) smtp.rcpttodomain=suse.com smtp.mailfrom=amd.com; dmarc=pass (p=quarantine sp=quarantine pct=100) action=none header.from=amd.com; dkim=none (message not signed); arc=none (0)
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=Ot3IJUV1xdELtyZiGrWHJxSTxoYg9ChiuQXnKiMQWHI=; b=B3/FqW3frhzVIHaQTH0U9ZCjNs7mUM77x3pwWtAwl1fr+b79l/2LqTZBy7dVCbc5CZkQ2A2vwiEZJE64eyFs+RW2403kZflVCvIuQfzJvFBMd+ceKHH1vnR8EhOrN6O/D9N9jXNv6sy1aZ0kfLY3A79jgTyJ0vAtEqi6OrJpQS3ZO3V68I3rBEkgFbI7oRtPMfj5Io++VN5ZdIW7RHABCSb1uYpme6OD3f05P7KFTrmCEO4aKSaMpz6IxrbW1jyhXdqhDjW4T9XfFFxXYMJ+qRJJ0l4rWldPdPK2EUXRHtgSl0/dTgxjEPyY805DUvF17r27rWpGcHMP9AFf9U3yXQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=ATT0cv4dNW3b6YGjNn85ijf5H+/FwcXebzzqj3fF7d+ljvdc6yod1HFUQ+XO6uSRmDsQXRWMEhsQAPxSw9CIAdxSvxLK9eKAxrBnf+xaaTO5dJYN/9/1QdoVqQOirD4RvKD+3wUECxvi8uuXxW59b7eWAGR885fJ596yufLtNbYYdnXQusiR0evIkWyh/4WJhR4MJYfuLtsW4J43crMpCqzV1rTFaBVel/SaghHhIQn+SrAgFaDOIuAQFSIvormUqlMIiscYteNbqsdaIdKAR7ibAOyMiU654x6FM7Dhc9u5RiqdGORAxjplx6jKys0rHzmwn1Ziszge5CevLQn81Q==
  • Cc: Wei Liu <wl@xxxxxxx>, Anthony PERARD <anthony.perard@xxxxxxxxxx>, "George Dunlap" <george.dunlap@xxxxxxxxxx>, Alec Kwapis <alec.kwapis@xxxxxxxxxxxxx>, <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>
  • Delivery-date: Tue, 02 Apr 2024 09:04:15 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

Hi Jan,

On 4/2/2024 4:51 PM, Jan Beulich wrote:
On 02.04.2024 10:43, Henry Wang wrote:
On 4/2/2024 3:05 PM, Jan Beulich wrote:
On 29.03.2024 06:11, Henry Wang wrote:
On 3/12/2024 1:07 AM, Jan Beulich wrote:
+/*
+ * Flag to force populate physmap to use pages from domheap instead of 1:1
+ * or static allocation.
+ */
+#define XENMEMF_force_heap_alloc  (1<<19)
    #endif
If this is for populate_physmap only, then other sub-ops need to reject
its use.

I have to admit I'm a little wary of allocating another flag here and ...

--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -205,6 +205,8 @@ struct npfec {
    #define  MEMF_no_icache_flush (1U<<_MEMF_no_icache_flush)
    #define _MEMF_no_scrub    8
    #define  MEMF_no_scrub    (1U<<_MEMF_no_scrub)
+#define _MEMF_force_heap_alloc 9
+#define  MEMF_force_heap_alloc (1U<<_MEMF_force_heap_alloc)
    #define _MEMF_node        16
    #define  MEMF_node_mask   ((1U << (8 * sizeof(nodeid_t))) - 1)
    #define  MEMF_node(n)     ((((n) + 1) & MEMF_node_mask) << _MEMF_node)
... here - we don't have that many left. Since other sub-ops aren't
intended to support this flag, did you consider adding another (perhaps
even arch-specific) sub-op instead?
While revisiting this comment when trying to come up with a V3, I
realized adding a sub-op here in the same level as
XENMEM_populate_physmap will basically duplicate the function
populate_physmap() with just the "else" (the non-1:1 allocation) part,
also a similar xc_domain_populate_physmap_exact() & co will be needed
from the toolstack side to call the new sub-op. So I am having the
concern of the duplication of code and not sure if I understand you
correctly. Would you please elaborate a bit more or clarify if I
understand you correctly? Thanks!
Well, the goal is to avoid both code duplication and introduction of a new,
single-use flag. The new sub-op suggestion, I realize now, would mainly have
helped with avoiding the new flag in the public interface. That's still
desirable imo. Internally, have you checked which MEMF_* are actually used
by populate_physmap()? Briefly looking, e.g. MEMF_no_dma and MEMF_no_refcount
aren't. It therefore would be possible to consider re-purposing one that
isn't (likely to be) used there. Of course doing so requires care to avoid
passing that flag down to other code (page_alloc.c functions in particular),
where the meaning would be the original one.
I think you made a good point, however, to be honest I am not sure about
the repurposing flags such as MEMF_no_dma and MEMF_no_refcount, because
I think the name and the purpose of the flag should be clear and
less-misleading. Reusing either one for another meaning, namely forcing
a non-heap allocation in populate_physmap() would be confusing in the
future. Also if one day these flags will be needed in
populate_physmap(), current repurposing approach will lead to a even
confusing code base.
For the latter - hence "(likely to be)" in my earlier reply.

Agreed.

For the naming - of course an aliasing #define ought to be used then, to
make the purpose clear at the use sites.

Well I have to admit the alias #define approach is clever (thanks) and I am getting persuaded gradually, as there will be also another benefit for me to do less modification from my side :) I will firstly try this approach in v3 if ...

I also do agree very much that we need to also avoid the code
duplication, so compared to other two suggested approach, adding a new
MEMF would be the cleanest solution IMHO, as it is just one bit and MEMF
flags are not added very often.

I would also curious what the other common code maintainers will say on
this issue: @Andrew, @Stefano, @Julien, any ideas? Thanks!

...not receiving any other input, and we can continue the discussion in v3. Thanks!

Kind regards,
Henry

Jan




 


Rackspace

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