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 B8D0741EAC; Thu, 16 Mar 2023 09:38:46 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 93BFF40FDF; Thu, 16 Mar 2023 09:38:46 +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 03DFB40EF1 for ; Thu, 16 Mar 2023 09:38:44 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1678955924; 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=pyhPvs8EnzRZz6ktDNmQJcUtWE9uh/IHlsHWfRkr44g=; b=WInCQn5Y8Jurggidqn4twF2/0SOOV74AgBcg2khqlfPcS8zb4e2lf2U5Dz/RhoWSzGlUUU CGqlP5Uk5LTHhM78MdTyyxzGHHozAbPPtrPQjGUJKPI4mFOVVLhbauQplvZFbneFf7p0y6 6PxoLna6scp6W+IolxVoQaGwReXByak= Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-439-i15CB_uwOiCbKPepq-XCxw-1; Thu, 16 Mar 2023 04:38:41 -0400 X-MC-Unique: i15CB_uwOiCbKPepq-XCxw-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.rdu2.redhat.com [10.11.54.2]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id C77891C05AC4; Thu, 16 Mar 2023 08:38:40 +0000 (UTC) Received: from [10.39.208.23] (unknown [10.39.208.23]) by smtp.corp.redhat.com (Postfix) with ESMTPS id E8EC440C6E67; Thu, 16 Mar 2023 08:38:39 +0000 (UTC) Message-ID: Date: Thu, 16 Mar 2023 09:38:38 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.8.0 To: David Marchand Cc: dev@dpdk.org, mkp@redhat.com, chenbo.xia@intel.com References: <20230315114010.444005-1-maxime.coquelin@redhat.com> From: Maxime Coquelin Subject: Re: [PATCH v2] vhost: fix madvise IOTLB entries pages overlap check In-Reply-To: X-Scanned-By: MIMEDefang 3.1 on 10.11.54.2 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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 3/16/23 09:13, David Marchand wrote: > Hello Maxime, > > On Wed, Mar 15, 2023 at 12:40 PM 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 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. 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. Thanks, Maxime >> >> Fixes: dea092d0addb ("vhost: fix madvise arguments alignment") >> >> Signed-off-by: Maxime Coquelin > >