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: <f32338c8-db1a-ba0c-9254-922d96f2e601@intel.com>
Date:   Fri, 23 Sep 2022 11:50:36 -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:     Alexander Duyck <alexander.duyck@gmail.com>
CC:     "Fabio M. De Francesco" <fmdefrancesco@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>,
        Ira Weiny <ira.weiny@intel.com>,
        LKML <linux-kernel@vger.kernel.org>,
        Tony Nguyen <anthony.l.nguyen@intel.com>
References: <20220629085836.18042-1-fmdefrancesco@gmail.com>
 <2254584.ElGaqSPkdT@opensuse>
 <CAKgT0UfThk3MLcE38wQu5+2Qy7Ld2px-2WJgnD+2xbDsA8iEEw@mail.gmail.com>
 <2834855.e9J7NaK4W3@opensuse>
 <d4e33ca3-92e5-ba30-f103-09d028526ea2@intel.com>
 <CAKgT0Uf1o+i0qKf7J_xqC3SACRFhiYqyhBeQydgUafB5uFkAvg@mail.gmail.com>
 <22aa8568-7f6e-605e-7219-325795b218b7@intel.com>
 <CAKgT0UfU6Hu3XtuJS_vvmeOMDdFcVanieGXRLyVRmPF7+eRjvg@mail.gmail.com>
From:   Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>
In-Reply-To: <CAKgT0UfU6Hu3XtuJS_vvmeOMDdFcVanieGXRLyVRmPF7+eRjvg@mail.gmail.com>
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?ODRTQzY2RzJOSkhkNDgzTE1mRW1BMFBMQlEwbit3ZHNRbXJzQnE1Y3RxWGVS?=
 =?utf-8?B?a1JEdGtsK3cxZE16TDhUUEpjMU1OTkVRd1lDdDFlS0Y5NDJNTnFCdEtBQmFU?=
 =?utf-8?B?ZFpkQm1vcFp2MU1FWHZ4T09XVkxVdzVhaTBLN1d0eXJxNlVKOWNaQ091c2Ux?=
 =?utf-8?B?ZWxzQ1cyWlJvV3dmMktmS2lxRDUvTnc5OXg1WmE5MHNTZzZDK3dUNlFGUXVC?=
 =?utf-8?B?bG8xUTJFbGgvUzdWem1RVkk5cXk3MEl2MEcrQmI2RHRHaGxSOWk4SmF6cUJF?=
 =?utf-8?B?RDFTai9BMjkrKzJ4RlllUk9zRkVZVTUycXdkaWxOdndkcDFWOEF2Q0NlL09Q?=
 =?utf-8?B?dWxwb1o2dE9jTzdJaE1DTDk4SXBHNWRuNFZ6bjRnQ3pJZC9tbDg3YkhhWHVs?=
 =?utf-8?B?QVV2ZGRUQmh6T3ZZSzArMEY1YmJpMEdOQ3pvSjFORUh6RnhPa1lySGJDVG9Z?=
 =?utf-8?B?UStOV05XaklvSFlZR1NDeUFNMyt5MkZROGdqcndhN1gxbzR2M3dubHZHbWxz?=
 =?utf-8?B?MGFHR085RjZCNzAxNzYvdno1OXFHNUt0NkEwak1MQkhidmp0NWk3dEJBVWFM?=
 =?utf-8?B?NnV2Z0x5TG90WGM1VXF2blhLaTk0bEJ2dXRiMkpadzJFQnJET0U4T1MwZ3hw?=
 =?utf-8?B?RXMwZm1aRU9hUm1Uczhtd2VteVBycHJzUTZwc2xmRXVRZFRpSUNPRG5wU25X?=
 =?utf-8?B?cFNHTm45a0s3YlJBTW90L2VoSTBZSDFPcXZ2QldmQWR5QmhhN1ptdUt6TU1p?=
 =?utf-8?B?U2dSQ09DTlRWc1AveE1zZ2czWDM0bnpxMEVRM0pGRGR1L2RMVUU5ckZSY2ZP?=
 =?utf-8?B?Nk4yMnczeVQvMXY1dit6Sldkd2xqTGVaa2JnWG1sRHpUcmV6S1J2SllMVWly?=
 =?utf-8?B?Wk8rc1dzNXFyTENtMFJucEZGVlYveU1zR2ZFMC9xUDlMRFpKSGd1bDlFUnNZ?=
 =?utf-8?B?aWpQZThlQ3VXWTVrbFBvQXc5MnhQTlJ0RlFsY0FkSStnZHNwVEZDa2NnZW9q?=
 =?utf-8?B?Uy9HZ3RBWWgvNjdWUzRicEQ4bUVsazQ2YkpkbXpPMjdnR1R2bnZyMURJY214?=
 =?utf-8?B?dXlvQXhleGZ5SkZrQW1BVFhsOEVOT1hSRTQ4Tk42SW52cE9vK1h0emJLNktx?=
 =?utf-8?B?K0lwc0tnOEtXZURYMERtRmhGSHFkMUE3cnByUnlWbXhvaXpzR3RhTllrcnJp?=
 =?utf-8?B?S1FBUDFkejZva0k0K0NiRVJsMVVLUVZnZVdkNFk1Q0hUS21EVnBSOFh2dG51?=
 =?utf-8?B?YzRRak9iZHlDZ1IvNm13c3dWK0JTdUY5RzFpeGdaR3VGeHI0Q1RRRXNSVndz?=
 =?utf-8?B?dkZrZittRitLMFphS3NuSldOSGhBZnc4WWF1a3lFdVR5UXd4NnJobEx0Vkdo?=
 =?utf-8?B?bXgwdjhkbTU1MnZOVFdWMU8rSlVhOUF2TzFGVkxua0JqRkdqamdidEN3OVFM?=
 =?utf-8?B?YWhTQmwwVUltazJBYjhkVUVNN29RMm82Q1EvNkd1STN5ZVRvbDlmNXVPSE5M?=
 =?utf-8?B?Wkt4ZFdJRWY3OTlaL3pLZmQ4QjUzY2NrdUVCbDFLUzdvM3M3aE4vUEdkTGRj?=
 =?utf-8?B?Z2FxeDVYRXlqcDNBcXo5Y0pZUSsyNUkwb0pFQ2NrUUtLTC9sbXBPZkM1MTlv?=
 =?utf-8?B?L2crVjdUMEc1NExuQXhEd2xKZG5GTkM0dlpOeXA0c0VjZkdHTWM0RVVLMlVv?=
 =?utf-8?B?aGJGTVRiQ2xDY1YzMnRSTmZpYTFuVXo4SVM3QS9oNlRQc0s5LzZ3SHBxWGkx?=
 =?utf-8?B?UHlvOHBzbDRpRE9vQnVlYUxPY3c1SUdDRG9kR1VQYmRNcVFUQVBCSWJBMVgy?=
 =?utf-8?B?WFFUYlI0bkZNVXpwVEZMYitKcDBWYnR3MjYxRHNzZTU3TTVrUk9VYWdvRnhi?=
 =?utf-8?B?Y0VkR2ZQMitta1JiQVdseGxDRU1NMzZ1d3dyMjM0ZXhDZy9pT0JyZ3hwRVh4?=
 =?utf-8?B?YWRkY1ZYZ3BXS0QwYlJjRHdRN3oxOWRHSXowZVNDYkRTdjhVeUhsWDI0ZkJv?=
 =?utf-8?B?RmRIVVBRWkMwQ1dpdGloSmNLZlVLT2Zac21udTF1VEtsVWN4bCs1N2hPakxY?=
 =?utf-8?B?cC9EcllPbDc0cW8weDRVYzNYM1ppVVdsVk4rMTRsZW1aN1E1MWU3UlpsR0I3?=
 =?utf-8?B?MFl5NmhqK1NhMzVxSjVQRjhhR2toMjlSa052V1dxSVN4V1ZYeE14RzF3ZG1X?=
 =?utf-8?Q?EokZIbd6HiB3CzwI4GFcN6E=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: a8af877e-9ffc-44e7-07f3-08da9d9482cd
