From: Razvan Cojocaru <rcojocaru@bitdefender.com>
To: Jan Beulich <JBeulich@suse.com>,
George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>,
xen-devel <xen-devel@lists.xenproject.org>,
Wei Liu <wei.liu2@citrix.com>,
george.dunlap@citrix.com, Roger Pau Monne <roger.pau@citrix.com>
Subject: Re: [PATCH V10 4/5] p2m: Always use hostp2m when clipping rangesets
Date: Fri, 30 Nov 2018 23:59:53 +0200 [thread overview]
Message-ID: <b1d92858-ff44-4d2f-a32f-98e5efde9cb2@bitdefender.com> (raw)
In-Reply-To: <5BFFF06B02000078002013BD@prv1-mh.provo.novell.com>
On 11/29/18 3:58 PM, Jan Beulich wrote:
>>>> On 29.11.18 at 14:23, <rcojocaru@bitdefender.com> wrote:
>> On 11/29/18 12:04 PM, Jan Beulich wrote:
>>>>>> On 28.11.18 at 22:56, <rcojocaru@bitdefender.com> wrote:
>>>> Changes since V9:
>>>> - Removed the patch RFC (replaced by a printk(XENLOG_G_WARNING).
>>>> - Reused start and end in change_type_range() and removed the
>>>> intermediary variables range_start and range_end.
>>>> - Added an extra explanation for the if ( start > end ) return;
>>>> code in the comment.
>>>
>>> This last item isn't really taking care of the comments I gave on v9.
>>> The _incoming_ start being larger than the _incoming_ end is
>>> something worth to point out. But you put that check after clipping
>>> end. Furthermore it looks like you continue to break the case
>>> where ->max_mapped_pfn increases subsequently, i.e. you still
>>> don't update the rangeset with the unmodified incoming values.
>>> Or otherwise I would have expected an explanation (as a reply
>>> to my comments, not necessarily by adding to description or
>>> comments of the patch here) why either this is not an issue or I'm
>>> misreading anything.
>>
>> max_mapped_pfn _should_ end up being >= the logdirty range upper bound,
>> since AFAICT the logdirty ranges are tied to ept_set_entry() calls,
>> which always end up calling p2m_altp2m_propagate_change() when they
>> occur on the hostp2m (which in turn calls p2m_set_entry() on the
>> altp2ms, and so on).
>
> Altp2m-s don't matter here at all. My point is that the present,
> unpatched p2m_change_type_range() updates the log-dirty
> ranges with the unclipped [start,end), but calls
> p2m->change_entry_type_range() with a possibly reduced
> range. Any subsequent caller of p2m_is_logdirty_range() may
> thus be mislead if the rangeset update now also used only the
> clipped range.
I've been reading and re-reading the code and I'm still not sure I follow:
973 if ( unlikely(end > p2m->max_mapped_pfn) )
974 {
975 if ( !gfn )
976 {
977 p2m->change_entry_type_global(p2m, ot, nt);
978 gfn = end;
979 }
980 end = p2m->max_mapped_pfn + 1;
end is being clipped here ...
981 }
982 if ( gfn < end )
983 rc = p2m->change_entry_type_range(p2m, ot, nt, gfn, end - 1);
... and the if() above is not an else if(), so if ( unlikely(end >
p2m->max_mapped_pfn) ) we always clip end. What this new patch does in
that regard is just making sure it uses the hostp2m's max_mapped_pfn
instead of the altp2m's.
984 if ( rc )
985 {
986 printk(XENLOG_G_ERR "Error %d changing Dom%d GFNs [%lx,%lx]
from %d to %d\n",
987 rc, d->domain_id, start, end - 1, ot, nt);
988 domain_crash(d);
989 }
990
991 switch ( nt )
992 {
993 case p2m_ram_rw:
994 if ( ot == p2m_ram_logdirty )
995 rc = rangeset_remove_range(p2m->logdirty_ranges, start,
end - 1);
996 break;
997 case p2m_ram_logdirty:
998 if ( ot == p2m_ram_rw )
999 rc = rangeset_add_range(p2m->logdirty_ranges, start,
end - 1);
1000 break;
1001 default:
1002 break;
1003 }
Then above it calls rangeset_remove_range() or rangeset_add_range() with
the clipped end. rangeset_add_range() ASSERT()s that start <= end, so
we've established that if ( start > end ) return; is at least healthy
for that.
I could move the if ( start > end ) return; below the
p2m->change_entry_type_global(p2m, ot, nt); call so that the code uses
the same flow as it does now. But that would only matter for the case
when start == 0 and end < 0 (which is impossible, with end being an
unsigned long).
The current code already checks if ( gfn < end ) (where gfn is start in
the new patch) before calling p2m->change_entry_type_range() (again,
with the clipped end), so in that respect it's not different at all from
the current logic.
In light of all of that, I'm reading your comment to mean that you think
that the current logic is flawed because the actual work inside
p2m_change_type_range() is done on a clipped range - so you'd like to
either have the new patch refrain from clipping anything, or an
explanation as to why this is proper behaviour (and I was wrong to pay
special attention to the if() returning early you've mentioned in your
original review). Am I correct?
Thanks,
Razvan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
next prev parent reply other threads:[~2018-11-30 22:00 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-11-28 21:56 [PATCH V10 0/5] Fix VGA logdirty related display freezes with altp2m Razvan Cojocaru
2018-11-28 21:56 ` [PATCH V10 1/5] x86/p2m: allocate logdirty_ranges for altp2ms Razvan Cojocaru
2018-11-28 21:56 ` [PATCH V10 2/5] x86/p2m: refactor p2m_reset_altp2m() Razvan Cojocaru
2018-11-28 21:56 ` [PATCH V10 3/5] x86/altp2m: fix display frozen when switching to a new view early Razvan Cojocaru
2018-11-28 21:56 ` [PATCH V10 4/5] p2m: Always use hostp2m when clipping rangesets Razvan Cojocaru
2018-11-29 10:04 ` Jan Beulich
2018-11-29 13:23 ` Razvan Cojocaru
2018-11-29 13:58 ` Jan Beulich
2018-11-30 21:59 ` Razvan Cojocaru [this message]
2018-12-03 8:49 ` Jan Beulich
2018-12-04 12:18 ` Razvan Cojocaru
2018-12-04 12:54 ` Jan Beulich
2018-12-04 14:05 ` Razvan Cojocaru
2018-11-28 21:56 ` [PATCH V10 5/5] p2m: change_type_range: Only invalidate mapped gfns Razvan Cojocaru
2018-11-28 22:01 ` Razvan Cojocaru
2018-11-29 10:07 ` Jan Beulich
2018-11-29 11:47 ` Razvan Cojocaru
2018-12-04 3:27 ` [PATCH V10 0/5] Fix VGA logdirty related display freezes with altp2m Tamas K Lengyel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=b1d92858-ff44-4d2f-a32f-98e5efde9cb2@bitdefender.com \
--to=rcojocaru@bitdefender.com \
--cc=George.Dunlap@eu.citrix.com \
--cc=JBeulich@suse.com \
--cc=andrew.cooper3@citrix.com \
--cc=george.dunlap@citrix.com \
--cc=roger.pau@citrix.com \
--cc=wei.liu2@citrix.com \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.