Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <01fa5b0f-d209-6eb5-9911-b7522eb491d6@intel.com>
Date:   Fri, 23 Sep 2022 10:59:35 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.13.1
Subject: Re: [Intel-wired-lan] [PATCH] ixgbe: Use kmap_local_page in
 ixgbe_check_lbtest_frame()
Content-Language: en-US
To:     "Fabio M. De Francesco" <fmdefrancesco@gmail.com>,
        Ira Weiny <ira.weiny@intel.com>
CC:     Alexander Duyck <alexander.duyck@gmail.com>,
        Jakub Kicinski <kuba@kernel.org>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Netdev <netdev@vger.kernel.org>,
        Maciej Fijalkowski <maciej.fijalkowski@intel.com>,
        Jesper Dangaard Brouer <hawk@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        intel-wired-lan <intel-wired-lan@lists.osuosl.org>,
        Alexander Duyck <alexanderduyck@fb.com>,
        John Fastabend <john.fastabend@gmail.com>,
        Jesse Brandeburg <jesse.brandeburg@intel.com>,
        Alexei Starovoitov <ast@kernel.org>, bpf <bpf@vger.kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        LKML <linux-kernel@vger.kernel.org>,
        Tony Nguyen <anthony.l.nguyen@intel.com>
References: <20220629085836.18042-1-fmdefrancesco@gmail.com>
 <CAKgT0Uf1o+i0qKf7J_xqC3SACRFhiYqyhBeQydgUafB5uFkAvg@mail.gmail.com>
 <22aa8568-7f6e-605e-7219-325795b218b7@intel.com>
 <27280395.gRfpFWEtPU@localhost.localdomain>
From:   Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>
In-Reply-To: <27280395.gRfpFWEtPU@localhost.localdomain>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?Wm1VQU1MNVpacys3Vm85eHFDYjJ3TnhiSElFY1F3SzJqR3Z1NDMxL1JMZElp?=
 =?utf-8?B?U0VKOXhKeTdCQW82dDVIclNrNnAyLzBEWG9ZeDNvVHUzUVFaRTJIOHQ4NE5y?=
 =?utf-8?B?ZnVRbzFsMFJBaVpHU2lqd2l0Q0JsdWI1cWVOMW5SOXhwTG00SkZlOEIrSUpk?=
 =?utf-8?B?RWZMS3B1UFRZYVV6MFo5bi9nQWd5NWNKUnF5T1JWK0tydEdGT1BGY0VnbmJP?=
 =?utf-8?B?YmorUGhiK0ZwRWd2N3QvcFA4dXdDMkRxOWowLzNCcUY3VWZnZFBJS1gwTE42?=
 =?utf-8?B?eU9HbVFOc3JMMzV1a2tRL0NsZldzSDZ4b3lnK3JDekR1c1hLQWFEQmVQeUlo?=
 =?utf-8?B?NjBiVkg3NmVmNldJMWx4NTZ0aFlZNGhyMDdRekUxcDFBdGNxdFdNeUFkMldv?=
 =?utf-8?B?cEpKN3JDRVZleXNaOEkxMndiNVhqSXBsanBtUkpxT01UdTVjMzhrblNtcmFX?=
 =?utf-8?B?ZTFXV0RlMy9ZNjh1UjRsZlNlRDV5WklhempQNWxWVy8xOE53bDhzUWdWNGZu?=
 =?utf-8?B?amU2RUV1WEVDQXU0eGI2bThackF0bk5iVVJOc3JhSitYQmt0NUJRWFBTbjFT?=
 =?utf-8?B?bjRjTlZjeXBLTllOSjVmSENvdDlIazFBSjFaMjgxSnM2VkVoZy84VUIrUzFN?=
 =?utf-8?B?T04zUkVrcTc4c0VzL2lnQWNoNnRjNjh0N0tPL1JrNU5xQ0ZVODN2QTZIeHp0?=
 =?utf-8?B?OEllakxXRFhON1E3a2ZyWHdBeHp3RnVlZjY2VFBXWUlEWU5FMGJ0ckFnQlVP?=
 =?utf-8?B?Sko5RmN5cDREWE5lZWpacEdUSHBUTm5zZVZuSkN3UFQ5ZEJwemRnbmZ2Yzlh?=
 =?utf-8?B?ekUzYU5qM2NseTU5YkZUVDRpL2NQa08vYUJnaU1Qa2pEUEtpMkdsK28xbkNq?=
 =?utf-8?B?aXhEVG4vK0lYMkdzWmtkTEFMUjIrMmF5b0tmL2cvd2FIaGpNNDRnMUtVbWJC?=
 =?utf-8?B?eUZ2LzY5L25zQVJwZHhxczJObVBUdFYxZWlmMTY5NWZqOHpiZ3NiZngvRXR1?=
 =?utf-8?B?YTZWTEJvMndUZGdldENIeFFyanU0TmZHQ0cvbmVlYmJSYWVRaXR3NHVUZ2k1?=
 =?utf-8?B?eEJCUUdwLy9LWTkraUVPbWlCeFdRSWc2UzhhejhhTjVzMkFkdzh0ZWwzL09U?=
 =?utf-8?B?MkQxWUkwYmFFMWxwNjE5YWZSMWpobjlDNFBqZitIdzBYWjhsUGlnVHVZQTRK?=
 =?utf-8?B?TjN2NzQ5cVVBTHFFR2h3bHEwTUVRN3BNcklBWmVkc0owVnhqQ296QUNlSFF1?=
 =?utf-8?B?RDY0OGJVTHdhaXI1clpCMTR3YjRXc0wza3FnTXJpb0dZQURUdU4ydm10Vmt0?=
 =?utf-8?B?MklvekJNZHN5UFIrTC95Y2pwSHRZUFo2cTdmTENGcGxQZFluenJyV0VuTlJL?=
 =?utf-8?B?SjFOWGh4N0FhWXljSjFWeXQ5N1lJM29ScVlOUGZabkdaaDZTV25qS2tyRUFQ?=
 =?utf-8?B?R2RuRDhnMkFwZUVsd1hrbDJyTElaTlJFNUc3TkFrUFU5eHpqbTZkenU3VXEr?=
 =?utf-8?B?a04zYjBsMVRTT2dOZkxzSEZFaUFVUWR3SXljcWtCMzh6d2V3S1V4ZUNETjVL?=
 =?utf-8?B?OFMwRlVvWWRtN3JVNnVocFZEME1Ua2dOb3pIRkVQMEFiRVh2eGFIRHg5T1RB?=
 =?utf-8?B?c1FVZnN2RFEvR1lOMlFIR2xFM0UzbnpTV0tYK0VrRmZmQ1hhOXB4eTRtQUs5?=
 =?utf-8?B?WVQrSlNCUGlYZVQ4eGJzYkhrMklMUVI2MWJvYzZXaHlTdzE1Rjl0UkJoVHJG?=
 =?utf-8?B?ZWZCV0t0RmVDcHZUS2U0dVJNdmtrbmxpR0tvbFVXTWJFc3l4UE93QzNPcnR4?=
 =?utf-8?B?VzN3ZFRQaCszMGxTUUJWWjAzS1JtU1VDNHBJRkRpdXQ1VmtZYU1pUGM0OSt6?=
 =?utf-8?B?bW1FYStFWTNzUDJiME1ocTJQZ0hKNVVXemJyWVIxSFpDMk4yRnUxaFBpYit2?=
 =?utf-8?B?emhyZjdjQnFjSWNPcW9pOFFsN3VsR2xtbCtMSGhOakQwRE1kMU53OEg5NEpW?=
 =?utf-8?B?Z2ExNnh2NWJaTUJ4SmxxeExsWXZyM2RoMEpYb3Bqem5QYmk3Q3I2emNHbHlh?=
 =?utf-8?B?S01XV2ZCS0hyWjdKWWFBKzJMenhoVWZicGc3SnVEZzFGTjR4NHRjTFdhQ0dU?=
 =?utf-8?B?MDJ1aHdvcHVGUTZaVXY1b1BVMnpmVlVuRk5PVUNTZWpMVHJ6V3ZrRGpYZUpj?=
 =?utf-8?Q?UvSW4Q2vtASdsUQMzzSmEWQ=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 8205854c-df68-4e94-fb86-08da9d8d61d9
