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;
Date:   Sun, 18 Jun 2023 11:11:08 -0700
From:   Ira Weiny <ira.weiny@intel.com>
To:     Sumitra Sharma <sumitraartsy@gmail.com>,
        Jani Nikula <jani.nikula@linux.intel.com>,
        Joonas Lahtinen <joonas.lahtinen@linux.intel.com>,
        Rodrigo Vivi <rodrigo.vivi@intel.com>,
        Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>,
        David Airlie <airlied@gmail.com>,
        Daniel Vetter <daniel@ffwll.ch>,
        <intel-gfx@lists.freedesktop.org>,
        <dri-devel@lists.freedesktop.org>, <linux-kernel@vger.kernel.org>,
        Thomas =?iso-8859-1?Q?Hellstr=F6m_=28Intel=29?= 
        <thomas_os@shipmail.org>
CC:     Ira Weiny <ira.weiny@intel.com>, Fabio <fmdefrancesco@gmail.com>,
        "Deepak R Varma" <drv@mailo.com>,
        Sumitra Sharma <sumitraartsy@gmail.com>
Subject: Re: [PATCH v2] drm/i915: Replace kmap() with kmap_local_page()
Message-ID: <648f48bc3d3c2_1de3f9294a3@iweiny-mobl.notmuch>
References: <20230617180420.GA410966@sumitra.com>
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20230617180420.GA410966@sumitra.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?bTQ4WkMxUnlEOUFpb29jeHlrVDh0WExFZkRSLzA5aytOVHAvdHQzWXZuTUR0?=
 =?utf-8?B?MFNVNXpKQ1dhcGxsT05WcWY2eURES2hqL2EvZTJqbjUwMjY3bjBpWGJoQ2l6?=
 =?utf-8?B?SU04OE9YSWhjUkNmU0pVNGtjWjZxNXFudzJzRFpidENUZjAvamk1SC8rQmNp?=
 =?utf-8?B?OGxuYzMvVVFuZUl1QmpUR0dIWlh3MkZONEdwWEVQOXE0WEV3VS9SSERjUnA4?=
 =?utf-8?B?L3REL095RGU2OVJtNHEvSGtVSE5QWTdSSjJiQy9yU3pvaXlvNlBtcERGWld2?=
 =?utf-8?B?UkpOM2RkQkFlN2p2WjZjRFZwWEE0aTRYeXNIOHBSU0JqK2lQVXFrOXF6Y2NN?=
 =?utf-8?B?anVudTFhdm1hWU9UbHJpL0VaYjUzNVNpQ1VWUkJ0TGd6TW94d3ltWk93eVFn?=
 =?utf-8?B?dVRFUWpNblNqNVl5M2NhTUJCZkRsS0RMcm5CRmliTWVYTC9TeWdvSSswREhK?=
 =?utf-8?B?ZmJCbFg1SnBrN0x3clBRcUNxWVVKcXV1elRGa3RURzdpdFgzM0FKZ2FMQzJW?=
 =?utf-8?B?RDd0UDh4ajNsYjh4WHhVeGNyMmRET3UxTDZyemNjbWdVSHFIRTdVd2Joa256?=
 =?utf-8?B?M0hsMVFMYnZFYlg1WFF1NHJhenZmbkdlR0M5djZNcVlBOTg4TEJIWDFqNVNE?=
 =?utf-8?B?UlhPS0Z1Tkhrd3diQUNuNUt6L3FGQUd1OGg1QlQ3ZmsvM1FrbEpWZEF5Skhw?=
 =?utf-8?B?bHNmSldhbkw0LzUwd1F3NkpQWUt4U3c0U2c3S0tOWVJ6OGxhblpVL3BoRkZj?=
 =?utf-8?B?aEtMaHdQMUZCZm5BZUJHNSt3SFlVM20veGRrN3M4dDRDZ3c2SlVPdFZQQkdQ?=
 =?utf-8?B?UFFZZGdCeFI5MXlFSW05d24rbG1ZSkdjd1FFTm83ZVAvNTJtYjBwaXBMVUxH?=
 =?utf-8?B?MisraHJId0psS01sUStTUTMxNmRLYmZ0WjJ0OVpXaWFLSFpDellhUVRwaHh4?=
 =?utf-8?B?ZksvYXk1R0VnM1o0c0o2RXFscDRhTUpXSEl0WUhNQm9peUI1KzJncDZyV2tL?=
 =?utf-8?B?OUNFOUpBZjdQbDQ4SldXWWVnNDNrSEY0NDRQRHlVc1BZRzFxSzhjOVlSNTdy?=
 =?utf-8?B?VzNvbU9sTkwxL1lzVUlhZTJxWjVEMDhwWUp0TlczYUxQRkNUdjVxSE9ZaWho?=
 =?utf-8?B?aGFSOWRrVkduMmxrcEh1K1E0Zlh0Mk9SQ2d5aWJvbXYydkc2SWt0bTViL2hR?=
 =?utf-8?B?Vy9XTEJHMXFrQ2l3U3JoQ2drZHZQQy9hT1BVU0ZDY083SzlOLzd2dTRqeWxO?=
 =?utf-8?B?TUdzV2JyaVFGWjhOMXZBVng4VEVXYkt6SWYwRVhrOWtKdStsb2h1dmFteWl4?=
 =?utf-8?B?OHRQbFhYTGZ0QmJON3Vocy8yWm90WmxyNjFKS2l3VWJMV05EajREeHJXdmlj?=
 =?utf-8?B?dWFFOHpzSUJWZHpzWndLc0NLMmo3WUtzTmJweGZmNS9odlVaekNCMDFnTmx2?=
 =?utf-8?B?S2hxWEpOUmFoRUlqQURWTGVidlBYMXNSNWRpYlJlKzBPdWorL1RDL0dPQkhx?=
 =?utf-8?B?bTJnU1hiUVh3Qi9xbE1tekYwc2Vmc1RiTE9HdGNLVFBmOEg5ZGtTWW04bkcv?=
 =?utf-8?B?b0lDdnJJeUErR2srbUJqWWNWeFd4TzFRVkU2WWRoYXdRNVRiVXpWdmhqSVpM?=
 =?utf-8?B?aGZ2QzRzTSsvSDdIVEVrbURzYWlxQ2ZoQ2JHeFNkenVuWUUwUFlCN205Z0d3?=
 =?utf-8?B?Y2FhbjVnYlR3V0JmNE9FbDMrNFNXQTVJc3REcTZHR1ZaYnJacTZBNFBVSzZW?=
 =?utf-8?B?amJuZDlnN0JQNXpQQ014TWluZmZNWTBDOHpCU2N1eElTTWtad2thTjBWWWpw?=
 =?utf-8?B?eE9iSjFuLzJJd0JwR0ZqNHQ1SHlrNUVGVlIxQTVaMkpBamhxZzFQQkZCQlBY?=
 =?utf-8?B?MUYrSGRZZUV6eXN6cUEyL2JHREZGQldJOXdtU0tnQnZOS0V1YUV3Sm0wOEV0?=
 =?utf-8?B?STRMalFKZmNqMEFCTlV0Nm81QWZ2aW5Ga0kvWXN0RVMxRmtONHF1eEpWKy9q?=
 =?utf-8?B?ay9TMTRLVlU0MWkybVJDMXJZUlJyU0dacE9LK2RoR3NNT1lMQkR1OWlCcy9G?=
 =?utf-8?B?MlRDR1ozckNKVGZMSk9Sd3ZXenhPaWhteHVVSmEwMW1KajNzNU5MS0xtTmhq?=
 =?utf-8?Q?NZ+AuDGB8q9qjdUErnG6rr0xZ?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 26b1cb24-f28e-4bbd-6f51-08db702767ab
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6733.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 Jun 2023 18:11:15.2309
 (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: Xtj5Dzb99TbtFiVzRlZrcKZICE0fc2xRKQldOhcZq4LkUcevWBgnFCEAPD/2Sx/4Jg0XABBHR+9SldOIfn7b1g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CYYPR11MB8358
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,RCVD_IN_DNSWL_MED,
        SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE 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

Sumitra Sharma wrote:
> kmap() has been deprecated in favor of the kmap_local_page()
> due to high cost, restricted mapping space, the overhead of a
> global lock for synchronization, and making the process sleep
> in the absence of free slots.
> 
> kmap_local_page() is faster than kmap() and offers thread-local
> and CPU-local mappings, take pagefaults in a local kmap region
> and preserves preemption by saving the mappings of outgoing tasks
> and restoring those of the incoming one during a context switch.
> 
> The mapping is kept thread local in the function
> “i915_vma_coredump_create” in i915_gpu_error.c
> 
> Therefore, replace kmap() with kmap_local_page().
> 
> Suggested-by: Ira Weiny <ira.weiny@intel.com>
> 

NIT: No need for the line break between Suggested-by and your signed off line.

> Signed-off-by: Sumitra Sharma <sumitraartsy@gmail.com>
> ---
> 
> Changes in v2:
> 	- Replace kmap() with kmap_local_page().

Generally it is customary to attribute a change like this to those who
suggested it in a V1 review.

For example:

 	- Tvrtko/Thomas: Use kmap_local_page() instead of page_address()

Also I don't see Thomas on the new email list.  Since he took the time to
review V1 he might want to check this version out.  I've added him to the
'To:' list.

Also a link to V1 is nice.  B4 formats it like this:

- Link to v1: https://lore.kernel.org/all/20230614123556.GA381200@sumitra.com/

All that said the code looks good to me.  So with the above changes.

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> 	- Change commit subject and message.
> 
>  drivers/gpu/drm/i915/i915_gpu_error.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> index f020c0086fbc..bc41500eedf5 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -1164,9 +1164,9 @@ i915_vma_coredump_create(const struct intel_gt *gt,
>  
>  			drm_clflush_pages(&page, 1);
>  
> -			s = kmap(page);
> +			s = kmap_local_page(page);
>  			ret = compress_page(compress, s, dst, false);
> -			kunmap(page);
> +			kunmap_local(s);
>  
>  			drm_clflush_pages(&page, 1);
>  
> -- 
> 2.25.1
>