On Thu, Mar 16, 2023 at 9:38 AM Maxime Coquelin <maxime.coque...@redhat.com> wrote: > On 3/16/23 09:13, David Marchand wrote: > > On Wed, Mar 15, 2023 at 12:40 PM Maxime Coquelin > > <maxime.coque...@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
