Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
Date:   Thu, 14 Sep 2023 13:17:27 -0700
From:   Ira Weiny <ira.weiny@intel.com>
To:     =?utf-8?B?VG9tw6HFoQ==?= Glozar <tglozar@gmail.com>,
        <nvdimm@lists.linux.dev>
CC:     <dan.j.williams@intel.com>, <vishal.l.verma@intel.com>,
        <dave.jiang@intel.com>, <ira.weiny@intel.com>,
        <linux-kernel@vger.kernel.org>, Tomas Glozar <tglozar@redhat.com>
Subject: Re: [PATCH] nd_btt: Make BTT lanes preemptible
Message-ID: <65036a57ea900_35db10294ec@iweiny-mobl.notmuch>
References: <20230912082440.325189-1-tglozar@gmail.com>
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20230912082440.325189-1-tglozar@gmail.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?SmZNNUFFV1lmSjZoZ1dJTGVKcjkzdU5ZVGlQenRKSFhuSGs5SzdicExzazR6?=
 =?utf-8?B?WlhBRmxkZUU2VzM0WWFCSmJURHJxYnkwVGs3QllOclRmV083UTJESVhHREJp?=
 =?utf-8?B?dmgwbXRTa0lSOXFhbm5aM2wybjBKUGExb2M3a3hCQUxRRWpDbVdWeko4Qytt?=
 =?utf-8?B?NVF2YkdTWW1xQTRGQUhMbTdtczhJTzJnbmtDV251QXpUemliNUFwaUFCcjR0?=
 =?utf-8?B?a0ExYk1SVG1mOUVibkVXZHZqL2luMUZ5VDR5L0IybzZIejFpNUMvcE4zcGFM?=
 =?utf-8?B?VWU2bjRsYVg1dlYxWUoxRHlHZVFrdXhPYzdlU2NuQmhCZ21vUnlYelJUZjMv?=
 =?utf-8?B?eS9RUTM1RTNpSXZ0RnN2SWd4ZnB1dXZNbVlIZ3MwQUZFdU1oZXViZlJ3dnE4?=
 =?utf-8?B?b1pwZTUzR0doa0hvRG5NUkZITDNieER3WFVQa1lydllUNTgwWmpDOCs2RE5G?=
 =?utf-8?B?c2MvMkNrWHlvQ1ZrbGVUT2hlVnZnUm83SktuVy9RUHpuSktCdTZEcVFxYURn?=
 =?utf-8?B?M1JRaXo1bVF0blpNRG1XMjRDTlp2Y0Y4ajNoYTlMd0xxa1E5WDUzdzZlemNm?=
 =?utf-8?B?TmtZMnF4SldSaENmb2dDR1BTNkFZS3hXZTJycDNHM0VDOFpqZFJxc21SRjd2?=
 =?utf-8?B?bEk3ZUU4Zlk5dTczM1N6dHRFa0JXdkJhbE11VmNiK2x6SG5WNUxWYzVCdmpB?=
 =?utf-8?B?b1BOSmxSMEN1UFdqekhaOFY2SVV6bTZ3UGZxanRYUmtScnpiZFI1Nng2Rk5a?=
 =?utf-8?B?SFNDWGJ3VVVzUXVDZDZoUWdxM1VJd2hleFFUNThveWVpaU5Gc0JXeHp6NGVi?=
 =?utf-8?B?TmVQUzJIV1JoWHZGOTM0Qy9JTTliTS95MllSZ0trZllGWUUxR2cxZ29ENCtt?=
 =?utf-8?B?MWpmczhzZkMvRWlSUVd2eDh2Vm5RYS9HUFgvZmczN0h4M2MvYktJQk5pS2Fu?=
 =?utf-8?B?bHRQcmJ1N1hrKzBkcS9USlJlK1RTWW1heWxPNy91YmY1Mk9JeDNQdmxqS0RP?=
 =?utf-8?B?aFV1akMyMk9yS21aTHRCT2xlRnVZdGJVZTluQ0M3d09MQXN4UGswOUJoREVN?=
 =?utf-8?B?KzVzcjdGbjZiQ20waUw2SldkTlUxS3NlUTBPOXZadnhzZVlNNXlvWE9IMWRz?=
 =?utf-8?B?ckRYZXZlcjVWeDNVNTJVRWU4L29SMU5JbmtEbUFqVXFIbEtCV3JhZnk3SHM0?=
 =?utf-8?B?R2ViMGVNV0dVSEJra2pkS2FkMi9DdlJGRi84SEY1UW1vUkw5RldMSlZERitY?=
 =?utf-8?B?Q0Vjcm5PRFFOZkZmOU9zWDZkMlFnd21FUmF4MnZSTXNJRU9sZUdGM1gwcnFU?=
 =?utf-8?B?NTdvczhjYUh2cG5yTnVUWCtsYXdHMEJlOWJLOWR3bXZOZHExeWdhMHJNOC9K?=
 =?utf-8?B?bC9YV292QXNWQVhyTjJPaU9ta1dmYkxrZ1FSTkU0Q3N2ZEJVU0dIT1NIZHR6?=
 =?utf-8?B?UWJ5aG1rYXFWb3NuU1ArOW5YZUVxSCtZOWlYaDNqUzhHZ3c2RGw5ajhaZDFB?=
 =?utf-8?B?RFFNMzhHckNXOGl0RWN5TkpTUlJINkk5dWp5RTN1ZHQyYTJsWEF2Vk9QOThK?=
 =?utf-8?B?aGRiZXJiNHltNW9UaWxzZVJSS3l4Z1BuYjVnNG1nbCs3YW9jdDNlQ1FZWksz?=
 =?utf-8?B?c2QwaGkwL0F0OWw1SVR1VnhQRk5odXVRaWdHcHNBanBlcHV5a1ZGdHNXT3Bv?=
 =?utf-8?B?RGV6SVZVK1BNdnRXQ0kvYUt5VFlzRlJQQ2dLTWNXT0hXMVNwSTBEbFJhclpN?=
 =?utf-8?B?UUcxaDByUmNCNVBGN0xvbHdadXNseDNyc1ZOVVFqMmtxWjgvUC8yVDJBUkp6?=
 =?utf-8?B?QnhpWlpYa3BDMnhJUkMzZmNxbE1CMmRESXNLNjJlQVZVS05tTkJJdnA5SXZP?=
 =?utf-8?B?MTFORWFnYTRTdDg4SSs3ODRlYVpiQnZHcjZSbi80RzJubmpkaERyeEJUUzZj?=
 =?utf-8?B?bGxkYWVGd2ZYcEI4OENzYUJ0VDRCWWI1M1ljTWVyMnZJYzRIVWEvbHZ2NHJw?=
 =?utf-8?B?VSt5d0R6OENRdWpvMU83UE90MzhFaFVDWVhxMUxmdW41MmE1RnJRN3RCcFB5?=
 =?utf-8?B?TE5tVzV3VktpTStCdFlyandyNzRxblptMnZ5aG0yeHlCS29TZUJkcTNSdDNv?=
 =?utf-8?Q?vWGjDlEq+wstag3f6CIoIVdtn?=
