[PATCH v2 0/2] xen/gnttab: reduce size of struct active_grant_entry

[PATCH v2 0/2] xen/gnttab: reduce size of struct active_grant_entry

[Juergen Gross]

While looking at the grant table code I thought it should be possible
to have a smaller struct active_grant_entry. This approach should only
hit transitive grants with some negative performance effect, "normal"
grants should be not affected.

Juergen Gross (2):
  xen: add knownalive_domain_from_domid() helper
  xen/gnttab: reduce size of struct active_grant_entry

 xen/common/domain.c      | 53 +++++++++++++++++++++++++---------------
 xen/common/grant_table.c | 13 +++++-----
 xen/include/xen/sched.h  |  4 +++
 3 files changed, 44 insertions(+), 26 deletions(-)

-- 
2.35.3

[PATCH v2 1/2] xen: add knownalive_domain_from_domid() helper

[Juergen Gross]

Add a helper knownalive_domain_from_domid() returning the struct domain
pointer for a domain give by its domid and which is known not being
able to be released (its reference count isn't incremented and no
rcu_lock_domain() is called for it).

In order to simplify coding add an internal helper for doing the lookup
and call that from the new function and similar functions.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- rename helper to knownalive_domain_from_domid() (Jan Beulich)
- enhance comment in header (Jan Beulich)
- rename internal helper (Julien Grall)
---
 xen/common/domain.c     | 53 +++++++++++++++++++++++++----------------
 xen/include/xen/sched.h |  4 ++++
 2 files changed, 37 insertions(+), 20 deletions(-)

diff --git a/xen/common/domain.c b/xen/common/domain.c
index 8dd6cd5a8f..35e0dc5139 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -835,25 +835,32 @@ out:
     return 0;
 }
 
-
-struct domain *get_domain_by_id(domid_t dom)
+/* rcu_read_lock(&domlist_read_lock) must be held. */
+static struct domain *domid_to_domain(domid_t dom)
 {
     struct domain *d;
 
-    rcu_read_lock(&domlist_read_lock);
-
     for ( d = rcu_dereference(domain_hash[DOMAIN_HASH(dom)]);
           d != NULL;
           d = rcu_dereference(d->next_in_hashbucket) )
     {
         if ( d->domain_id == dom )
-        {
-            if ( unlikely(!get_domain(d)) )
-                d = NULL;
-            break;
-        }
+            return d;
     }
 
+    return NULL;
+}
+
+struct domain *get_domain_by_id(domid_t dom)
+{
+    struct domain *d;
+
+    rcu_read_lock(&domlist_read_lock);
+
+    d = domid_to_domain(dom);
+    if ( d && unlikely(!get_domain(d)) )
+        d = NULL;
+
     rcu_read_unlock(&domlist_read_lock);
 
     return d;
@@ -862,20 +869,26 @@ struct domain *get_domain_by_id(domid_t dom)
 
 struct domain *rcu_lock_domain_by_id(domid_t dom)
 {
-    struct domain *d = NULL;
+    struct domain *d;
 
     rcu_read_lock(&domlist_read_lock);
 
-    for ( d = rcu_dereference(domain_hash[DOMAIN_HASH(dom)]);
-          d != NULL;
-          d = rcu_dereference(d->next_in_hashbucket) )
-    {
-        if ( d->domain_id == dom )
-        {
-            rcu_lock_domain(d);
-            break;
-        }
-    }
+    d = domid_to_domain(dom);
+    if ( d )
+        rcu_lock_domain(d);
+
+    rcu_read_unlock(&domlist_read_lock);
+
+    return d;
+}
+
+struct domain *knownalive_domain_from_domid(domid_t dom)
+{
+    struct domain *d;
+
+    rcu_read_lock(&domlist_read_lock);
+
+    d = domid_to_domain(dom);
 
     rcu_read_unlock(&domlist_read_lock);
 
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 557b3229f6..9e9c3d834b 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -737,8 +737,12 @@ static inline struct domain *rcu_lock_current_domain(void)
     return /*rcu_lock_domain*/(current->domain);
 }
 
+/* Get struct domain AND increase ref-count of domain. */
 struct domain *get_domain_by_id(domid_t dom);
 
+/* Get struct domain known to have reference held or being RCU-locked. */
+struct domain *knownalive_domain_from_domid(domid_t dom);
+
 struct domain *get_pg_owner(domid_t domid);
 
 static inline void put_pg_owner(struct domain *pg_owner)
-- 
2.35.3

[PATCH v2 2/2] xen/gnttab: reduce size of struct active_grant_entry

[Juergen Gross]

The size of struct active_grant_entry for 64-bit builds is 40 or 48
bytes today (with or without NDEBUG).

It can easily be reduced by 8 bytes by replacing the trans_domain
pointer with the domid of the related domain. trans_domain is only ever
used for transitive grants, which doesn't have any known users.

This reduction will result in less memory usage and (for production
builds) in faster code, as indexing into the active_grant_entry array
will be much easier with an entry having a power-of-2 size.

The performance loss when using transitive grants shouldn't really
matter, given the probability that those aren't in use at all.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- rename trans_domid to src_domid (Jan Beulich)
---
 xen/common/grant_table.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index fba329dcc2..59342df3b7 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -381,13 +381,13 @@ struct active_grant_entry {
 })
 
     domid_t       domid;  /* Domain being granted access.             */
