| Message ID | 79ed633d-b0bd-4a7d-a0c6-37a034e1ee96@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] Argo: don't obtain excess page references | expand |
Hi Jan, On 14/02/2024 10:12, Jan Beulich wrote: > find_ring_mfn() already holds a page reference when trying to obtain a > writable type reference. We shouldn't make assumptions on the general > reference count limit being effectively "infinity". Obtain merely a type > ref, re-using the general ref by only dropping the previously acquired > one in the case of an error. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Julien Grall <jgrall@amazon.com> > --- > I further question the log-dirty check there: The present P2M type of a > page doesn't really matter for writing to the page (plus it's stale by > the time it is looked at). Instead I think every write to such a page > needs to be accompanied by a call to paging_mark_dirty(). I agree with that. Cheers,
On 18.02.2024 19:01, Julien Grall wrote: > On 14/02/2024 10:12, Jan Beulich wrote: >> find_ring_mfn() already holds a page reference when trying to obtain a >> writable type reference. We shouldn't make assumptions on the general >> reference count limit being effectively "infinity". Obtain merely a type >> ref, re-using the general ref by only dropping the previously acquired >> one in the case of an error. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Reviewed-by: Julien Grall <jgrall@amazon.com> I'll give it till the end of the week for an ack to arrive (or a substantial objection), and commit some time next week in the absence of any response. Jan
On Wed, Feb 28, 2024 at 6:52 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 18.02.2024 19:01, Julien Grall wrote: > > On 14/02/2024 10:12, Jan Beulich wrote: > >> find_ring_mfn() already holds a page reference when trying to obtain a > >> writable type reference. We shouldn't make assumptions on the general > >> reference count limit being effectively "infinity". Obtain merely a type > >> ref, re-using the general ref by only dropping the previously acquired > >> one in the case of an error. > >> > >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > > > Reviewed-by: Julien Grall <jgrall@amazon.com> > > I'll give it till the end of the week for an ack to arrive (or a substantial > objection), and commit some time next week in the absence of any response. I haven't gotten to this as quickly as I'd hoped for, sorry, but I've given reference to this thread to some of the downstream argo users / developers to see if there can be attention available and I'll be attending the Community Call. Christopher > > Jan >
On Sun, Feb 18, 2024 at 10:01 AM Julien Grall <julien@xen.org> wrote: > > Hi Jan, > > On 14/02/2024 10:12, Jan Beulich wrote: > > find_ring_mfn() already holds a page reference when trying to obtain a > > writable type reference. We shouldn't make assumptions on the general > > reference count limit being effectively "infinity". Obtain merely a type > > ref, re-using the general ref by only dropping the previously acquired > > one in the case of an error. > > > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Reviewed-by: Julien Grall <jgrall@amazon.com> > > > --- > > I further question the log-dirty check there: The present P2M type of a > > page doesn't really matter for writing to the page (plus it's stale by > > the time it is looked at). Instead I think every write to such a page > > needs to be accompanied by a call to paging_mark_dirty(). > > I agree with that. Adding OpenXT mailing list as I have found that I have not had the time available that I had hoped for to spend on reviewing this Argo change, and to provide opportunity for downstream feedback. Link to the posted patch (start of this thread): https://lists.xenproject.org/archives/html/xen-devel/2024-02/msg00858.html Christopher > > Cheers, > > -- > Julien Grall
On Wed, Mar 6, 2024 at 11:38 PM Christopher Clark <christopher.w.clark@gmail.com> wrote: > > On Sun, Feb 18, 2024 at 10:01 AM Julien Grall <julien@xen.org> wrote: > > > > Hi Jan, > > > > On 14/02/2024 10:12, Jan Beulich wrote: > > > find_ring_mfn() already holds a page reference when trying to obtain a > > > writable type reference. We shouldn't make assumptions on the general > > > reference count limit being effectively "infinity". Obtain merely a type > > > ref, re-using the general ref by only dropping the previously acquired > > > one in the case of an error. > > > > > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > > > Reviewed-by: Julien Grall <jgrall@amazon.com> > > > > > --- > > > I further question the log-dirty check there: The present P2M type of a > > > page doesn't really matter for writing to the page (plus it's stale by > > > the time it is looked at). Instead I think every write to such a page > > > needs to be accompanied by a call to paging_mark_dirty(). > > > > I agree with that. > > Adding OpenXT mailing list as I have found that I have not had the > time available that I had hoped for to spend on reviewing this Argo > change, and to provide opportunity for downstream feedback. > > Link to the posted patch (start of this thread): > https://lists.xenproject.org/archives/html/xen-devel/2024-02/msg00858.html Could we add some more designated reviewers / maintainers to the Argo code to help spread the load a bit? -George
--- a/xen/common/argo.c +++ b/xen/common/argo.c @@ -1426,7 +1426,7 @@ find_ring_mfn(struct domain *d, gfn_t gf switch ( p2mt ) { case p2m_ram_rw: - if ( !get_page_and_type(page, d, PGT_writable_page) ) + if ( !get_page_type(page, PGT_writable_page) ) ret = -EINVAL; break; @@ -1441,7 +1441,8 @@ find_ring_mfn(struct domain *d, gfn_t gf break; } - put_page(page); + if ( unlikely(ret) ) + put_page(page); return ret; }
find_ring_mfn() already holds a page reference when trying to obtain a writable type reference. We shouldn't make assumptions on the general reference count limit being effectively "infinity". Obtain merely a type ref, re-using the general ref by only dropping the previously acquired one in the case of an error. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- I further question the log-dirty check there: The present P2M type of a page doesn't really matter for writing to the page (plus it's stale by the time it is looked at). Instead I think every write to such a page needs to be accompanied by a call to paging_mark_dirty(). --- v2: Re-base.