X-MS-Exchange-CrossTenant-Network-Message-Id: e5eea614-4555-4013-dd58-08dbb55f9f82
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6733.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Sep 2023 20:17:30.8013
 (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: XuSp5pBiRdumfd5gVcpFiHFOQevL/4v2Cr+JdpyVXtIGvEwQ4PboTZU4hexNDYfehqehkPQv6+/qdRNPIK58gw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA0PR11MB7282
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (fry.vger.email [0.0.0.0]); Thu, 14 Sep 2023 13:17:48 -0700 (PDT)
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=unavailable
	autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on fry.vger.email

Tomáš Glozar wrote:
> From: Tomas Glozar <tglozar@redhat.com>
> 
> nd_region_acquire_lane uses get_cpu, which disables preemption. This is
> an issue on PREEMPT_RT kernels, since btt_write_pg and also
> nd_region_acquire_lane itself take a spin lock, resulting in BUG:
> sleeping function called from invalid context.

Is the bug in 1 of 2 places?

1) When btt_write_pg()->lock_map() (when the number of lanes is < number
   of cpus) and the lane is acquired is called?

*or*

2) When nd_region_acquire_lane() internally trys to take it's lock?

A copy/paste of the BUG observed would have been more clear I think.

Regardless I *think* this is ok but I'm worried I don't fully understand
what the problem is.

Ira

> 
> Fix the issue by replacing get_cpu with smp_process_id and
> migrate_disable when needed. This makes BTT operations preemptible, thus
> permitting the use of spin_lock.
> 
> Fixes: 5212e11fde4d ("nd_btt: atomic sector updates")
> Signed-off-by: Tomas Glozar <tglozar@redhat.com>
> ---
>  drivers/nvdimm/region_devs.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c
> index 0a81f87f6f6c..e2f1fb99707f 100644
> --- a/drivers/nvdimm/region_devs.c
> +++ b/drivers/nvdimm/region_devs.c
> @@ -939,7 +939,8 @@ unsigned int nd_region_acquire_lane(struct nd_region *nd_region)
>  {
>  	unsigned int cpu, lane;
>  
> -	cpu = get_cpu();
> +	migrate_disable();
> +	cpu = smp_processor_id();
>  	if (nd_region->num_lanes < nr_cpu_ids) {
>  		struct nd_percpu_lane *ndl_lock, *ndl_count;
>  
> @@ -958,16 +959,15 @@ EXPORT_SYMBOL(nd_region_acquire_lane);
>  void nd_region_release_lane(struct nd_region *nd_region, unsigned int lane)
>  {
>  	if (nd_region->num_lanes < nr_cpu_ids) {
> -		unsigned int cpu = get_cpu();
> +		unsigned int cpu = smp_processor_id();
>  		struct nd_percpu_lane *ndl_lock, *ndl_count;
>  
>  		ndl_count = per_cpu_ptr(nd_region->lane, cpu);
>  		ndl_lock = per_cpu_ptr(nd_region->lane, lane);
>  		if (--ndl_count->count == 0)
>  			spin_unlock(&ndl_lock->lock);
> -		put_cpu();
>  	}
> -	put_cpu();
> +	migrate_enable();
>  }
>  EXPORT_SYMBOL(nd_region_release_lane);
>  
> -- 
> 2.39.3
>