+    domid_t       src_domid; /* Domain granting access.               */
     unsigned int  start:15; /* For sub-page grants, the start offset
                                in the page.                           */
     bool          is_sub_page:1; /* True if this is a sub-page grant. */
     unsigned int  length:16; /* For sub-page grants, the length of the
                                 grant.                                */
     grant_ref_t   trans_gref;
-    struct domain *trans_domain;
     mfn_t         mfn;    /* Machine frame being granted.             */
 #ifndef NDEBUG
     gfn_t         gfn;    /* Guest's idea of the frame being granted. */
@@ -1095,7 +1095,7 @@ map_grant_ref(
             act->start = 0;
             act->length = PAGE_SIZE;
             act->is_sub_page = false;
-            act->trans_domain = rd;
+            act->src_domid = rd->domain_id;
             act->trans_gref = ref;
         }
     }
@@ -2494,7 +2494,8 @@ release_grant_for_copy(
     else
     {
         status = &status_entry(rgt, gref);
-        td = act->trans_domain;
+        td = (act->src_domid == rd->domain_id)
+             ? rd : knownalive_domain_from_domid(act->src_domid);
         trans_gref = act->trans_gref;
     }
 
@@ -2657,7 +2658,7 @@ acquire_grant_for_copy(
                           !mfn_eq(act->mfn, grant_mfn) ||
                           act->start != trans_page_off ||
                           act->length != trans_length ||
-                          act->trans_domain != td ||
+                          act->src_domid != td->domain_id ||
                           act->trans_gref != trans_gref ||
                           !act->is_sub_page)) )
         {
@@ -2676,7 +2677,7 @@ acquire_grant_for_copy(
             act->domid = ldom;
             act->start = trans_page_off;
             act->length = trans_length;
-            act->trans_domain = td;
+            act->src_domid = td->domain_id;
             act->trans_gref = trans_gref;
             act->mfn = grant_mfn;
             act_set_gfn(act, INVALID_GFN);
@@ -2738,7 +2739,7 @@ acquire_grant_for_copy(
             act->is_sub_page = is_sub_page;
             act->start = trans_page_off;
             act->length = trans_length;
-            act->trans_domain = td;
+            act->src_domid = td->domain_id;
             act->trans_gref = trans_gref;
             act->mfn = grant_mfn;
         }
-- 
2.35.3

Re: [PATCH v2 2/2] xen/gnttab: reduce size of struct active_grant_entry

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1823 bytes --]

On 15.09.22 13:50, Jan Beulich wrote:
> On 13.09.2022 11:32, Juergen Gross wrote:
>> The size of struct active_grant_entry for 64-bit builds is 40 or 48
>> bytes today (with or without NDEBUG).
>>
>> It can easily be reduced by 8 bytes by replacing the trans_domain
>> pointer with the domid of the related domain. trans_domain is only ever
>> used for transitive grants, which doesn't have any known users.
>>
>> This reduction will result in less memory usage and (for production
>> builds) in faster code, as indexing into the active_grant_entry array
>> will be much easier with an entry having a power-of-2 size.
>>
>> The performance loss when using transitive grants shouldn't really
>> matter, given the probability that those aren't in use at all.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>> V2:
>> - rename trans_domid to src_domid (Jan Beulich)
>> ---
>>   xen/common/grant_table.c | 13 +++++++------
>>   1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
>> index fba329dcc2..59342df3b7 100644
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -381,13 +381,13 @@ struct active_grant_entry {
>>   })
>>   
>>       domid_t       domid;  /* Domain being granted access.             */
>> +    domid_t       src_domid; /* Domain granting access.               */
> 
> I'm afraid I still view the comment as ambiguous, for there being two
> domains involved in granting access for transitive grants. Preferably
> with e.g "Original" added (which of course could be done by the
> committer, provided this isn't lost by the time 4.18 opens),
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

I'll just send an updated V3 (probably tomorrow).


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]