| Message ID | 1503952829-11065-4-git-send-email-boris.ostrovsky@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Aug 28, 2017 at 04:40:28PM -0400, Boris Ostrovsky wrote: > There is no need to scrub pages freed during dom0 construction > since heap will be scrubbed once dom0 is ready (by scrub_heap_pages()). > > Since boot_scrub_done will not be set if boot-time scrubbing is off we > also check for domain state. > > Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> > --- > xen/common/page_alloc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c > index 34a7992..b93dae9 100644 > --- a/xen/common/page_alloc.c > +++ b/xen/common/page_alloc.c > @@ -2259,7 +2259,7 @@ void free_domheap_pages(struct page_info *pg, unsigned int order) > */ > scrub = !!d->is_dying; > #else > - scrub = true; > + scrub = boot_scrub_done || !!d->is_dying; It seems that the debug and non-debug case should use the same predicate.
>>> On 29.08.17 at 13:56, <wei.liu2@citrix.com> wrote: > On Mon, Aug 28, 2017 at 04:40:28PM -0400, Boris Ostrovsky wrote: >> There is no need to scrub pages freed during dom0 construction >> since heap will be scrubbed once dom0 is ready (by scrub_heap_pages()). >> >> Since boot_scrub_done will not be set if boot-time scrubbing is off we >> also check for domain state. >> >> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> >> --- >> xen/common/page_alloc.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c >> index 34a7992..b93dae9 100644 >> --- a/xen/common/page_alloc.c >> +++ b/xen/common/page_alloc.c >> @@ -2259,7 +2259,7 @@ void free_domheap_pages(struct page_info *pg, unsigned int order) >> */ >> scrub = !!d->is_dying; >> #else >> - scrub = true; >> + scrub = boot_scrub_done || !!d->is_dying; > > It seems that the debug and non-debug case should use the same > predicate. No - boot_scrub_done doesn't even exist in the other case. Jan
>>> On 28.08.17 at 22:40, <boris.ostrovsky@oracle.com> wrote: > --- a/xen/common/page_alloc.c > +++ b/xen/common/page_alloc.c > @@ -2259,7 +2259,7 @@ void free_domheap_pages(struct page_info *pg, unsigned int order) > */ > scrub = !!d->is_dying; > #else > - scrub = true; > + scrub = boot_scrub_done || !!d->is_dying; > #endif Actually Wei has a point here - already when reviewing the original series I had wondered whether a "#define boot_scrub_done false" in the non-debug case wouldn't help eliminate some #ifdef-ary. Here it would mean fulfilling Wei's request indirectly, by simply removing the preprocessor directives altogether. In any event the !! is pointless on an operand to || or && . Jan
On 08/29/2017 09:22 AM, Jan Beulich wrote: >>>> On 29.08.17 at 13:56, <wei.liu2@citrix.com> wrote: >> On Mon, Aug 28, 2017 at 04:40:28PM -0400, Boris Ostrovsky wrote: >>> There is no need to scrub pages freed during dom0 construction >>> since heap will be scrubbed once dom0 is ready (by scrub_heap_pages()). >>> >>> Since boot_scrub_done will not be set if boot-time scrubbing is off we >>> also check for domain state. >>> >>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> >>> --- >>> xen/common/page_alloc.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c >>> index 34a7992..b93dae9 100644 >>> --- a/xen/common/page_alloc.c >>> +++ b/xen/common/page_alloc.c >>> @@ -2259,7 +2259,7 @@ void free_domheap_pages(struct page_info *pg, unsigned int order) >>> */ >>> scrub = !!d->is_dying; >>> #else >>> - scrub = true; >>> + scrub = boot_scrub_done || !!d->is_dying; >> It seems that the debug and non-debug case should use the same >> predicate. > No - boot_scrub_done doesn't even exist in the other case. I read Wei's message as suggesting scrub = !!d->is_dying; #ifdef CONFIG_SCRUB_DEBUG scrub |= boot_scrub_done; #endif which I could do. -boris
On Tue, Aug 29, 2017 at 07:26:28AM -0600, Jan Beulich wrote: > >>> On 28.08.17 at 22:40, <boris.ostrovsky@oracle.com> wrote: > > --- a/xen/common/page_alloc.c > > +++ b/xen/common/page_alloc.c > > @@ -2259,7 +2259,7 @@ void free_domheap_pages(struct page_info *pg, unsigned int order) > > */ > > scrub = !!d->is_dying; > > #else > > - scrub = true; > > + scrub = boot_scrub_done || !!d->is_dying; > > #endif > > Actually Wei has a point here - already when reviewing the original > series I had wondered whether a "#define boot_scrub_done false" > in the non-debug case wouldn't help eliminate some #ifdef-ary. > Here it would mean fulfilling Wei's request indirectly, by simply > removing the preprocessor directives altogether. This SGTM
>>> On 29.08.17 at 15:27, <boris.ostrovsky@oracle.com> wrote: > On 08/29/2017 09:22 AM, Jan Beulich wrote: >>>>> On 29.08.17 at 13:56, <wei.liu2@citrix.com> wrote: >>> On Mon, Aug 28, 2017 at 04:40:28PM -0400, Boris Ostrovsky wrote: >>>> There is no need to scrub pages freed during dom0 construction >>>> since heap will be scrubbed once dom0 is ready (by scrub_heap_pages()). >>>> >>>> Since boot_scrub_done will not be set if boot-time scrubbing is off we >>>> also check for domain state. >>>> >>>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> >>>> --- >>>> xen/common/page_alloc.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c >>>> index 34a7992..b93dae9 100644 >>>> --- a/xen/common/page_alloc.c >>>> +++ b/xen/common/page_alloc.c >>>> @@ -2259,7 +2259,7 @@ void free_domheap_pages(struct page_info *pg, unsigned > int order) >>>> */ >>>> scrub = !!d->is_dying; >>>> #else >>>> - scrub = true; >>>> + scrub = boot_scrub_done || !!d->is_dying; >>> It seems that the debug and non-debug case should use the same >>> predicate. >> No - boot_scrub_done doesn't even exist in the other case. > > > I read Wei's message as suggesting > > scrub = !!d->is_dying; > #ifdef CONFIG_SCRUB_DEBUG > scrub |= boot_scrub_done; > #endif > > > which I could do. Yes, but please consider the "#define boot_scrub_done 0" approach too. Jan
> Yes, but please consider the "#define boot_scrub_done 0" approach > too. Yes, I'll do that but I will rename boot_scrub_done to scrub_debug since having boot_scrub_done=0 in non-debug case will not convey what it actually is supposed to mean. -boris
>>> On 29.08.17 at 16:20, <boris.ostrovsky@oracle.com> wrote: >> Yes, but please consider the "#define boot_scrub_done 0" approach >> too. > > Yes, I'll do that but I will rename boot_scrub_done to scrub_debug since > having boot_scrub_done=0 in non-debug case will not convey what it > actually is supposed to mean. Yeah, I too did realize that, but didn't have a good idea for a replacement name. Yours sounds good. Jan
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c index 34a7992..b93dae9 100644 --- a/xen/common/page_alloc.c +++ b/xen/common/page_alloc.c @@ -2259,7 +2259,7 @@ void free_domheap_pages(struct page_info *pg, unsigned int order) */ scrub = !!d->is_dying; #else - scrub = true; + scrub = boot_scrub_done || !!d->is_dying; #endif } else
There is no need to scrub pages freed during dom0 construction since heap will be scrubbed once dom0 is ready (by scrub_heap_pages()). Since boot_scrub_done will not be set if boot-time scrubbing is off we also check for domain state. Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> --- xen/common/page_alloc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)