X-MS-Exchange-CrossTenant-AuthSource: MW3PR11MB4764.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Sep 2022 17:59:38.5533
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: T7erSR0BgLSrgQ4zcbLj+mpuXJ+ptN3sWwKx1gdYNBil7r0S44vh0HDGZDKJOEVSbknnkCMovY58KQIyX/bFFhqVftSEt0G1zaisEwLrLfdCMyu/Ots2keSWUxa//NOB
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW3PR11MB4586
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_MED,SPF_HELO_PASS,SPF_NONE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 9/23/2022 8:05 AM, Fabio M. De Francesco wrote:
> Hi Anirudh,
> 
> On Friday, September 23, 2022 12:38:02 AM CEST Anirudh Venkataramanan wrote:
>> On 9/22/2022 1:58 PM, Alexander Duyck wrote:
>>> On Thu, Sep 22, 2022 at 1:07 PM Anirudh Venkataramanan
>>> <anirudh.venkataramanan@intel.com> wrote:
>>>>
>>>>
>>>> Following Fabio's patches, I made similar changes for e1000/e1000e and
>>>> submitted them to IWL [1].
> 
> I saw your patches and they look good to me. I might comment and probably
> review them, however I prefer to wait for Ira to do that. Furthermore, looking
> again at your patches made me recall that I need to talk with him about
> something that is only indirectly related with you work.
> 
> Please don't rely on older patches of mine as models for your next patches. In
> the last months I changed many things in the way I handle the removal of
> kmap() in favour of a plain page_address() or decide to convert to
> kmap_local_page(). Obviously I'm talking about pages which cannot come from
> ZONE_HIGHMEM.
> 
>>>> Yesterday, Ira Weiny pointed me to some feedback from Dave Hansen on the
>>>> use of page_address() [2]. My understanding of this feedback is that
>>>> it's safer to use kmap_local_page() instead of page_address(), because
>>>> you don't always know how the underlying page was allocated.
> 
> Your understanding of Dave's message is absolutely correct.
> 
>>>> This approach (of using kmap_local_page() instead of page_address())
>>>> makes sense to me. Any reason not to go this way?
> 
>>>> [1]
>>>>
>>>> https://patchwork.ozlabs.org/project/intel-wired-lan/patch/
> 20220919180949.388785-1-anirudh.venkataramanan@intel.com/
>>>>
>>>> https://patchwork.ozlabs.org/project/intel-wired-lan/patch/
> 20220919180949.388785-2-anirudh.venkataramanan@intel.com/
>>>>
>>>> [2]
>>>> https://lore.kernel.org/lkml/5d667258-b58b-3d28-3609-e7914c99b31b@intel.com/
>>>>
>>>> Ani
>>>
>>> For the two patches you referenced the driver is the one allocating
>>> the pages. So in such a case the page_address should be acceptable.
>>> Specifically we are falling into alloc_page(GFP_ATOMIC) which should
>>> fall into the first case that Dave Hansen called out.
>>
>> Right. However, I did run into a case in the chelsio inline crypto
>> driver where it seems like the pages are allocated outside the driver.
>> In such cases, kmap_local_page() would be the right approach, as the
>> driver can't make assumptions on how the page was allocated.
> 
> The mere fact that we are still discussing this particular topic is my only
> fault. I mean that the guidelines about what to do with ZONE_NORMAL or lower
> pages is not enough clear. I'll have to improve that paragraph.
> 
> For now let me tell you what I'm doing whenever I have to decide between a
> conversion  from kmap{,_atomic}() to kmap_local_page() or a kmap() removal  in
> favour of page_address() use.
> 
>> ... and this makes me wonder why not just use kmap_local_page() even in
>> cases where the page allocation was done in the driver. IMO, this is
>> simpler because
>>
>> a) you don't have to care how a page was allocated. kmap_local_page()
>> will create a temporary mapping if required, if not it just becomes a
>> wrapper to page_address().
>>
>> b) should a future patch change the allocation to be from highmem, you
>> don't have to change a bunch of page_address() calls to be
>> kmap_local_page().
> 
> "a" and "b" are good arguments with sound logic. However there are a couple of
> cases that you are not yet considering.
> 
> As my main rule I prefer the use of kmap_local_page() whenever tracking if
> pages can't come from Highmem is complex, especially when allocation is
> performed in other translation units of the same driver or, worse, pages come
> from different subsystems.
> 
> Instead, I don't like to use kmap_local_page() when the allocation is in the
> same function and you see immediately that it cannot come from ZONE_HIGHMEM.
> 
> Sometimes it's so clear that using kmap_local_page() looks silly to me :-)
> For example...
> 
> void *silly_alloc_and_map() {
>           	struct *page;
> 	
> 	page = alloc_page(GFP_KERNEL);
> 	return kmap_local_page(page);
> }
> 
> In this case you know without any effort that the page cannot come from
> ZONE_HIGHMEM. Therefore, why bother with mapping and unmapping (and perhaps
> write a function for unmapping).

