DPDK patches and discussions
 help / color / mirror / Atom feed
From: David Marchand <david.marchand@redhat.com>
To: Maxime Coquelin <maxime.coquelin@redhat.com>
Cc: dev@dpdk.org, mkp@redhat.com, chenbo.xia@intel.com
Subject: Re: [PATCH v2] vhost: fix madvise IOTLB entries pages overlap check
Date: Thu, 16 Mar 2023 09:13:38 +0100	[thread overview]
Message-ID: <CAJFAV8zEKADVEQniRk0JpYxsYKpP9hyKQN9cO6ARkTuOj4pPUQ@mail.gmail.com> (raw)
In-Reply-To: <20230315114010.444005-1-maxime.coquelin@redhat.com>

Hello Maxime,

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?

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.

>
> Fixes: dea092d0addb ("vhost: fix madvise arguments alignment")
>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>


-- 
David Marchand


  parent reply	other threads:[~2023-03-16  8:13 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-03-15 11:40 Maxime Coquelin
2023-03-16  1:57 ` Xia, Chenbo
2023-03-16  8:13 ` David Marchand [this message]
2023-03-16  8:38   ` Maxime Coquelin
2023-03-16  8:52     ` David Marchand
2023-03-16 14:45       ` Maxime Coquelin
2023-03-16 14:45 ` Maxime Coquelin

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=CAJFAV8zEKADVEQniRk0JpYxsYKpP9hyKQN9cO6ARkTuOj4pPUQ@mail.gmail.com \
    --to=david.marchand@redhat.com \
    --cc=chenbo.xia@intel.com \
    --cc=dev@dpdk.org \
    --cc=maxime.coquelin@redhat.com \
    --cc=mkp@redhat.com \
    /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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).