Re: [PATCH v2] vhost: fix madvise IOTLB entries pages overlap check

[David Marchand <david.marchand@redhat.com>]

On Thu, Mar 16, 2023 at 9:38 AM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
> On 3/16/23 09:13, David Marchand wrote:
> > On Wed, Mar 15, 2023 at 12:40 PM Maxime Coquelin
> > <maxime.coquelin@redhat.com> wrote:
> >>
> >> At removal time, when testing whether the IOTLB entry has
> >> shared pages with the previous and next entries in the
> >> cache, it checks whether the start address of the entry to
> >> be removed is on the same page as the start address of the
> >> next entry in the cache.
> >>
> >> This is not correct, as an entry could cover several page
> >> so the end address of the entry to be remove should be
> >> used. This patch address this issue.
> >
> > I'm trying to understand the logic, so I needed to write this down :-).
> >
> > Let's imagine the cache contained 3 nodes, "prev", "node" and "next".
> > All those nodes (in this example) do not start or end on a page boundary.
> > Prior to touching those entries, all pages of the nodes are marked as DODUMP.
> >
> > "prev" spans over two pages, "a" and "b".
> > "node" spans over three pages, "b", "c" and "d".
> > "next" spans over two pages, "d" and "e".
> >
> > IOW, "prev" and "node" are sharing the "b" page.
> > IOW, "node" and "next" are sharing the "d" page.
> >
> > Something like (better displayed with fixed-width chars):
> >     prev      node      next
> >    <----> <----------> <---->
> > |  a  |  b  |  c  |  d  |  e  |
> >
> >
> >
> > Previous to this fix, since we were testing the first page of each
> > node, it resulted in page "b" being marked as DONTDUMP, while it was
> > still in use for "prev".
> > And for the same reason, page "d" would be marked as DONTDUMP too.
> >
> > After this fix, all pages are left with DODUMP.
> >
> > Is my understanding correct?
>
> It is correct, that's the other bug I mentioned you yesterday.

Probably, but I did not catch it at the time :-).


> I should have mentioned it in the commit log.
>
> > If so, there is still one (minor?) issue to look into: we leave the
> > "c" page as DODUMP while it won't contain useful information.
>
> In my opinion, this is a minor issue as it indeed keeps some pages as
> DODUMP while they should be set as DONTDUMP. And the changes required to
> fix it seems too big at the stage of the release, and I would prefer to
> fix it in v23.07 to be on the safe side.
>
> It is the opposite for this fix, which is trivial and prevent missing
> pages in the coredump.
>
> Does that sounds good to you? I can add a note in the commit message if
> you want.

Ok for me with a note yes.
This code is not trivial :-).


Thanks.


-- 
David Marchand