That's fair. When I suggested using kmap_local_page() even for 
driver-local page allocations, I was thinking of situations where the 
page allocation and mapping/access happen in different functions in the 
same driver. Not that these are impossible to trace, just takes some 
more time and effort.

> 
> While working on the removals or the conversions of kmap(), I noticed that
> people tend to forget to call kunmap(). We have a limited amount of kmap()
> slots. If the mapping space is fully utilized we'll have the next slot
> available only after reboot or unloading and reloading a module.
> 
> If I recall correctly, with kmap_local_page() we can map a maximum of 16 pages
> per task_struct. Therefore, limits are everywhere and people tend to leak
> resources.
> 
> To summarize: whenever allocation is easily trackable, and pages cannot come
> from ZONE_HIGHMEM, I prefer page_address().

How would you define "easily track-able"? Does it make more sense to 
instead say "if page allocation is module-local and can't come from 
highmem, then use page_address()".

> 
> Honestly, if code is well designed I don't care whether or not within 5 days
> or 10 years decide to change the allocation. I think it's like to refrain from
> deleting unreachable code, variables, partially implemented functions, and so
> on just because one day someone may think to make something useful from those
> things.

(a) is the primary reason to use kmap_local_page(). (b) is a co-traveler.

> 
> Greg K-H taught me that I must see the code as is now and don't speculate
> about possible future scenarios. I agree with him in full :-)
> 
> Very different case where I _need_ page_address() are due to the strict rules
> of nesting mapping and unmapping-mapping. I recall that I spent days on a
> function in Btrfs because I could not map and unmap with the usual Last In -
> First Out (LIFO) rule.

Right, so maybe instead of me saying "use kmap_local_page() everywhere" 
I should have said "kmap_local_page() should be preferred where possible".

To summarize, how about this for a guideline?

- For module-local page allocations that can't come from highmem, using 
page_address() is acceptable.

- For pages that are allocated outside the module but passed to the 
module, use the appropriate kmap() function.

Ani