| Message ID | 20220912055356.24064-3-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/gnttab: reduce size of struct active_grant_entry | expand |
On 12.09.2022 07:53, Juergen Gross wrote: > The size of struct active_grant_entry for 64-bit builds is 40 or 48 > bytes today (with or without NDEBUG). ... and leaving aside DEBUG_LOCKS / DEBUG_LOCK_PROFILE. > 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 last known user has been the old > Xenolinux branch of the Linux kernel. That tree used grant transfers, but I'm unaware of (actual) uses of transitive grants; there may have been code there to allow their use, but iirc that would then not have had any users. Or am I mis-remembering? > --- 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 trans_domid; /* Domain granting access. */ I'm afraid I consider the comment misleading and not in line with field name. For normal grants this is merely the "source" domain (i.e. "granting" in the comment is correct, but "trans" in the name isn't), while for transitive grants there are two domains granting access. Therefore may I suggest to use src_domid or orig_domid, or otherwise omit the comment (just like there was none originally)? Jan
On 12.09.22 10:11, Jan Beulich wrote: > On 12.09.2022 07:53, Juergen Gross wrote: >> The size of struct active_grant_entry for 64-bit builds is 40 or 48 >> bytes today (with or without NDEBUG). > > ... and leaving aside DEBUG_LOCKS / DEBUG_LOCK_PROFILE. Only leaving aside DEBUG_LOCK_PROFILE. DEBUG_LOCKS doesn't matter regarding struct active_grant_entry size. > >> 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 last known user has been the old >> Xenolinux branch of the Linux kernel. > > That tree used grant transfers, but I'm unaware of (actual) uses of > transitive grants; there may have been code there to allow their > use, but iirc that would then not have had any users. Or am I > mis-remembering? Oh, maybe I mixed those two cases up. >> --- 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 trans_domid; /* Domain granting access. */ > > I'm afraid I consider the comment misleading and not in line with > field name. For normal grants this is merely the "source" domain > (i.e. "granting" in the comment is correct, but "trans" in the > name isn't), while for transitive grants there are two domains > granting access. Therefore may I suggest to use src_domid or > orig_domid, or otherwise omit the comment (just like there was > none originally)? I'd prefer the rename to src_domid (orig_domid sounds for me like an old value of domid). Juergen
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c index fba329dcc2..8e4bbe3824 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 trans_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->trans_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->trans_domid == rd->domain_id) + ? rd : domid_to_domain(act->trans_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->trans_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->trans_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->trans_domid = td->domain_id; act->trans_gref = trans_gref; act->mfn = grant_mfn; }
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 last known user has been the old Xenolinux branch of the Linux kernel. 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 today. Signed-off-by: Juergen Gross <jgross@suse.com> --- xen/common/grant_table.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)