Received: by 2002:a25:5b86:0:0:0:0:0 with SMTP id p128csp945387ybb; Thu, 28 Mar 2019 15:42:50 -0700 (PDT) X-Google-Smtp-Source: APXvYqyMh03LU6hsdTWn/ocBJuS+V6Etb+Me1yc3kA06MDjnRC6GwUS8MOjHnx58dveOC1F3SBVU X-Received: by 2002:a17:902:8d8b:: with SMTP id v11mr46200268plo.241.1553812970092; Thu, 28 Mar 2019 15:42:50 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1553812970; cv=none; d=google.com; s=arc-20160816; b=hbZeN2QG7VpKpvk+V49n/bOAKnRY0fLJhsdjPlT9FiW8FASEf+8SW1z58IS71Eq7Iv 6JStbA9iXkMVYOmZHMX+7WD5h8IHqfulOmfpLXcWxlN7c1asO5rpRf7e7hHkgw6CAeyp x1eEN/GkaYgsRScpTgHITFR1ikYyMz89rEj8tNJ0YmbiD64HtSFO8b2mafsPAKcM932b sspXmukiBomYxa2Myyux7mDnNz6PfvIocfinWn5RtFOYWZdS8JUo8En0aBIApjXM859L dxt24SxJXRGvbBqANs4IxHltdT9D9TEMuNcbKFSRIY4khrKIohmt5TXb1/okuZiKzRyy Udkw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:dkim-signature:content-transfer-encoding :content-language:in-reply-to:mime-version:user-agent:date :message-id:from:references:cc:to:subject; bh=OfUK1aEAY6Zsl63UJUEUMpkH+P62LWdbOYog0WT8rz4=; b=J1zFUQAQTt6izVyb1B4Eh2tkbpSSGteYmzX/SizOhbGajRL0zppVDY2ib4s7Isxz2k UCM/UYZLtASfEgXJeZgwZaCoZ7W7ubJqI4nEBSH9+2jdqAkqZCbUoEwK1kZhcaC5ombY 9iHduqw13C9lqx4G4h8l7oGwa7HQGnuwa/z3tStPDhQvJCPYQwWCNGR4Va+xmLEwZBVU bv7Jnkyl97HmaXKPIuvnOkgtn1SGUSUfmQetTCMfPpqhhX8sE1cERgMuIerUKwGknSM6 YzVl0gtZqqvGfi7zPUK7O0r1r3ejpXw74+TrNE3hKWa9NdNzD7GJ9mChxaMowhC95EJN afLg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@nvidia.com header.s=n1 header.b=M5UHoEjg; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=nvidia.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id n28si210927pfi.109.2019.03.28.15.42.33; Thu, 28 Mar 2019 15:42:50 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@nvidia.com header.s=n1 header.b=M5UHoEjg; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=nvidia.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727963AbfC1Wkr (ORCPT + 99 others); Thu, 28 Mar 2019 18:40:47 -0400 Received: from hqemgate16.nvidia.com ([216.228.121.65]:16050 "EHLO hqemgate16.nvidia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726506AbfC1Wkq (ORCPT ); Thu, 28 Mar 2019 18:40:46 -0400 Received: from hqpgpgate101.nvidia.com (Not Verified[216.228.121.13]) by hqemgate16.nvidia.com (using TLS: TLSv1.2, DES-CBC3-SHA) id ; Thu, 28 Mar 2019 15:40:42 -0700 Received: from hqmail.nvidia.com ([172.20.161.6]) by hqpgpgate101.nvidia.com (PGP Universal service); Thu, 28 Mar 2019 15:40:44 -0700 X-PGP-Universal: processed; by hqpgpgate101.nvidia.com on Thu, 28 Mar 2019 15:40:44 -0700 Received: from [10.110.48.28] (172.20.13.39) by HQMAIL101.nvidia.com (172.20.187.10) with Microsoft SMTP Server (TLS) id 15.0.1473.3; Thu, 28 Mar 2019 22:40:42 +0000 Subject: Re: [PATCH v2 07/11] mm/hmm: add default fault flags to avoid the need to pre-fill pfns arrays. To: Jerome Glisse CC: , , Andrew Morton , Dan Williams References: <20190325144011.10560-1-jglisse@redhat.com> <20190325144011.10560-8-jglisse@redhat.com> <2f790427-ea87-b41e-b386-820ccdb7dd38@nvidia.com> <20190328221203.GF13560@redhat.com> <555ad864-d1f9-f513-9666-0d3d05dbb85d@nvidia.com> <20190328223153.GG13560@redhat.com> From: John Hubbard X-Nvconfidentiality: public Message-ID: <768f56f5-8019-06df-2c5a-b4187deaac59@nvidia.com> Date: Thu, 28 Mar 2019 15:40:42 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.5.3 MIME-Version: 1.0 In-Reply-To: <20190328223153.GG13560@redhat.com> X-Originating-IP: [172.20.13.39] X-ClientProxiedBy: HQMAIL106.nvidia.com (172.18.146.12) To HQMAIL101.nvidia.com (172.20.187.10) Content-Type: text/plain; charset="utf-8" Content-Language: en-US-large Content-Transfer-Encoding: quoted-printable DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nvidia.com; s=n1; t=1553812842; bh=OfUK1aEAY6Zsl63UJUEUMpkH+P62LWdbOYog0WT8rz4=; h=X-PGP-Universal:Subject:To:CC:References:From:X-Nvconfidentiality: Message-ID:Date:User-Agent:MIME-Version:In-Reply-To: X-Originating-IP:X-ClientProxiedBy:Content-Type:Content-Language: Content-Transfer-Encoding; b=M5UHoEjg/pye8s/V9BWWTr2SPNaB4/v7N0AgemPbujCD1AtnpUEl8SGpHHF2a1mwD PrsbQRvTCMc36QgfskTOSOkCLtJGzv7FKtcJLpcJItFg3qXDBug/4g5RVJLyfiDk+M d4rN0QGvmDgJ+6UNqgsNavS83Jk4AXgUatCETkGMN9i2aZL0+etcuJ7K2mlW1BbapK MUM68sgzd4jVhRop9knpusl2eXTBwVEc1EaGODI/fA4JkqmiYmQZQ+oxWu/R9vCESG DOkphyrEySgLcm8pKg1Lr8gNOxyfTQVIYFa3LD1fp+HZ4GoRpN8VRpM/Gxy9XP1QHn a6nExVPulspbw== Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 3/28/19 3:31 PM, Jerome Glisse wrote: > On Thu, Mar 28, 2019 at 03:19:06PM -0700, John Hubbard wrote: >> On 3/28/19 3:12 PM, Jerome Glisse wrote: >>> On Thu, Mar 28, 2019 at 02:59:50PM -0700, John Hubbard wrote: >>>> On 3/25/19 7:40 AM, jglisse@redhat.com wrote: >>>>> From: J=C3=A9r=C3=B4me Glisse >>>>> >>>>> The HMM mirror API can be use in two fashions. The first one where th= e HMM >>>>> user coalesce multiple page faults into one request and set flags per= pfns >>>>> for of those faults. The second one where the HMM user want to pre-fa= ult a >>>>> range with specific flags. For the latter one it is a waste to have t= he user >>>>> pre-fill the pfn arrays with a default flags value. >>>>> >>>>> This patch adds a default flags value allowing user to set them for a= range >>>>> without having to pre-fill the pfn array. >>>>> >>>>> Signed-off-by: J=C3=A9r=C3=B4me Glisse >>>>> Reviewed-by: Ralph Campbell >>>>> Cc: Andrew Morton >>>>> Cc: John Hubbard >>>>> Cc: Dan Williams >>>>> --- >>>>> include/linux/hmm.h | 7 +++++++ >>>>> mm/hmm.c | 12 ++++++++++++ >>>>> 2 files changed, 19 insertions(+) >>>>> >>>>> diff --git a/include/linux/hmm.h b/include/linux/hmm.h >>>>> index 79671036cb5f..13bc2c72f791 100644 >>>>> --- a/include/linux/hmm.h >>>>> +++ b/include/linux/hmm.h >>>>> @@ -165,6 +165,8 @@ enum hmm_pfn_value_e { >>>>> * @pfns: array of pfns (big enough for the range) >>>>> * @flags: pfn flags to match device driver page table >>>>> * @values: pfn value for some special case (none, special, error, .= ..) >>>>> + * @default_flags: default flags for the range (write, read, ...) >>>>> + * @pfn_flags_mask: allows to mask pfn flags so that only default_fl= ags matter >>>>> * @pfn_shifts: pfn shift value (should be <=3D PAGE_SHIFT) >>>>> * @valid: pfns array did not change since it has been fill by an HM= M function >>>>> */ >>>>> @@ -177,6 +179,8 @@ struct hmm_range { >>>>> uint64_t *pfns; >>>>> const uint64_t *flags; >>>>> const uint64_t *values; >>>>> + uint64_t default_flags; >>>>> + uint64_t pfn_flags_mask; >>>>> uint8_t pfn_shift; >>>>> bool valid; >>>>> }; >>>>> @@ -521,6 +525,9 @@ static inline int hmm_vma_fault(struct hmm_range = *range, bool block) >>>>> { >>>>> long ret; >>>>> =20 >>>>> + range->default_flags =3D 0; >>>>> + range->pfn_flags_mask =3D -1UL; >>>> >>>> Hi Jerome, >>>> >>>> This is nice to have. Let's constrain it a little bit more, though: th= e pfn_flags_mask >>>> definitely does not need to be a run time value. And we want some assu= rance that >>>> the mask is=20 >>>> a) large enough for the flags, and >>>> b) small enough to avoid overrunning the pfns field. >>>> >>>> Those are less certain with a run-time struct field, and more obviousl= y correct with >>>> something like, approximately: >>>> >>>> #define PFN_FLAGS_MASK 0xFFFF >>>> >>>> or something. >>>> >>>> In other words, this is more flexibility than we need--just a touch to= o much, >>>> IMHO. >>> >>> This mirror the fact that flags are provided as an array and some devic= es use >>> the top bits for flags (read, write, ...). So here it is the safe defau= lt to >>> set it to -1. If the caller want to leverage this optimization it can o= verride >>> the default_flags value. >>> >> >> Optimization? OK, now I'm a bit lost. Maybe this is another place where = I could >> use a peek at the calling code. The only flags I've seen so far use the = bottom >> 3 bits and that's it.=20 >> >> Maybe comments here? >> >>>> >>>>> + >>>>> ret =3D hmm_range_register(range, range->vma->vm_mm, >>>>> range->start, range->end); >>>>> if (ret) >>>>> diff --git a/mm/hmm.c b/mm/hmm.c >>>>> index fa9498eeb9b6..4fe88a196d17 100644 >>>>> --- a/mm/hmm.c >>>>> +++ b/mm/hmm.c >>>>> @@ -415,6 +415,18 @@ static inline void hmm_pte_need_fault(const stru= ct hmm_vma_walk *hmm_vma_walk, >>>>> if (!hmm_vma_walk->fault) >>>>> return; >>>>> =20 >>>>> + /* >>>>> + * So we not only consider the individual per page request we also >>>>> + * consider the default flags requested for the range. The API can >>>>> + * be use in 2 fashions. The first one where the HMM user coalesce >>>>> + * multiple page fault into one request and set flags per pfns for >>>>> + * of those faults. The second one where the HMM user want to pre- >>>>> + * fault a range with specific flags. For the latter one it is a >>>>> + * waste to have the user pre-fill the pfn arrays with a default >>>>> + * flags value. >>>>> + */ >>>>> + pfns =3D (pfns & range->pfn_flags_mask) | range->default_flags; >>>> >>>> Need to verify that the mask isn't too large or too small. >>> >>> I need to check agin but default flag is anded somewhere to limit >>> the bit to the one we expect. >> >> Right, but in general, the *mask* could be wrong. It would be nice to ha= ve >> an assert, and/or a comment, or something to verify the mask is proper. >> >> Really, a hardcoded mask is simple and correct--unless it *definitely* m= ust >> vary for devices of course. >=20 > Ok so re-read the code and it is correct. The helper for compatibility wi= th > old API (so that i do not break nouveau upstream code) initialize those t= o > the safe default ie: >=20 > range->default_flags =3D 0; > range->pfn_flags_mask =3D -1; >=20 > Which means that in the above comment we are in the case where it is the > individual entry within the pfn array that will determine if we fault or > not. >=20 > Driver using the new API can either use this safe default or use the > second case in the above comment and set default_flags to something > else than 0. >=20 > Note that those default_flags are not set in the final result they are > use to determine if we need to do a page fault. For instance if you set > the write bit in the default flags then the pfns computed above will > have the write bit set and when we compare with the CPU pte if the CPU > pte do not have the write bit set then we will fault. What matter is > that in this case the value within the pfns array is totaly pointless > ie we do not care what it is, it will not affect the decission ie the > decision is made by looking at the default flags. >=20 > Hope this clarify thing. You can look at the ODP patch to see how it > is use: >=20 > https://cgit.freedesktop.org/~glisse/linux/commit/?h=3Dhmm-odp-v2&id=3Dee= bd4f3095290a16ebc03182e2d3ab5dfa7b05ec >=20 Hi Jerome, I think you're talking about flags, but I'm talking about the mask. The=20 above link doesn't appear to use the pfn_flags_mask, and the default_flags= =20 that it uses are still in the same lower 3 bits: +static uint64_t odp_hmm_flags[HMM_PFN_FLAG_MAX] =3D { + ODP_READ_BIT, /* HMM_PFN_VALID */ + ODP_WRITE_BIT, /* HMM_PFN_WRITE */ + ODP_DEVICE_BIT, /* HMM_PFN_DEVICE_PRIVATE */ +}; So I still don't see why we need the flexibility of a full 0xFFFFFFFFFFFFFF= FF mask, that is *also* runtime changeable.=20 thanks, --=20 John Hubbard NVIDIAr