Re: [Xen-devel] [PATCH v8 2/4] add a domain_tot_pages() helper function
> -Original Message- > From: Xen-devel On Behalf Of > Durrant, Paul > Sent: 31 January 2020 15:19 > To: Jan Beulich ; Julien Grall > Cc: Stefano Stabellini ; Wei Liu ; > Konrad Rzeszutek Wilk ; George Dunlap > ; Andrew Cooper ; > Ian Jackson ; Tim Deegan ; xen- > de...@lists.xenproject.org; Roger Pau Monné > Subject: Re: [Xen-devel] [PATCH v8 2/4] add a domain_tot_pages() helper > function > > > -Original Message- > > From: Jan Beulich > > Sent: 31 January 2020 12:53 > > To: Durrant, Paul > > Cc: xen-devel@lists.xenproject.org; Andrew Cooper > > ; Wei Liu ; Roger Pau Monné > > ; George Dunlap ; Ian > > Jackson ; Julien Grall ; > Konrad > > Rzeszutek Wilk ; Stefano Stabellini > > ; Tim Deegan > > Subject: Re: [PATCH v8 2/4] add a domain_tot_pages() helper function > > > > On 30.01.2020 15:57, Paul Durrant wrote: > > > v8: > > > - New in v8 > > > --- > > > xen/arch/x86/domain.c | 2 +- > > > xen/arch/x86/mm.c | 6 +++--- > > > xen/arch/x86/mm/p2m-pod.c | 10 +- > > > xen/arch/x86/mm/shadow/common.c | 2 +- > > > xen/arch/x86/msi.c | 2 +- > > > xen/arch/x86/numa.c | 2 +- > > > xen/arch/x86/pv/dom0_build.c| 25 + > > > xen/arch/x86/pv/domain.c| 2 +- > > > xen/common/domctl.c | 2 +- > > > xen/common/grant_table.c| 4 ++-- > > > xen/common/keyhandler.c | 2 +- > > > xen/common/memory.c | 4 ++-- > > > xen/common/page_alloc.c | 15 --- > > > xen/include/public/memory.h | 4 ++-- > > > xen/include/xen/sched.h | 24 ++-- > > > 15 files changed, 60 insertions(+), 46 deletions(-) > > > > From this, with the comment you add next to the struct field, and > > with your reply yesterday, what about the uses in > > - arch/arm/arm64/domctl.c:switch_mode(), > > TBH I'm not sure with that one. It looks to me like it needs to check > whether the domain has *any* memory assigned. Perhaps checking page_list > would be more appropriate. Perhaps Julien can comment? > Having chatted to Julien, the aim of checking tot_pages here is to test whether the domain is newly created, in which case using domain_tot_pages() is the appropriate thing to do. Paul ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v8 2/4] add a domain_tot_pages() helper function
> -Original Message- > From: Jan Beulich > Sent: 31 January 2020 12:53 > To: Durrant, Paul > Cc: xen-devel@lists.xenproject.org; Andrew Cooper > ; Wei Liu ; Roger Pau Monné > ; George Dunlap ; Ian > Jackson ; Julien Grall ; Konrad > Rzeszutek Wilk ; Stefano Stabellini > ; Tim Deegan > Subject: Re: [PATCH v8 2/4] add a domain_tot_pages() helper function > > On 30.01.2020 15:57, Paul Durrant wrote: > > v8: > > - New in v8 > > --- > > xen/arch/x86/domain.c | 2 +- > > xen/arch/x86/mm.c | 6 +++--- > > xen/arch/x86/mm/p2m-pod.c | 10 +- > > xen/arch/x86/mm/shadow/common.c | 2 +- > > xen/arch/x86/msi.c | 2 +- > > xen/arch/x86/numa.c | 2 +- > > xen/arch/x86/pv/dom0_build.c| 25 + > > xen/arch/x86/pv/domain.c| 2 +- > > xen/common/domctl.c | 2 +- > > xen/common/grant_table.c| 4 ++-- > > xen/common/keyhandler.c | 2 +- > > xen/common/memory.c | 4 ++-- > > xen/common/page_alloc.c | 15 --- > > xen/include/public/memory.h | 4 ++-- > > xen/include/xen/sched.h | 24 ++-- > > 15 files changed, 60 insertions(+), 46 deletions(-) > > From this, with the comment you add next to the struct field, and > with your reply yesterday, what about the uses in > - arch/arm/arm64/domctl.c:switch_mode(), TBH I'm not sure with that one. It looks to me like it needs to check whether the domain has *any* memory assigned. Perhaps checking page_list would be more appropriate. Perhaps Julien can comment? > - arch/x86/pv/shim.c:pv_shim_setup_dom(), > - arch/x86/pv/shim.c:write_start_info()? It looks like both of those should be changed. > > > --- a/xen/arch/x86/mm.c > > +++ b/xen/arch/x86/mm.c > > @@ -4194,8 +4194,8 @@ long do_mmu_update( > > * - page caching attributes cleaned up > > * - removed from the domain's page_list > > * > > - * If MEMF_no_refcount is not set, the domain's tot_pages will be > > - * adjusted. If this results in the page count falling to 0, > > + * If MEMF_no_refcount is not set, the domain_adjust_tot_pages() will > > + * be called. If this results in the page count falling to 0, > > * put_domain() will be called. > > If you fiddle with this comment, please also drop the "the" ahead > of the function name. Unless you as a native speaker would confirm > it's appropriate there (it doesn't seem so to me). Of course I > also wouldn't mind leaving this untouched altogether. > Ok, I'll drop that hunk. > > --- a/xen/common/memory.c > > +++ b/xen/common/memory.c > > @@ -717,7 +717,7 @@ static long > memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg) > > > > /* > > * Pages in in_chunk_list is stolen without > > - * decreasing the tot_pages. If the domain is dying > when > > + * decreasing domain_tot_pages(). If the domain is > dying when > > I'd leave this comment alone, or at least not use the function > name. Maybe do as you did in the public header? > OK I'll leave this alone too. > > --- a/xen/include/xen/sched.h > > +++ b/xen/include/xen/sched.h > > @@ -364,12 +364,18 @@ struct domain > > spinlock_t page_alloc_lock; /* protects all the following > fields */ > > struct page_list_head page_list; /* linked list */ > > struct page_list_head xenpage_list; /* linked list (size > xenheap_pages) */ > > -unsigned int tot_pages; /* number of pages currently > possesed */ > > -unsigned int xenheap_pages; /* # pages allocated from Xen > heap*/ > > -unsigned int outstanding_pages; /* pages claimed but not > possessed */ > > -unsigned int max_pages; /* maximum value for tot_pages > */ > > -atomic_t shr_pages; /* number of shared pages > */ > > -atomic_t paged_pages; /* number of paged-out pages > */ > > + > > +/* > > + * This field should only be directly accessed by > domain_adjust_tot_pages() > > + * and the domain_tot_pages() helper function defined below. > > + */ > > +unsigned int tot_pages; > > If the three missing ones got taken care of, with there being arguments > both pro and con your change to dump_pageframe_info(), I'd be okay with > it getting changed as you do, to not render this comment partially > wrong. Ok. Paul > > Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v8 2/4] add a domain_tot_pages() helper function
On 30.01.2020 15:57, Paul Durrant wrote: > v8: > - New in v8 > --- > xen/arch/x86/domain.c | 2 +- > xen/arch/x86/mm.c | 6 +++--- > xen/arch/x86/mm/p2m-pod.c | 10 +- > xen/arch/x86/mm/shadow/common.c | 2 +- > xen/arch/x86/msi.c | 2 +- > xen/arch/x86/numa.c | 2 +- > xen/arch/x86/pv/dom0_build.c| 25 + > xen/arch/x86/pv/domain.c| 2 +- > xen/common/domctl.c | 2 +- > xen/common/grant_table.c| 4 ++-- > xen/common/keyhandler.c | 2 +- > xen/common/memory.c | 4 ++-- > xen/common/page_alloc.c | 15 --- > xen/include/public/memory.h | 4 ++-- > xen/include/xen/sched.h | 24 ++-- > 15 files changed, 60 insertions(+), 46 deletions(-) From this, with the comment you add next to the struct field, and with your reply yesterday, what about the uses in - arch/arm/arm64/domctl.c:switch_mode(), - arch/x86/pv/shim.c:pv_shim_setup_dom(), - arch/x86/pv/shim.c:write_start_info()? > --- a/xen/arch/x86/mm.c > +++ b/xen/arch/x86/mm.c > @@ -4194,8 +4194,8 @@ long do_mmu_update( > * - page caching attributes cleaned up > * - removed from the domain's page_list > * > - * If MEMF_no_refcount is not set, the domain's tot_pages will be > - * adjusted. If this results in the page count falling to 0, > + * If MEMF_no_refcount is not set, the domain_adjust_tot_pages() will > + * be called. If this results in the page count falling to 0, > * put_domain() will be called. If you fiddle with this comment, please also drop the "the" ahead of the function name. Unless you as a native speaker would confirm it's appropriate there (it doesn't seem so to me). Of course I also wouldn't mind leaving this untouched altogether. > --- a/xen/common/memory.c > +++ b/xen/common/memory.c > @@ -717,7 +717,7 @@ static long > memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg) > > /* > * Pages in in_chunk_list is stolen without > - * decreasing the tot_pages. If the domain is dying when > + * decreasing domain_tot_pages(). If the domain is dying when I'd leave this comment alone, or at least not use the function name. Maybe do as you did in the public header? > --- a/xen/include/xen/sched.h > +++ b/xen/include/xen/sched.h > @@ -364,12 +364,18 @@ struct domain > spinlock_t page_alloc_lock; /* protects all the following fields > */ > struct page_list_head page_list; /* linked list */ > struct page_list_head xenpage_list; /* linked list (size xenheap_pages) > */ > -unsigned int tot_pages; /* number of pages currently possesed > */ > -unsigned int xenheap_pages; /* # pages allocated from Xen heap > */ > -unsigned int outstanding_pages; /* pages claimed but not possessed > */ > -unsigned int max_pages; /* maximum value for tot_pages > */ > -atomic_t shr_pages; /* number of shared pages > */ > -atomic_t paged_pages; /* number of paged-out pages > */ > + > +/* > + * This field should only be directly accessed by > domain_adjust_tot_pages() > + * and the domain_tot_pages() helper function defined below. > + */ > +unsigned int tot_pages; If the three missing ones got taken care of, with there being arguments both pro and con your change to dump_pageframe_info(), I'd be okay with it getting changed as you do, to not render this comment partially wrong. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v8 2/4] add a domain_tot_pages() helper function
On 30.01.2020 17:32, Durrant, Paul wrote:
>> -Original Message-
>> From: Jan Beulich
>> Sent: 30 January 2020 16:29
>> To: Durrant, Paul
>> Cc: xen-devel@lists.xenproject.org; Andrew Cooper
>> ; Wei Liu ; Roger Pau Monné
>> ; George Dunlap ; Ian
>> Jackson ; Julien Grall ; Konrad
>> Rzeszutek Wilk ; Stefano Stabellini
>> ; Tim Deegan
>> Subject: Re: [PATCH v8 2/4] add a domain_tot_pages() helper function
>>
>> On 30.01.2020 15:57, Paul Durrant wrote:
>>> --- a/xen/arch/x86/domain.c
>>> +++ b/xen/arch/x86/domain.c
>>> @@ -218,7 +218,7 @@ void dump_pageframe_info(struct domain *d)
>>>
>>> printk("Memory pages belonging to domain %u:\n", d->domain_id);
>>>
>>> -if ( d->tot_pages >= 10 && d->is_dying < DOMDYING_dead )
>>> +if ( domain_tot_pages(d) >= 10 && d->is_dying < DOMDYING_dead )
>>
>> Before I go any further - are you simply replacing _all_
>> ->tot_pages uses by the new helper?
>
> Basically, apart from domain_adjust_tot_pages(), yes.
>
>> In the case here, for
>> example, I don't think this is what we want.
>>
>
> Why not? I would have thought any 'extra' pages would always be of interest.
Could be hundreds or thousands in the future. As long as it's in
reality just one, perhaps it indeed doesn't matter much. I'll
take a closer look tomorrow, to see if there are other places
where the adjusted count would better not be used. From my
partial audit in the morning I seem to recall that there were
both kinds of situations...
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v8 2/4] add a domain_tot_pages() helper function
> -Original Message-
> From: Jan Beulich
> Sent: 30 January 2020 16:29
> To: Durrant, Paul
> Cc: xen-devel@lists.xenproject.org; Andrew Cooper
> ; Wei Liu ; Roger Pau Monné
> ; George Dunlap ; Ian
> Jackson ; Julien Grall ; Konrad
> Rzeszutek Wilk ; Stefano Stabellini
> ; Tim Deegan
> Subject: Re: [PATCH v8 2/4] add a domain_tot_pages() helper function
>
> On 30.01.2020 15:57, Paul Durrant wrote:
> > --- a/xen/arch/x86/domain.c
> > +++ b/xen/arch/x86/domain.c
> > @@ -218,7 +218,7 @@ void dump_pageframe_info(struct domain *d)
> >
> > printk("Memory pages belonging to domain %u:\n", d->domain_id);
> >
> > -if ( d->tot_pages >= 10 && d->is_dying < DOMDYING_dead )
> > +if ( domain_tot_pages(d) >= 10 && d->is_dying < DOMDYING_dead )
>
> Before I go any further - are you simply replacing _all_
> ->tot_pages uses by the new helper?
Basically, apart from domain_adjust_tot_pages(), yes.
> In the case here, for
> example, I don't think this is what we want.
>
Why not? I would have thought any 'extra' pages would always be of interest.
Paul
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v8 2/4] add a domain_tot_pages() helper function
On 30.01.2020 15:57, Paul Durrant wrote:
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -218,7 +218,7 @@ void dump_pageframe_info(struct domain *d)
>
> printk("Memory pages belonging to domain %u:\n", d->domain_id);
>
> -if ( d->tot_pages >= 10 && d->is_dying < DOMDYING_dead )
> +if ( domain_tot_pages(d) >= 10 && d->is_dying < DOMDYING_dead )
Before I go any further - are you simply replacing _all_
->tot_pages uses by the new helper? In the case here, for
example, I don't think this is what we want.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
