| Message ID | 20240129171811.21382-10-carlo.nonato@minervasys.tech (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Arm cache coloring | expand |
On 29.01.2024 18:18, Carlo Nonato wrote: > PGC_static and PGC_extra are flags that needs to be preserved when assigning > a page. Define a new macro that groups those flags and use it instead of > or'ing every time. While here you say where the "preserving" applies, ... > --- a/xen/common/page_alloc.c > +++ b/xen/common/page_alloc.c > @@ -157,6 +157,8 @@ > #define PGC_static 0 > #endif > > +#define PGC_preserved (PGC_extra | PGC_static) ... nothing is said here. From the earlier version I also seem to recall that the constant was then used outside of assign_pages(). That would then mean amending whatever comment would be added here. Jan
Hi Jan, On Thu, Feb 1, 2024 at 3:24 PM Jan Beulich <jbeulich@suse.com> wrote: > > On 29.01.2024 18:18, Carlo Nonato wrote: > > PGC_static and PGC_extra are flags that needs to be preserved when assigning > > a page. Define a new macro that groups those flags and use it instead of > > or'ing every time. > > While here you say where the "preserving" applies, ... > > > --- a/xen/common/page_alloc.c > > +++ b/xen/common/page_alloc.c > > @@ -157,6 +157,8 @@ > > #define PGC_static 0 > > #endif > > > > +#define PGC_preserved (PGC_extra | PGC_static) > > ... nothing is said here. From the earlier version I also seem to recall > that the constant was then used outside of assign_pages(). That would > then mean amending whatever comment would be added here. Yes, but it was used in places where the name didn't fit (to stop merging in free_heap_pages()) and so I thought it would've been better to use the constant only for one of the two concepts: only for preserved flags in assign_pages(). Are you suggesting adding a comment to this #define to clarify its usage? Thanks. > Jan
On 01.02.2024 15:49, Carlo Nonato wrote: > On Thu, Feb 1, 2024 at 3:24 PM Jan Beulich <jbeulich@suse.com> wrote: >> On 29.01.2024 18:18, Carlo Nonato wrote: >>> PGC_static and PGC_extra are flags that needs to be preserved when assigning >>> a page. Define a new macro that groups those flags and use it instead of >>> or'ing every time. >> >> While here you say where the "preserving" applies, ... >> >>> --- a/xen/common/page_alloc.c >>> +++ b/xen/common/page_alloc.c >>> @@ -157,6 +157,8 @@ >>> #define PGC_static 0 >>> #endif >>> >>> +#define PGC_preserved (PGC_extra | PGC_static) >> >> ... nothing is said here. From the earlier version I also seem to recall >> that the constant was then used outside of assign_pages(). That would >> then mean amending whatever comment would be added here. > > Yes, but it was used in places where the name didn't fit (to stop merging in > free_heap_pages()) and so I thought it would've been better to use the > constant only for one of the two concepts: only for preserved flags in > assign_pages(). > > Are you suggesting adding a comment to this #define to clarify its usage? Yes. Albeit if I understand the earlier paragraph right, you don't use it further (anymore). In which case the question would be: Is the patch still needed, and if so is it rightfully part of this series? Jan
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c index 2ec17df9b4..7c135e0bb4 100644 --- a/xen/common/page_alloc.c +++ b/xen/common/page_alloc.c @@ -157,6 +157,8 @@ #define PGC_static 0 #endif +#define PGC_preserved (PGC_extra | PGC_static) + #ifndef PGT_TYPE_INFO_INITIALIZER #define PGT_TYPE_INFO_INITIALIZER 0 #endif @@ -2362,7 +2364,7 @@ int assign_pages( for ( i = 0; i < nr; i++ ) { - ASSERT(!(pg[i].count_info & ~(PGC_extra | PGC_static))); + ASSERT(!(pg[i].count_info & ~PGC_preserved)); if ( pg[i].count_info & PGC_extra ) extra_pages++; } @@ -2422,7 +2424,7 @@ int assign_pages( page_set_owner(&pg[i], d); smp_wmb(); /* Domain pointer must be visible before updating refcnt. */ pg[i].count_info = - (pg[i].count_info & (PGC_extra | PGC_static)) | PGC_allocated | 1; + (pg[i].count_info & PGC_preserved) | PGC_allocated | 1; page_list_add_tail(&pg[i], page_to_list(d, &pg[i])); }
PGC_static and PGC_extra are flags that needs to be preserved when assigning a page. Define a new macro that groups those flags and use it instead of or'ing every time. Signed-off-by: Carlo Nonato <carlo.nonato@minervasys.tech> --- v6: - preserved_flags renamed to PGC_preserved - PGC_preserved is used only in assign_pages() v5: - new patch --- xen/common/page_alloc.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)