X-MS-Exchange-CrossTenant-AuthSource: MW3PR11MB4764.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Sep 2022 18:50:40.2635
 (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: WjLTkC/HdXRdndibuhYx3G4YKNUYw/qLgoSuXyawzVIqs2YgC5QmEZE6efhoCxMxOrwGduFaCzd/aqBPwUzVQbNZJBc0o+tnC7cfQC8jI9QaNyfOUiJJWELloHytX0dA
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB4830
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_NONE,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:31 AM, Alexander Duyck wrote:
> On Thu, Sep 22, 2022 at 3:38 PM Anirudh Venkataramanan
> <anirudh.venkataramanan@intel.com> 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].
>>>>
>>>> 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.
>>>>
>>>> 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.
> 
> Right, but that is comparing apples and oranges. As I said for Tx it
> would make sense, but since we are doing the allocations for Rx that
> isn't the case so we don't need it.
> 
>> ... 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().
>>
>> Is using page_address() directly beneficial in some way?
> 
> By that argument why don't we just leave the code alone and keep using
> kmap? I am pretty certain that is the logic that had us using kmap in
> the first place since it also dumps us with page_address in most cases
> and we didn't care much about the other architectures.

Well, my understanding is that kmap_local_page() doesn't have the 
overheads kmap() has, and that alone is reason enough to replace kmap() 
and kmap_atomic() with kmap_local_page() where possible.

> If you look at
> the kmap_local_page() it just adds an extra step or two to calling
> page_address(). In this case it is adding extra complication to
> something that isn't needed which is the reason why we are going
> through this in the first place. If we are going to pull the bandage I
> suggest we might as well just go all the way and not take a half-step
> since we don't actually need kmap or its related calls for this.

I don't really see this as "pulling the kmap() bandage", but a "use a 
more appropriate kmap function if you can" type situation.

FWIW, I am not against using page_address(). Just wanted to hash this 
out and get to a conclusion before I made new changes.

Ani