Re: [PATCH] device-tree: optimize dt_device_for_passthrough()

["Orzel, Michal" <michal.orzel@xxxxxxx>]

On 11/02/2025 16:14, Grygorii Strashko wrote:
> 
> 
> On 11.02.25 14:32, Julien Grall wrote:
>>
>>
>> On 11/02/2025 11:57, Orzel, Michal wrote:
>>> On 11/02/2025 12:18, Grygorii Strashko wrote:
>>>>
>>>>
>>>> The dt_device_for_passthrough() is called many times during Xen
>>>> initialization and Dom0 creation. On every call it traverses struct
>>>> dt_device_node properties list and compares compares properties name with
>>> double "compares"
> 
> 10x
> 
>>>
>>>> "xen,passthrough" which is runtime overhead. This can be optimized by
>>> Are you sure? Looking at the calls, it's almost only used at boot except 
>>> for dt
>>> overlay.
>>>
>>>> marking dt_device_node as passthrough while unflattening DT.
>>>>
>>>> This patch introduced new struct dt_device_node property "is_passthrough"
>>>> which is filled if "xen,passthrough" property is present while unflattening
>>>> DT and dt_device_for_passthrough() just return it's value.
>>> In the past we were skeptical about adding new fields to the dt_device_node
>>> structure for use cases like this one. I would say this optimization is not
>>> worth it. Also, why would you optimize dt_device_for_passthrough but not
>>> e.g. dt_device_is_available.
>>
>> So we are trading speed with memory usage. It looks like we may be using a 
>> padding, although I didn't check whether the existing structure could be 
>> packed...
> 
> Actually I see it consumes nothing due to alignments:
> - before sizeof(dt_device_node)=144
> - after sizeof(dt_device_node)=144
> 
> But it could be made bool is_passthrough:1; together with other bools, and 
> probably moved at the end of struct dt_device_node.
> 
> By the way, below diff can save 8 bytes on arm64 per struct dt_device_node.
See below.

> 
>>
>>>
>>> You can check with other Arm maintainers.
>>
>> Before forging an opinion, I'd like to see some numbers showing the 
>> performance improvement. Also, is this impacting only boot?
> 
> Sry, indeed only boot, need to be more specific.
> 
> My DT: ~700 nodes
> Number of calls till the end of create_dom0():
> (XEN) =============== dt_device_for_passthrough=2684 
> dt_device_is_available=1429.
> 
> I've enabled console_timestamps=boot and did 5 boots and calculated average - 
> it gives ~20 microseconds improvement.
> 
> 
>>
>> Also, I agree with Michal, if this is a concern for 
>> dt_device_device_for_passthrough(). Then it would be a concern for a few 
>> others calls using dt_find_property(). Are you going to modify all of them?
> 
> I follow the rule one patch one functional change. Regarding further 
> optimization - I think only generic properties can be optimized and 
> personally I see now only "xen,passthrough" and may be "status".
> 
> Ok. 20 microseconds. it's probably more like a measurement error margin.
> Please advice if i should continue or drop?
I'd suggest to drop it.

If we want to optimize the struct, then it does not make sense to do this only
for one architecture. If at all, I would do:

diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index 5ff763bb80bb..0ff80fda04da 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -81,17 +81,10 @@ struct dt_property {
 struct dt_device_node {
     const char *name;
     const char *type;
-    dt_phandle phandle;
     char *full_name;
+    dt_phandle phandle;
     domid_t used_by; /* By default it's used by dom0 */

-    struct dt_property *properties;
-    struct dt_device_node *parent;
-    struct dt_device_node *child;
-    struct dt_device_node *sibling;
-    struct dt_device_node *next; /* TODO: Remove it. Only use to know the last
children */
-    struct dt_device_node *allnext;
-
     /* IOMMU specific fields */
     bool is_protected;

@@ -100,6 +93,13 @@ struct dt_device_node {
     bool static_evtchn_created;
 #endif

+    struct dt_property *properties;
+    struct dt_device_node *parent;
+    struct dt_device_node *child;
+    struct dt_device_node *sibling;
+    struct dt_device_node *next; /* TODO: Remove it. Only use to know the last
children */
+    struct dt_device_node *allnext;
+
     /*
      * The main purpose of this list is to link the structure in the list
      * of devices assigned to domain.

Results (defconfig):
ARM64: before: 144B, 16B holes
ARM64: after: 128B, no holes (-16B)

ARM32: before: 72B, 4B holes
ARM32: after: 68B, no holes (-4B)

~Michal