Re: [PATCH v2] target: add iscsi/cpus_allowed_list in configfs

From: Zou Mingzhe <mingzhe.zou@easystack.cn>
To: Mike Christie <michael.christie@oracle.com>,
	martin.petersen@oracle.com, linux-scsi@vger.kernel.org,
	target-devel@vger.kernel.org
Cc: zoumingzhe@qq.com
Subject: Re: [PATCH v2] target: add iscsi/cpus_allowed_list in configfs
Date: Tue, 1 Mar 2022 15:35:01 +0800	[thread overview]
Message-ID: <a9ce2a8d-1d63-b887-42bd-8e7137c2a877@easystack.cn> (raw)
In-Reply-To: <2f65d5dd-f2d8-3279-5a71-691614e65ae0@oracle.com>

在 2022/3/1 01:58, Mike Christie 写道:
> On 2/17/22 1:45 AM, mingzhe.zou@easystack.cn wrote:
>> From: Mingzhe Zou <mingzhe.zou@easystack.cn>
>>
>> The RX/TX threads for iSCSI connection can be scheduled to
>> any online cpus, and will not be rescheduled.
>>
>> If bind other heavy load threads with iSCSI connection
>> RX/TX thread to the same cpu, the iSCSI performance will
>> be worse.
>>
>> This patch add iscsi/cpus_allowed_list in configfs. The
>> available cpus set of iSCSI connection RX/TX threads is
>> allowed_cpus & online_cpus. If it is modified, all RX/TX
>> threads will be rescheduled.
>>
>> Signed-off-by: Mingzhe Zou <mingzhe.zou@easystack.cn>
>> ---
>>   drivers/target/iscsi/iscsi_target.c          | 66 +++++++++++++++++++-
>>   drivers/target/iscsi/iscsi_target_configfs.c | 32 ++++++++++
>>   drivers/target/iscsi/iscsi_target_login.c    |  8 +++
>>   include/target/iscsi/iscsi_target_core.h     | 31 ++-------
>>   4 files changed, 109 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
>> index 2c54c5d8412d..82f54b59996d 100644
>> --- a/drivers/target/iscsi/iscsi_target.c
>> +++ b/drivers/target/iscsi/iscsi_target.c
>> @@ -702,13 +702,19 @@ static int __init iscsi_target_init_module(void)
>>   	if (!iscsit_global->ts_bitmap)
>>   		goto configfs_out;
>>   
>> +	if (!zalloc_cpumask_var(&iscsit_global->allowed_cpumask, GFP_KERNEL)) {
>> +		pr_err("Unable to allocate iscsit_global->allowed_cpumask\n");
>> +		goto bitmap_out;
>> +	}
>> +	cpumask_setall(iscsit_global->allowed_cpumask);
>> +
>>   	lio_qr_cache = kmem_cache_create("lio_qr_cache",
>>   			sizeof(struct iscsi_queue_req),
>>   			__alignof__(struct iscsi_queue_req), 0, NULL);
>>   	if (!lio_qr_cache) {
>>   		pr_err("Unable to kmem_cache_create() for"
>>   				" lio_qr_cache\n");
>> -		goto bitmap_out;
>> +		goto cpumask_out;
>>   	}
>>   
>>   	lio_dr_cache = kmem_cache_create("lio_dr_cache",
>> @@ -753,6 +759,8 @@ static int __init iscsi_target_init_module(void)
>>   	kmem_cache_destroy(lio_dr_cache);
>>   qr_out:
>>   	kmem_cache_destroy(lio_qr_cache);
>> +cpumask_out:
>> +	free_cpumask_var(iscsit_global->allowed_cpumask);
>>   bitmap_out:
>>   	vfree(iscsit_global->ts_bitmap);
>>   configfs_out:
>> @@ -782,6 +790,7 @@ static void __exit iscsi_target_cleanup_module(void)
>>   
>>   	target_unregister_template(&iscsi_ops);
>>   
>> +	free_cpumask_var(iscsit_global->allowed_cpumask);
>>   	vfree(iscsit_global->ts_bitmap);
>>   	kfree(iscsit_global);
>>   }
>> @@ -3587,6 +3596,11 @@ static int iscsit_send_reject(
>>   void iscsit_thread_get_cpumask(struct iscsi_conn *conn)
>>   {
>>   	int ord, cpu;
>> +	cpumask_t conn_allowed_cpumask;
>> +
>> +	cpumask_and(&conn_allowed_cpumask, iscsit_global->allowed_cpumask,
>> +		    cpu_online_mask);
>> +
>>   	/*
>>   	 * bitmap_id is assigned from iscsit_global->ts_bitmap from
>>   	 * within iscsit_start_kthreads()
>> @@ -3595,8 +3609,9 @@ void iscsit_thread_get_cpumask(struct iscsi_conn *conn)
>>   	 * iSCSI connection's RX/TX threads will be scheduled to
>>   	 * execute upon.
>>   	 */
>> -	ord = conn->bitmap_id % cpumask_weight(cpu_online_mask);
>> -	for_each_online_cpu(cpu) {
>> +	cpumask_clear(conn->conn_cpumask);
>> +	ord = conn->bitmap_id % cpumask_weight(&conn_allowed_cpumask);
>> +	for_each_cpu(cpu, &conn_allowed_cpumask) {
>>   		if (ord-- == 0) {
>>   			cpumask_set_cpu(cpu, conn->conn_cpumask);
>>   			return;
>> @@ -3609,6 +3624,51 @@ void iscsit_thread_get_cpumask(struct iscsi_conn *conn)
>>   	cpumask_setall(conn->conn_cpumask);
>>   }
>>   
>> +static void iscsit_thread_reschedule(struct iscsi_conn *conn)
>> +{
>> +	/*
>> +	 * If iscsit_global->allowed_cpumask modified, reschedule iSCSI
>> +	 * connection's RX/TX threads update conn->allowed_cpumask.
>> +	 */
>> +	if (!cpumask_equal(iscsit_global->allowed_cpumask,
>> +			   conn->allowed_cpumask)) {
>> +		iscsit_thread_get_cpumask(conn);
>> +		conn->conn_tx_reset_cpumask = 1;
>> +		conn->conn_rx_reset_cpumask = 1;
>> +		cpumask_copy(conn->allowed_cpumask,
>> +			     iscsit_global->allowed_cpumask);
>> +	}
>> +}
>> +
>> +void iscsit_thread_check_cpumask(
>> +	struct iscsi_conn *conn,
>> +	struct task_struct *p,
>> +	int mode)
>> +{
>> +	iscsit_thread_reschedule(conn);
>> +
>> +	/*
>> +	 * mode == 1 signals iscsi_target_tx_thread() usage.
>> +	 * mode == 0 signals iscsi_target_rx_thread() usage.
>> +	 */
>> +	if (mode == 1) {
>> +		if (!conn->conn_tx_reset_cpumask)
>> +			return;
>> +		conn->conn_tx_reset_cpumask = 0;
>> +	} else {
>> +		if (!conn->conn_rx_reset_cpumask)
>> +			return;
>> +		conn->conn_rx_reset_cpumask = 0;
>> +	}
>> +	/*
>> +	 * Update the CPU mask for this single kthread so that
>> +	 * both TX and RX kthreads are scheduled to run on the
>> +	 * same CPU.
>> +	 */
>> +	set_cpus_allowed_ptr(p, conn->conn_cpumask);
>> +}
> We can hit a race where we call this twice for the same CPU right?
Yes, it should be safe to call twice set_cpus_allowed_ptr() with the 
same CPU for the same task.
>
> The rx and tx thread both call iscsit_thread_reschedule and cpumask_equal
> and it returns false for them. The rx thread might be faster and return
> from iscsit_thread_reschedule and is setting conn_rx_reset_cpumask to 0.
> Then the tx thread is sets it back to 1. The next time the rx thread loops
> it sees conn_rx_reset_cpumask set to 1 and calls set_cpus_allowed_ptr.

You are right. But we can call set_cpus_allowed_ptr() first, then set 
conn_rx_reset_cpumask to 1.

This will reduce such problems. I will modify it in v3.

>
> Is that the only possible race? If so it seems ok. Maybe just add a comment,
> so later when someone else is looking at the code they don't waste time
> and think it's broken and know it was intentional or at least we didn't care.
OK. I will add a comment in v3.
>