From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id AECC541EAC; Thu, 16 Mar 2023 09:53:10 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 99BA540FDF; Thu, 16 Mar 2023 09:53:10 +0100 (CET) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mails.dpdk.org (Postfix) with ESMTP id AE16240EF1 for ; Thu, 16 Mar 2023 09:53:08 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1678956788; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=TcIgHqMzBcysxjD2ZplYe07TZ8+3fyj9VUxt1G904jo=; b=Li7SP7DLhCj6bNVANfs4ZvReWgfqwHlnprPm/yu/C/754TfPHi41USJX/ncNgeQgZUIht/ Nc8fkcAFA+FPToGNlES+9y3YbqfVGfLsq1HoSa8t5zPqHnpLMZqFv18FY7U1Viq4gqXBh6 QTEBrqwQNQLwXScXiU/qfqq2Gk9/ZPI= Received: from mail-pf1-f198.google.com (mail-pf1-f198.google.com [209.85.210.198]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-591-pgQs1IKTN7mfIlU10bfU0w-1; Thu, 16 Mar 2023 04:53:05 -0400 X-MC-Unique: pgQs1IKTN7mfIlU10bfU0w-1 Received: by mail-pf1-f198.google.com with SMTP id s11-20020a056a00194b00b0062586c7a2acso772870pfk.23 for ; Thu, 16 Mar 2023 01:53:05 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1678956785; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=TcIgHqMzBcysxjD2ZplYe07TZ8+3fyj9VUxt1G904jo=; b=5OsDR/jI08QJcqwjoBceiR3Gg8MZC0hT242XFHYrnf1GFZaHVXFSoB3oZ1hrEkOUX0 kgvCT3CUyV89Bk4Wdlq/HynPVUDg+BPXaHfDYf7r0WZ+PZs675rNsusNl0L12E6D/+oq 3qdiWKyZ/b7+72To6ogCwDBIhUPtOgNx35YuGedMPVKPyE7p46CH+V3+PZFDSsLt5lUY EXWaL3JAqJx+dr5HoqkLOhu2nan06G5zYDs29OsQiaSdA1DISCe/7kGsc/+Zf/cXk6JC 5hGRL6v0ou1jP51eNG3xa4ViaCgeKMG2kXiiAL9T1rHapMudhVC73TRoXOthsLMgvu5x pO3w== X-Gm-Message-State: AO0yUKUEtGMad7A5khrOmh7SCaZOawPephZAQm0GnuaDH9elUe1QSM+c fiR0unS9NUT/4DtzFAlY7DZK+Q8VONu0R+iRQI6xIWAnqjvG+7BHkZqhtEFbaZy0OwBOeT1n4G4 pHT+Uq7yPHiOPTcv4UOk= X-Received: by 2002:a17:90a:1bcd:b0:23b:316d:2b90 with SMTP id r13-20020a17090a1bcd00b0023b316d2b90mr882595pjr.8.1678956784811; Thu, 16 Mar 2023 01:53:04 -0700 (PDT) X-Google-Smtp-Source: AK7set/fOcFmh+TVIC+ah+XVrV2zv0nZOV6ypn+/WuZMx7MjK0NPTxP7eKrKE3a9YeNcrqDC9VdNcu/m2dh1HTyshm4= X-Received: by 2002:a17:90a:1bcd:b0:23b:316d:2b90 with SMTP id r13-20020a17090a1bcd00b0023b316d2b90mr882585pjr.8.1678956784531; Thu, 16 Mar 2023 01:53:04 -0700 (PDT) MIME-Version: 1.0 References: <20230315114010.444005-1-maxime.coquelin@redhat.com> In-Reply-To: From: David Marchand Date: Thu, 16 Mar 2023 09:52:52 +0100 Message-ID: Subject: Re: [PATCH v2] vhost: fix madvise IOTLB entries pages overlap check To: Maxime Coquelin Cc: dev@dpdk.org, mkp@redhat.com, chenbo.xia@intel.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On Thu, Mar 16, 2023 at 9:38=E2=80=AFAM Maxime Coquelin wrote: > On 3/16/23 09:13, David Marchand wrote: > > On Wed, Mar 15, 2023 at 12:40=E2=80=AFPM Maxime Coquelin > > 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 boundar= y. > > Prior to touching those entries, all pages of the nodes are marked as D= ODUMP. > > > > "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. --=20 David Marchand