[PATCH] target: add iscsi/cpus_allowed_list in configfs

[PATCH] target: add iscsi/cpus_allowed_list in configfs

[mingzhe.zou]

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          | 21 ++++++++++--
 drivers/target/iscsi/iscsi_target.h          | 17 ++++++++++
 drivers/target/iscsi/iscsi_target_configfs.c | 34 ++++++++++++++++++++
 drivers/target/iscsi/iscsi_target_login.c    |  7 ++++
 include/target/iscsi/iscsi_target_core.h     |  1 +
 5 files changed, 78 insertions(+), 2 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 2c54c5d8412d..a18d3fc3cfd1 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -693,6 +693,11 @@ static int __init iscsi_target_init_module(void)
 	mutex_init(&auth_id_lock);
 	idr_init(&tiqn_idr);
 
+	/*
+	 * allow all cpus run iscsi_ttx and iscsi_trx
+	 */
+	cpumask_setall(&__iscsi_allowed_cpumask);
+
 	ret = target_register_template(&iscsi_ops);
 	if (ret)
 		goto out;
@@ -3587,6 +3592,15 @@ static int iscsit_send_reject(
 void iscsit_thread_get_cpumask(struct iscsi_conn *conn)
 {
 	int ord, cpu;
+	cpumask_t conn_allowed_cpumask;
+
+	/*
+	 * The available cpus set of iSCSI connection's RX/TX threads
+	 */
+	cpumask_and(&conn_allowed_cpumask,
+		&__iscsi_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;
@@ -3821,6 +3836,7 @@ int iscsi_target_tx_thread(void *arg)
 		 * Ensure that both TX and RX per connection kthreads
 		 * are scheduled to run on the same CPU.
 		 */
+		iscsit_thread_reschedule(conn);
 		iscsit_thread_check_cpumask(conn, current, 1);
 
 		wait_event_interruptible(conn->queues_wq,
@@ -3966,6 +3982,7 @@ static void iscsit_get_rx_pdu(struct iscsi_conn *conn)
 		 * Ensure that both TX and RX per connection kthreads
 		 * are scheduled to run on the same CPU.
 		 */
+		iscsit_thread_reschedule(conn);
 		iscsit_thread_check_cpumask(conn, current, 0);
 
 		memset(&iov, 0, sizeof(struct kvec));
diff --git a/drivers/target/iscsi/iscsi_target.h b/drivers/target/iscsi/iscsi_target.h
index b35a96ded9c1..cb97a316d76d 100644
--- a/drivers/target/iscsi/iscsi_target.h
+++ b/drivers/target/iscsi/iscsi_target.h
@@ -57,4 +57,21 @@ extern struct kmem_cache *lio_r2t_cache;
 extern struct ida sess_ida;
 extern struct mutex auth_id_lock;
 
+extern cpumask_t __iscsi_allowed_cpumask;
+
+static inline void iscsit_thread_reschedule(struct iscsi_conn *conn)
+{
+	/*
+	 * If __iscsi_allowed_cpumask modified, reschedule iSCSI connection's
+	 * RX/TX threads update conn->allowed_cpumask.
+	 */
+	if (!cpumask_equal(&__iscsi_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,
+			&__iscsi_allowed_cpumask);
+	}
+}
+
 #endif   /*** ISCSI_TARGET_H ***/
diff --git a/drivers/target/iscsi/iscsi_target_configfs.c b/drivers/target/iscsi/iscsi_target_configfs.c
index 2a9de24a8bbe..dc12b1695838 100644
--- a/drivers/target/iscsi/iscsi_target_configfs.c
+++ b/drivers/target/iscsi/iscsi_target_configfs.c
@@ -1127,8 +1127,42 @@ static ssize_t lio_target_wwn_lio_version_show(struct config_item *item,
 
 CONFIGFS_ATTR_RO(lio_target_wwn_, lio_version);
 
+cpumask_t __iscsi_allowed_cpumask;
+
+static ssize_t lio_target_wwn_cpus_allowed_list_show(
+		struct config_item *item, char *page)
+{
+	return sprintf(page, "%*pbl\n",
+		cpumask_pr_args(&__iscsi_allowed_cpumask));
+}
+
+static ssize_t lio_target_wwn_cpus_allowed_list_store(
+		struct config_item *item, const char *page, size_t count)
+{
+	int ret;
+	char *orig;
+	cpumask_t new_allowed_cpumask;
+
+	orig = kstrdup(page, GFP_KERNEL);
+	if (!orig)
+		return -ENOMEM;
+
+	cpumask_clear(&new_allowed_cpumask);
+	ret = cpulist_parse(orig, &new_allowed_cpumask);
+
+	kfree(orig);
+	if (ret != 0)
+		return ret;
+
+	cpumask_copy(&__iscsi_allowed_cpumask, &new_allowed_cpumask);
+	return count;
+}
+
+CONFIGFS_ATTR(lio_target_wwn_, cpus_allowed_list);
+
 static struct configfs_attribute *lio_target_wwn_attrs[] = {
 	&lio_target_wwn_attr_lio_version,
+	&lio_target_wwn_attr_cpus_allowed_list,
 	NULL,
 };
 
diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c
index 1a9c50401bdb..910f35e4648a 100644
--- a/drivers/target/iscsi/iscsi_target_login.c
+++ b/drivers/target/iscsi/iscsi_target_login.c
@@ -1129,8 +1129,15 @@ static struct iscsi_conn *iscsit_alloc_conn(struct iscsi_np *np)
 		goto free_conn_ops;
 	}
 
+	if (!zalloc_cpumask_var(&conn->allowed_cpumask, GFP_KERNEL)) {
+		pr_err("Unable to allocate conn->allowed_cpumask\n");
+		goto free_conn_cpumask;
+	}
+
 	return conn;
 
+free_conn_cpumask:
+	free_cpumask_var(conn->allowed_cpumask);
 free_conn_ops:
 	kfree(conn->conn_ops);
 put_transport:
diff --git a/include/target/iscsi/iscsi_target_core.h b/include/target/iscsi/iscsi_target_core.h
index 1eccb2ac7d02..c5e9cad187cf 100644
--- a/include/target/iscsi/iscsi_target_core.h
+++ b/include/target/iscsi/iscsi_target_core.h
@@ -580,6 +580,7 @@ struct iscsi_conn {
 	struct ahash_request	*conn_tx_hash;
 	/* Used for scheduling TX and RX connection kthreads */
 	cpumask_var_t		conn_cpumask;
+	cpumask_var_t		allowed_cpumask;
 	unsigned int		conn_rx_reset_cpumask:1;
 	unsigned int		conn_tx_reset_cpumask:1;
 	/* list_head of struct iscsi_cmd for this connection */
-- 
2.17.1

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

[Mike Christie]

On 1/25/22 2:38 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          | 21 ++++++++++--
>  drivers/target/iscsi/iscsi_target.h          | 17 ++++++++++
>  drivers/target/iscsi/iscsi_target_configfs.c | 34 ++++++++++++++++++++
>  drivers/target/iscsi/iscsi_target_login.c    |  7 ++++
>  include/target/iscsi/iscsi_target_core.h     |  1 +
>  5 files changed, 78 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
> index 2c54c5d8412d..a18d3fc3cfd1 100644
> --- a/drivers/target/iscsi/iscsi_target.c
> +++ b/drivers/target/iscsi/iscsi_target.c
> @@ -693,6 +693,11 @@ static int __init iscsi_target_init_module(void)
>  	mutex_init(&auth_id_lock);
>  	idr_init(&tiqn_idr);
>  
> +	/*
> +	 * allow all cpus run iscsi_ttx and iscsi_trx

I would just drop the comment. The "setall" in the function name
is pretty clear already.


> +	 */
> +	cpumask_setall(&__iscsi_allowed_cpumask);
> +
>  	ret = target_register_template(&iscsi_ops);
>  	if (ret)
>  		goto out;
> @@ -3587,6 +3592,15 @@ static int iscsit_send_reject(
>  void iscsit_thread_get_cpumask(struct iscsi_conn *conn)
>  {
>  	int ord, cpu;
> +	cpumask_t conn_allowed_cpumask;
> +
> +	/*
> +	 * The available cpus set of iSCSI connection's RX/TX threads
> +	 */

Probably can drop the comment too since the variable names make it
known what we are doing.


> +	cpumask_and(&conn_allowed_cpumask,
> +		&__iscsi_allowed_cpumask,
> +		cpu_online_mask);

The formatting needs some fix ups. I think __iscsi_allowed_cpumask can fit on the
line above it and then cpu_online_mask should be moved over a couple spaces to
align with the opening "(".


> +
>  	/*
>  	 * 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;
> @@ -3821,6 +3836,7 @@ int iscsi_target_tx_thread(void *arg)
>  		 * Ensure that both TX and RX per connection kthreads
>  		 * are scheduled to run on the same CPU.
>  		 */
> +		iscsit_thread_reschedule(conn);


If we have multiple sessions to the same portal, could we end up racing
where session0's tx/rx threads call iscsit_thread_reschedule and
iscsit_thread_check_cpumask at the same time as session1's threads and
they end up taking the same cpus. They both could be running
iscsit_thread_get_cpumask at the same time, see he same masks values
and in the for_each_cpu loop think the same cpu is free.


>  		iscsit_thread_check_cpumask(conn, current, 1);
>  
>  		wait_event_interruptible(conn->queues_wq,
> @@ -3966,6 +3982,7 @@ static void iscsit_get_rx_pdu(struct iscsi_conn *conn)
>  		 * Ensure that both TX and RX per connection kthreads
>  		 * are scheduled to run on the same CPU.
>  		 */
> +		iscsit_thread_reschedule(conn);
>  		iscsit_thread_check_cpumask(conn, current, 0);
>  
>  		memset(&iov, 0, sizeof(struct kvec));
> diff --git a/drivers/target/iscsi/iscsi_target.h b/drivers/target/iscsi/iscsi_target.h
> index b35a96ded9c1..cb97a316d76d 100644
> --- a/drivers/target/iscsi/iscsi_target.h
> +++ b/drivers/target/iscsi/iscsi_target.h
> @@ -57,4 +57,21 @@ extern struct kmem_cache *lio_r2t_cache;
>  extern struct ida sess_ida;
>  extern struct mutex auth_id_lock;
>  
> +extern cpumask_t __iscsi_allowed_cpumask;

I would rename to:

iscsit_allowed_cpumask


> +
> +static inline void iscsit_thread_reschedule(struct iscsi_conn *conn)
> +{
> +	/*
> +	 * If __iscsi_allowed_cpumask modified, reschedule iSCSI connection's
> +	 * RX/TX threads update conn->allowed_cpumask.
> +	 */
> +	if (!cpumask_equal(&__iscsi_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,
> +			&__iscsi_allowed_cpumask);

Fix up formatting like above.

> +	}
> +}
> +
>  #endif   /*** ISCSI_TARGET_H ***/
> diff --git a/drivers/target/iscsi/iscsi_target_configfs.c b/drivers/target/iscsi/iscsi_target_configfs.c
> index 2a9de24a8bbe..dc12b1695838 100644
> --- a/drivers/target/iscsi/iscsi_target_configfs.c
> +++ b/drivers/target/iscsi/iscsi_target_configfs.c
> @@ -1127,8 +1127,42 @@ static ssize_t lio_target_wwn_lio_version_show(struct config_item *item,
>  
>  CONFIGFS_ATTR_RO(lio_target_wwn_, lio_version);
>  
> +cpumask_t __iscsi_allowed_cpumask;

Maybe better to put this in iscsi_target.c with the other vars like
it.

> +
> +static ssize_t lio_target_wwn_cpus_allowed_list_show(
> +		struct config_item *item, char *page)
> +{
> +	return sprintf(page, "%*pbl\n",
> +		cpumask_pr_args(&__iscsi_allowed_cpumask));

Formatting like above.

> +}
> +
> +static ssize_t lio_target_wwn_cpus_allowed_list_store(
> +		struct config_item *item, const char *page, size_t count)
> +{
> +	int ret;
> +	char *orig;
> +	cpumask_t new_allowed_cpumask;
> +
> +	orig = kstrdup(page, GFP_KERNEL);
> +	if (!orig)
> +		return -ENOMEM;
> +
> +	cpumask_clear(&new_allowed_cpumask);
> +	ret = cpulist_parse(orig, &new_allowed_cpumask);

Are you supposed to do a zalloc_cpumask_var/free_cpumask_var before
using new_allowed_cpumask? It looks like other callers are doing it.

> +
> +	kfree(orig);
> +	if (ret != 0)
> +		return ret;
> +
> +	cpumask_copy(&__iscsi_allowed_cpumask, &new_allowed_cpumask);
> +	return count;
> +}
> +
> +CONFIGFS_ATTR(lio_target_wwn_, cpus_allowed_list);
> +
>  static struct configfs_attribute *lio_target_wwn_attrs[] = {
>  	&lio_target_wwn_attr_lio_version,
> +	&lio_target_wwn_attr_cpus_allowed_list,
>  	NULL,
>  };
>  
> diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c
> index 1a9c50401bdb..910f35e4648a 100644
> --- a/drivers/target/iscsi/iscsi_target_login.c
> +++ b/drivers/target/iscsi/iscsi_target_login.c
> @@ -1129,8 +1129,15 @@ static struct iscsi_conn *iscsit_alloc_conn(struct iscsi_np *np)
>  		goto free_conn_ops;
>  	}
>  
> +	if (!zalloc_cpumask_var(&conn->allowed_cpumask, GFP_KERNEL)) {
> +		pr_err("Unable to allocate conn->allowed_cpumask\n");
> +		goto free_conn_cpumask;
> +	}

I think in iscsit_free_conn you need a:

free_cpumask_var(conn->allowed_cpumask);

to go with this allocation.


> +
>  	return conn;
>  
> +free_conn_cpumask:
> +	free_cpumask_var(conn->allowed_cpumask);

I think you wanted conn->conn_cpumask here.

>  free_conn_ops:
>  	kfree(conn->conn_ops);
>  put_transport:
> diff --git a/include/target/iscsi/iscsi_target_core.h b/include/target/iscsi/iscsi_target_core.h
> index 1eccb2ac7d02..c5e9cad187cf 100644
> --- a/include/target/iscsi/iscsi_target_core.h
> +++ b/include/target/iscsi/iscsi_target_core.h
> @@ -580,6 +580,7 @@ struct iscsi_conn {
>  	struct ahash_request	*conn_tx_hash;
>  	/* Used for scheduling TX and RX connection kthreads */
>  	cpumask_var_t		conn_cpumask;
> +	cpumask_var_t		allowed_cpumask;
>  	unsigned int		conn_rx_reset_cpumask:1;
>  	unsigned int		conn_tx_reset_cpumask:1;
>  	/* list_head of struct iscsi_cmd for this connection */

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

[Zou Mingzhe]

在 2022/2/9 上午1:50, Mike Christie 写道:
> On 1/25/22 2:38 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          | 21 ++++++++++--
>>   drivers/target/iscsi/iscsi_target.h          | 17 ++++++++++
>>   drivers/target/iscsi/iscsi_target_configfs.c | 34 ++++++++++++++++++++
>>   drivers/target/iscsi/iscsi_target_login.c    |  7 ++++
>>   include/target/iscsi/iscsi_target_core.h     |  1 +
>>   5 files changed, 78 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
>> index 2c54c5d8412d..a18d3fc3cfd1 100644
>> --- a/drivers/target/iscsi/iscsi_target.c
>> +++ b/drivers/target/iscsi/iscsi_target.c
>> @@ -693,6 +693,11 @@ static int __init iscsi_target_init_module(void)
>>   	mutex_init(&auth_id_lock);
>>   	idr_init(&tiqn_idr);
>>   
>> +	/*
>> +	 * allow all cpus run iscsi_ttx and iscsi_trx
> I would just drop the comment. The "setall" in the function name
> is pretty clear already.
I will remove it.
>
>
>> +	 */
>> +	cpumask_setall(&__iscsi_allowed_cpumask);
>> +
>>   	ret = target_register_template(&iscsi_ops);
>>   	if (ret)
>>   		goto out;
>> @@ -3587,6 +3592,15 @@ static int iscsit_send_reject(
>>   void iscsit_thread_get_cpumask(struct iscsi_conn *conn)
>>   {
>>   	int ord, cpu;
>> +	cpumask_t conn_allowed_cpumask;
>> +
>> +	/*
>> +	 * The available cpus set of iSCSI connection's RX/TX threads
>> +	 */
> Probably can drop the comment too since the variable names make it
> known what we are doing.
I will remove it.
>
>
>> +	cpumask_and(&conn_allowed_cpumask,
>> +		&__iscsi_allowed_cpumask,
>> +		cpu_online_mask);
> The formatting needs some fix ups. I think __iscsi_allowed_cpumask can fit on the
> line above it and then cpu_online_mask should be moved over a couple spaces to
> align with the opening "(".

I will reformat.

>
>
>> +
>>   	/*
>>   	 * 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;
>> @@ -3821,6 +3836,7 @@ int iscsi_target_tx_thread(void *arg)
>>   		 * Ensure that both TX and RX per connection kthreads
>>   		 * are scheduled to run on the same CPU.
>>   		 */
>> +		iscsit_thread_reschedule(conn);
>
> If we have multiple sessions to the same portal, could we end up racing
> where session0's tx/rx threads call iscsit_thread_reschedule and
> iscsit_thread_check_cpumask at the same time as session1's threads and
> they end up taking the same cpus. They both could be running
> iscsit_thread_get_cpumask at the same time, see he same masks values
> and in the for_each_cpu loop think the same cpu is free.

Simply, this patch just adds a config item to skip some CPUs that are 
bound to other threads,

not to change the calculation method of cpumask.

When the cpus_allowed_list in sysfs is modified, the tx/rx threads of 
all sessions will clear current

cpumask and call iscsit_thread_reschedule and 
iscsit_thread_check_cpumask to get a new value.

Because each session is allocated a unique bitmap_id in advance, then 
calculate the cpumask

via bitmap_id. So the tx/rx threads of different sessions will be 
scheduled to different CPUs (when

the number of CPUs is sufficient).

>
>
>>   		iscsit_thread_check_cpumask(conn, current, 1);
>>   
>>   		wait_event_interruptible(conn->queues_wq,
>> @@ -3966,6 +3982,7 @@ static void iscsit_get_rx_pdu(struct iscsi_conn *conn)
>>   		 * Ensure that both TX and RX per connection kthreads
>>   		 * are scheduled to run on the same CPU.
>>   		 */
>> +		iscsit_thread_reschedule(conn);
>>   		iscsit_thread_check_cpumask(conn, current, 0);
>>   
>>   		memset(&iov, 0, sizeof(struct kvec));
>> diff --git a/drivers/target/iscsi/iscsi_target.h b/drivers/target/iscsi/iscsi_target.h
>> index b35a96ded9c1..cb97a316d76d 100644
>> --- a/drivers/target/iscsi/iscsi_target.h
>> +++ b/drivers/target/iscsi/iscsi_target.h
>> @@ -57,4 +57,21 @@ extern struct kmem_cache *lio_r2t_cache;
>>   extern struct ida sess_ida;
>>   extern struct mutex auth_id_lock;
>>   
>> +extern cpumask_t __iscsi_allowed_cpumask;
> I would rename to:
>
> iscsit_allowed_cpumask
>
I will rename to iscsit_allowed_cpumas.
>> +
>> +static inline void iscsit_thread_reschedule(struct iscsi_conn *conn)
>> +{
>> +	/*
>> +	 * If __iscsi_allowed_cpumask modified, reschedule iSCSI connection's
>> +	 * RX/TX threads update conn->allowed_cpumask.
>> +	 */
>> +	if (!cpumask_equal(&__iscsi_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,
>> +			&__iscsi_allowed_cpumask);
> Fix up formatting like above.
I will reformat.
>
>> +	}
>> +}
>> +
>>   #endif   /*** ISCSI_TARGET_H ***/
>> diff --git a/drivers/target/iscsi/iscsi_target_configfs.c b/drivers/target/iscsi/iscsi_target_configfs.c
>> index 2a9de24a8bbe..dc12b1695838 100644
>> --- a/drivers/target/iscsi/iscsi_target_configfs.c
>> +++ b/drivers/target/iscsi/iscsi_target_configfs.c
>> @@ -1127,8 +1127,42 @@ static ssize_t lio_target_wwn_lio_version_show(struct config_item *item,
>>   
>>   CONFIGFS_ATTR_RO(lio_target_wwn_, lio_version);
>>   
>> +cpumask_t __iscsi_allowed_cpumask;
> Maybe better to put this in iscsi_target.c with the other vars like
> it.

Originally I wanted to put it in struct iscsit_global and use it in 
iscsit_thread_check_cpumask.

However iscsit_thread_check_cpumask is also called in cxgbit_target.c. I 
don't know if I also

need to modify cxgbit at the same time, and I only modified two calls in 
iscsi_target.c. I would

like to know how to handle in cxgbit_target.c?

I want to move 'static inline void iscsit_thread_check_cpumask' from 
iscsi_target_core.h to

iscsi_target.c and EXPORT_SYMBOL(iscsit_thread_check_cpumask). Do you 
agree it?

>
>> +
>> +static ssize_t lio_target_wwn_cpus_allowed_list_show(
>> +		struct config_item *item, char *page)
>> +{
>> +	return sprintf(page, "%*pbl\n",
>> +		cpumask_pr_args(&__iscsi_allowed_cpumask));
> Formatting like above.
I will reformat.
>
>> +}
>> +
>> +static ssize_t lio_target_wwn_cpus_allowed_list_store(
>> +		struct config_item *item, const char *page, size_t count)
>> +{
>> +	int ret;
>> +	char *orig;
>> +	cpumask_t new_allowed_cpumask;
>> +
>> +	orig = kstrdup(page, GFP_KERNEL);
>> +	if (!orig)
>> +		return -ENOMEM;
>> +
>> +	cpumask_clear(&new_allowed_cpumask);
>> +	ret = cpulist_parse(orig, &new_allowed_cpumask);
> Are you supposed to do a  before
> using new_allowed_cpumask? It looks like other callers are doing it.

Other callers used cpumask_var_t, but the new_allowed_cpumask is cpumask_t.

I think just call cpumask_clear(&new_allowed_cpumask).


typedef struct cpumask { DECLARE_BITMAP(bits, NR_CPUS); } cpumask_t;

typedef struct cpumask cpumask_var_t[1];

>
>> +
>> +	kfree(orig);
>> +	if (ret != 0)
>> +		return ret;
>> +
>> +	cpumask_copy(&__iscsi_allowed_cpumask, &new_allowed_cpumask);
>> +	return count;
>> +}
>> +
>> +CONFIGFS_ATTR(lio_target_wwn_, cpus_allowed_list);
>> +
>>   static struct configfs_attribute *lio_target_wwn_attrs[] = {
>>   	&lio_target_wwn_attr_lio_version,
>> +	&lio_target_wwn_attr_cpus_allowed_list,
>>   	NULL,
>>   };
>>   
>> diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c
>> index 1a9c50401bdb..910f35e4648a 100644
>> --- a/drivers/target/iscsi/iscsi_target_login.c
>> +++ b/drivers/target/iscsi/iscsi_target_login.c
>> @@ -1129,8 +1129,15 @@ static struct iscsi_conn *iscsit_alloc_conn(struct iscsi_np *np)
>>   		goto free_conn_ops;
>>   	}
>>   
>> +	if (!zalloc_cpumask_var(&conn->allowed_cpumask, GFP_KERNEL)) {
>> +		pr_err("Unable to allocate conn->allowed_cpumask\n");
>> +		goto free_conn_cpumask;
>> +	}
> I think in iscsit_free_conn you need a:
>
> free_cpumask_var(conn->allowed_cpumask);
>
> to go with this allocation.
This is bug. I will fix up in v2.
>
>> +
>>   	return conn;
>>   
>> +free_conn_cpumask:
>> +	free_cpumask_var(conn->allowed_cpumask);
> I think you wanted conn->conn_cpumask here.
This is bug. I will fix up in v2.
>>   free_conn_ops:
>>   	kfree(conn->conn_ops);
>>   put_transport:
>> diff --git a/include/target/iscsi/iscsi_target_core.h b/include/target/iscsi/iscsi_target_core.h
>> index 1eccb2ac7d02..c5e9cad187cf 100644
>> --- a/include/target/iscsi/iscsi_target_core.h
>> +++ b/include/target/iscsi/iscsi_target_core.h
>> @@ -580,6 +580,7 @@ struct iscsi_conn {
>>   	struct ahash_request	*conn_tx_hash;
>>   	/* Used for scheduling TX and RX connection kthreads */
>>   	cpumask_var_t		conn_cpumask;
>> +	cpumask_var_t		allowed_cpumask;
>>   	unsigned int		conn_rx_reset_cpumask:1;
>>   	unsigned int		conn_tx_reset_cpumask:1;
>>   	/* list_head of struct iscsi_cmd for this connection */
>

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

[Mike Christie]

On 2/9/22 5:48 AM, Zou Mingzhe wrote:
>>>   +cpumask_t __iscsi_allowed_cpumask;
>> Maybe better to put this in iscsi_target.c with the other vars like
>> it.
> 
> Originally I wanted to put it in struct iscsit_global and use it in iscsit_thread_check_cpumask.
> 
> However iscsit_thread_check_cpumask is also called in cxgbit_target.c. I don't know if I also
> 
> need to modify cxgbit at the same time, and I only modified two calls in iscsi_target.c. I would
> 

The new configfs file shows up for software iscsi and cxgb right? If so I think you need to
modify both, or you end up with a file that returns success but doesn't do anything and it's
confusing to users. It's also a simple change to cxgb.


> like to know how to handle in cxgbit_target.c?
> 
> I want to move 'static inline void iscsit_thread_check_cpumask' from iscsi_target_core.h to
> 
> iscsi_target.c and EXPORT_SYMBOL(iscsit_thread_check_cpumask). Do you agree it?
> 

I think that is ok.

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

[mingzhe.zou]

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);
+}
+EXPORT_SYMBOL(iscsit_thread_check_cpumask);
+
 int
 iscsit_immediate_queue(struct iscsi_conn *conn, struct iscsi_cmd *cmd, int state)
 {
diff --git a/drivers/target/iscsi/iscsi_target_configfs.c b/drivers/target/iscsi/iscsi_target_configfs.c
index 2a9de24a8bbe..0cedcfe207b5 100644
--- a/drivers/target/iscsi/iscsi_target_configfs.c
+++ b/drivers/target/iscsi/iscsi_target_configfs.c
@@ -1127,8 +1127,40 @@ static ssize_t lio_target_wwn_lio_version_show(struct config_item *item,
 
 CONFIGFS_ATTR_RO(lio_target_wwn_, lio_version);
 
+static ssize_t lio_target_wwn_cpus_allowed_list_show(
+		struct config_item *item, char *page)
+{
+	return sprintf(page, "%*pbl\n",
+		       cpumask_pr_args(iscsit_global->allowed_cpumask));
+}
+
+static ssize_t lio_target_wwn_cpus_allowed_list_store(
+		struct config_item *item, const char *page, size_t count)
+{
+	int ret;
+	char *orig;
+	cpumask_t new_allowed_cpumask;
+
+	orig = kstrdup(page, GFP_KERNEL);
+	if (!orig)
+		return -ENOMEM;
+
+	cpumask_clear(&new_allowed_cpumask);
+	ret = cpulist_parse(orig, &new_allowed_cpumask);
+
+	kfree(orig);
+	if (ret != 0)
+		return ret;
+
+	cpumask_copy(iscsit_global->allowed_cpumask, &new_allowed_cpumask);
+	return count;
+}
+
+CONFIGFS_ATTR(lio_target_wwn_, cpus_allowed_list);
+
 static struct configfs_attribute *lio_target_wwn_attrs[] = {
 	&lio_target_wwn_attr_lio_version,
+	&lio_target_wwn_attr_cpus_allowed_list,
 	NULL,
 };
 
diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c
index 1a9c50401bdb..9c01fb864585 100644
--- a/drivers/target/iscsi/iscsi_target_login.c
+++ b/drivers/target/iscsi/iscsi_target_login.c
@@ -1129,8 +1129,15 @@ static struct iscsi_conn *iscsit_alloc_conn(struct iscsi_np *np)
 		goto free_conn_ops;
 	}
 
+	if (!zalloc_cpumask_var(&conn->allowed_cpumask, GFP_KERNEL)) {
+		pr_err("Unable to allocate conn->allowed_cpumask\n");
+		goto free_conn_cpumask;
+	}
+
 	return conn;
 
+free_conn_cpumask:
+	free_cpumask_var(conn->conn_cpumask);
 free_conn_ops:
 	kfree(conn->conn_ops);
 put_transport:
@@ -1142,6 +1149,7 @@ static struct iscsi_conn *iscsit_alloc_conn(struct iscsi_np *np)
 
 void iscsit_free_conn(struct iscsi_conn *conn)
 {
+	free_cpumask_var(conn->allowed_cpumask);
 	free_cpumask_var(conn->conn_cpumask);
 	kfree(conn->conn_ops);
 	iscsit_put_transport(conn->conn_transport);
diff --git a/include/target/iscsi/iscsi_target_core.h b/include/target/iscsi/iscsi_target_core.h
index 1eccb2ac7d02..adc87de0362b 100644
--- a/include/target/iscsi/iscsi_target_core.h
+++ b/include/target/iscsi/iscsi_target_core.h
@@ -580,6 +580,7 @@ struct iscsi_conn {
 	struct ahash_request	*conn_tx_hash;
 	/* Used for scheduling TX and RX connection kthreads */
 	cpumask_var_t		conn_cpumask;
+	cpumask_var_t		allowed_cpumask;
 	unsigned int		conn_rx_reset_cpumask:1;
 	unsigned int		conn_tx_reset_cpumask:1;
 	/* list_head of struct iscsi_cmd for this connection */
@@ -878,6 +879,7 @@ struct iscsit_global {
 	/* Thread Set bitmap pointer */
 	unsigned long		*ts_bitmap;
 	spinlock_t		ts_bitmap_lock;
+	cpumask_var_t		allowed_cpumask;
 	/* Used for iSCSI discovery session authentication */
 	struct iscsi_node_acl	discovery_acl;
 	struct iscsi_portal_group	*discovery_tpg;
@@ -898,29 +900,8 @@ static inline u32 session_get_next_ttt(struct iscsi_session *session)
 
 extern struct iscsi_cmd *iscsit_find_cmd_from_itt(struct iscsi_conn *, itt_t);
 
-static inline void iscsit_thread_check_cpumask(
-	struct iscsi_conn *conn,
-	struct task_struct *p,
-	int mode)
-{
-	/*
-	 * 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);
-}
+extern void iscsit_thread_check_cpumask(struct iscsi_conn *conn,
+					struct task_struct *p,
+					int mode);
+
 #endif /* ISCSI_TARGET_CORE_H */
-- 
2.17.1

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

[Zou Mingzhe]

Hi, christie

Can you help me review v2

Thanks

在 2022/2/17 15:45, mingzhe.zou@easystack.cn 写道:
> 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);
> +}
> +EXPORT_SYMBOL(iscsit_thread_check_cpumask);
> +
>   int
>   iscsit_immediate_queue(struct iscsi_conn *conn, struct iscsi_cmd *cmd, int state)
>   {
> diff --git a/drivers/target/iscsi/iscsi_target_configfs.c b/drivers/target/iscsi/iscsi_target_configfs.c
> index 2a9de24a8bbe..0cedcfe207b5 100644
> --- a/drivers/target/iscsi/iscsi_target_configfs.c
> +++ b/drivers/target/iscsi/iscsi_target_configfs.c
> @@ -1127,8 +1127,40 @@ static ssize_t lio_target_wwn_lio_version_show(struct config_item *item,
>   
>   CONFIGFS_ATTR_RO(lio_target_wwn_, lio_version);
>   
> +static ssize_t lio_target_wwn_cpus_allowed_list_show(
> +		struct config_item *item, char *page)
> +{
> +	return sprintf(page, "%*pbl\n",
> +		       cpumask_pr_args(iscsit_global->allowed_cpumask));
> +}
> +
> +static ssize_t lio_target_wwn_cpus_allowed_list_store(
> +		struct config_item *item, const char *page, size_t count)
> +{
> +	int ret;
> +	char *orig;
> +	cpumask_t new_allowed_cpumask;
> +
> +	orig = kstrdup(page, GFP_KERNEL);
> +	if (!orig)
> +		return -ENOMEM;
> +
> +	cpumask_clear(&new_allowed_cpumask);
> +	ret = cpulist_parse(orig, &new_allowed_cpumask);
> +
> +	kfree(orig);
> +	if (ret != 0)
> +		return ret;
> +
> +	cpumask_copy(iscsit_global->allowed_cpumask, &new_allowed_cpumask);
> +	return count;
> +}
> +
> +CONFIGFS_ATTR(lio_target_wwn_, cpus_allowed_list);
> +
>   static struct configfs_attribute *lio_target_wwn_attrs[] = {
>   	&lio_target_wwn_attr_lio_version,
> +	&lio_target_wwn_attr_cpus_allowed_list,
>   	NULL,
>   };
>   
> diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c
> index 1a9c50401bdb..9c01fb864585 100644
> --- a/drivers/target/iscsi/iscsi_target_login.c
> +++ b/drivers/target/iscsi/iscsi_target_login.c
> @@ -1129,8 +1129,15 @@ static struct iscsi_conn *iscsit_alloc_conn(struct iscsi_np *np)
>   		goto free_conn_ops;
>   	}
>   
> +	if (!zalloc_cpumask_var(&conn->allowed_cpumask, GFP_KERNEL)) {
> +		pr_err("Unable to allocate conn->allowed_cpumask\n");
> +		goto free_conn_cpumask;
> +	}
> +
>   	return conn;
>   
> +free_conn_cpumask:
> +	free_cpumask_var(conn->conn_cpumask);
>   free_conn_ops:
>   	kfree(conn->conn_ops);
>   put_transport:
> @@ -1142,6 +1149,7 @@ static struct iscsi_conn *iscsit_alloc_conn(struct iscsi_np *np)
>   
>   void iscsit_free_conn(struct iscsi_conn *conn)
>   {
> +	free_cpumask_var(conn->allowed_cpumask);
>   	free_cpumask_var(conn->conn_cpumask);
>   	kfree(conn->conn_ops);
>   	iscsit_put_transport(conn->conn_transport);
> diff --git a/include/target/iscsi/iscsi_target_core.h b/include/target/iscsi/iscsi_target_core.h
> index 1eccb2ac7d02..adc87de0362b 100644
> --- a/include/target/iscsi/iscsi_target_core.h
> +++ b/include/target/iscsi/iscsi_target_core.h
> @@ -580,6 +580,7 @@ struct iscsi_conn {
>   	struct ahash_request	*conn_tx_hash;
>   	/* Used for scheduling TX and RX connection kthreads */
>   	cpumask_var_t		conn_cpumask;
> +	cpumask_var_t		allowed_cpumask;
>   	unsigned int		conn_rx_reset_cpumask:1;
>   	unsigned int		conn_tx_reset_cpumask:1;
>   	/* list_head of struct iscsi_cmd for this connection */
> @@ -878,6 +879,7 @@ struct iscsit_global {
>   	/* Thread Set bitmap pointer */
>   	unsigned long		*ts_bitmap;
>   	spinlock_t		ts_bitmap_lock;
> +	cpumask_var_t		allowed_cpumask;
>   	/* Used for iSCSI discovery session authentication */
>   	struct iscsi_node_acl	discovery_acl;
>   	struct iscsi_portal_group	*discovery_tpg;
> @@ -898,29 +900,8 @@ static inline u32 session_get_next_ttt(struct iscsi_session *session)
>   
>   extern struct iscsi_cmd *iscsit_find_cmd_from_itt(struct iscsi_conn *, itt_t);
>   
> -static inline void iscsit_thread_check_cpumask(
> -	struct iscsi_conn *conn,
> -	struct task_struct *p,
> -	int mode)
> -{
> -	/*
> -	 * 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);
> -}
> +extern void iscsit_thread_check_cpumask(struct iscsi_conn *conn,
> +					struct task_struct *p,
> +					int mode);
> +
>   #endif /* ISCSI_TARGET_CORE_H */

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

[michael.christie]

On 2/23/22 12:24 AM, Zou Mingzhe wrote:
> Hi, christie
> 
> Can you help me review v2
> 

I'm working on it.

It looks like is a window where we can have the recv and xmit thread
on different CPUs. I thought there might be some iscsi target code that
relied on them being on the same CPU so I'm just trying to review the
iscsi target code paths related to that to make sure.


> Thanks
> 
> 在 2022/2/17 15:45, mingzhe.zou@easystack.cn 写道:
>> 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);
>> +}
>> +EXPORT_SYMBOL(iscsit_thread_check_cpumask);
>> +
>>   int
>>   iscsit_immediate_queue(struct iscsi_conn *conn, struct iscsi_cmd *cmd, int state)
>>   {
>> diff --git a/drivers/target/iscsi/iscsi_target_configfs.c b/drivers/target/iscsi/iscsi_target_configfs.c
>> index 2a9de24a8bbe..0cedcfe207b5 100644
>> --- a/drivers/target/iscsi/iscsi_target_configfs.c
>> +++ b/drivers/target/iscsi/iscsi_target_configfs.c
>> @@ -1127,8 +1127,40 @@ static ssize_t lio_target_wwn_lio_version_show(struct config_item *item,
>>     CONFIGFS_ATTR_RO(lio_target_wwn_, lio_version);
>>   +static ssize_t lio_target_wwn_cpus_allowed_list_show(
>> +        struct config_item *item, char *page)
>> +{
>> +    return sprintf(page, "%*pbl\n",
>> +               cpumask_pr_args(iscsit_global->allowed_cpumask));
>> +}
>> +
>> +static ssize_t lio_target_wwn_cpus_allowed_list_store(
>> +        struct config_item *item, const char *page, size_t count)
>> +{
>> +    int ret;
>> +    char *orig;
>> +    cpumask_t new_allowed_cpumask;
>> +
>> +    orig = kstrdup(page, GFP_KERNEL);
>> +    if (!orig)
>> +        return -ENOMEM;
>> +
>> +    cpumask_clear(&new_allowed_cpumask);
>> +    ret = cpulist_parse(orig, &new_allowed_cpumask);
>> +
>> +    kfree(orig);
>> +    if (ret != 0)
>> +        return ret;
>> +
>> +    cpumask_copy(iscsit_global->allowed_cpumask, &new_allowed_cpumask);
>> +    return count;
>> +}
>> +
>> +CONFIGFS_ATTR(lio_target_wwn_, cpus_allowed_list);
>> +
>>   static struct configfs_attribute *lio_target_wwn_attrs[] = {
>>       &lio_target_wwn_attr_lio_version,
>> +    &lio_target_wwn_attr_cpus_allowed_list,
>>       NULL,
>>   };
>>   diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c
>> index 1a9c50401bdb..9c01fb864585 100644
>> --- a/drivers/target/iscsi/iscsi_target_login.c
>> +++ b/drivers/target/iscsi/iscsi_target_login.c
>> @@ -1129,8 +1129,15 @@ static struct iscsi_conn *iscsit_alloc_conn(struct iscsi_np *np)
>>           goto free_conn_ops;
>>       }
>>   +    if (!zalloc_cpumask_var(&conn->allowed_cpumask, GFP_KERNEL)) {
>> +        pr_err("Unable to allocate conn->allowed_cpumask\n");
>> +        goto free_conn_cpumask;
>> +    }
>> +
>>       return conn;
>>   +free_conn_cpumask:
>> +    free_cpumask_var(conn->conn_cpumask);
>>   free_conn_ops:
>>       kfree(conn->conn_ops);
>>   put_transport:
>> @@ -1142,6 +1149,7 @@ static struct iscsi_conn *iscsit_alloc_conn(struct iscsi_np *np)
>>     void iscsit_free_conn(struct iscsi_conn *conn)
>>   {
>> +    free_cpumask_var(conn->allowed_cpumask);
>>       free_cpumask_var(conn->conn_cpumask);
>>       kfree(conn->conn_ops);
>>       iscsit_put_transport(conn->conn_transport);
>> diff --git a/include/target/iscsi/iscsi_target_core.h b/include/target/iscsi/iscsi_target_core.h
>> index 1eccb2ac7d02..adc87de0362b 100644
>> --- a/include/target/iscsi/iscsi_target_core.h
>> +++ b/include/target/iscsi/iscsi_target_core.h
>> @@ -580,6 +580,7 @@ struct iscsi_conn {
>>       struct ahash_request    *conn_tx_hash;
>>       /* Used for scheduling TX and RX connection kthreads */
>>       cpumask_var_t        conn_cpumask;
>> +    cpumask_var_t        allowed_cpumask;
>>       unsigned int        conn_rx_reset_cpumask:1;
>>       unsigned int        conn_tx_reset_cpumask:1;
>>       /* list_head of struct iscsi_cmd for this connection */
>> @@ -878,6 +879,7 @@ struct iscsit_global {
>>       /* Thread Set bitmap pointer */
>>       unsigned long        *ts_bitmap;
>>       spinlock_t        ts_bitmap_lock;
>> +    cpumask_var_t        allowed_cpumask;
>>       /* Used for iSCSI discovery session authentication */
>>       struct iscsi_node_acl    discovery_acl;
>>       struct iscsi_portal_group    *discovery_tpg;
>> @@ -898,29 +900,8 @@ static inline u32 session_get_next_ttt(struct iscsi_session *session)
>>     extern struct iscsi_cmd *iscsit_find_cmd_from_itt(struct iscsi_conn *, itt_t);
>>   -static inline void iscsit_thread_check_cpumask(
>> -    struct iscsi_conn *conn,
>> -    struct task_struct *p,
>> -    int mode)
>> -{
>> -    /*
>> -     * 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);
>> -}
>> +extern void iscsit_thread_check_cpumask(struct iscsi_conn *conn,
>> +                    struct task_struct *p,
>> +                    int mode);
>> +
>>   #endif /* ISCSI_TARGET_CORE_H */

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

[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?

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.

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.

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

[Zou Mingzhe]

在 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.
>

[PATCH v3] target: add iscsi/cpus_allowed_list in configfs

[mingzhe.zou]

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          | 77 +++++++++++++++++++-
 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, 120 insertions(+), 28 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 2c54c5d8412d..6fe6a6bab3f4 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,62 @@ 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)
+{
+	/*
+	 * The TX and RX threads maybe call iscsit_thread_check_cpumask()
+	 * at the same time. The RX thread might be faster and return from
+	 * iscsit_thread_reschedule() with conn_rx_reset_cpumask set to 0.
+	 * Then the TX thread 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() again and set it to 0.
+	 */
+	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;
+	} else {
+		if (!conn->conn_rx_reset_cpumask)
+			return;
+	}
+
+	/*
+	 * 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);
+	if (mode == 1)
+		conn->conn_tx_reset_cpumask = 0;
+	else
+		conn->conn_rx_reset_cpumask = 0;
+}
+EXPORT_SYMBOL(iscsit_thread_check_cpumask);
+
 int
 iscsit_immediate_queue(struct iscsi_conn *conn, struct iscsi_cmd *cmd, int state)
 {
diff --git a/drivers/target/iscsi/iscsi_target_configfs.c b/drivers/target/iscsi/iscsi_target_configfs.c
index 2a9de24a8bbe..0cedcfe207b5 100644
--- a/drivers/target/iscsi/iscsi_target_configfs.c
+++ b/drivers/target/iscsi/iscsi_target_configfs.c
@@ -1127,8 +1127,40 @@ static ssize_t lio_target_wwn_lio_version_show(struct config_item *item,
 
 CONFIGFS_ATTR_RO(lio_target_wwn_, lio_version);
 
+static ssize_t lio_target_wwn_cpus_allowed_list_show(
+		struct config_item *item, char *page)
+{
+	return sprintf(page, "%*pbl\n",
+		       cpumask_pr_args(iscsit_global->allowed_cpumask));
+}
+
+static ssize_t lio_target_wwn_cpus_allowed_list_store(
+		struct config_item *item, const char *page, size_t count)
+{
+	int ret;
+	char *orig;
+	cpumask_t new_allowed_cpumask;
+
+	orig = kstrdup(page, GFP_KERNEL);
+	if (!orig)
+		return -ENOMEM;
+
+	cpumask_clear(&new_allowed_cpumask);
+	ret = cpulist_parse(orig, &new_allowed_cpumask);
+
+	kfree(orig);
+	if (ret != 0)
+		return ret;
+
+	cpumask_copy(iscsit_global->allowed_cpumask, &new_allowed_cpumask);
+	return count;
+}
+
+CONFIGFS_ATTR(lio_target_wwn_, cpus_allowed_list);
+
 static struct configfs_attribute *lio_target_wwn_attrs[] = {
 	&lio_target_wwn_attr_lio_version,
+	&lio_target_wwn_attr_cpus_allowed_list,
 	NULL,
 };
 
diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c
index 1a9c50401bdb..9c01fb864585 100644
--- a/drivers/target/iscsi/iscsi_target_login.c
+++ b/drivers/target/iscsi/iscsi_target_login.c
@@ -1129,8 +1129,15 @@ static struct iscsi_conn *iscsit_alloc_conn(struct iscsi_np *np)
 		goto free_conn_ops;
 	}
 
+	if (!zalloc_cpumask_var(&conn->allowed_cpumask, GFP_KERNEL)) {
+		pr_err("Unable to allocate conn->allowed_cpumask\n");
+		goto free_conn_cpumask;
+	}
+
 	return conn;
 
+free_conn_cpumask:
+	free_cpumask_var(conn->conn_cpumask);
 free_conn_ops:
 	kfree(conn->conn_ops);
 put_transport:
@@ -1142,6 +1149,7 @@ static struct iscsi_conn *iscsit_alloc_conn(struct iscsi_np *np)
 
 void iscsit_free_conn(struct iscsi_conn *conn)
 {
+	free_cpumask_var(conn->allowed_cpumask);
 	free_cpumask_var(conn->conn_cpumask);
 	kfree(conn->conn_ops);
 	iscsit_put_transport(conn->conn_transport);
diff --git a/include/target/iscsi/iscsi_target_core.h b/include/target/iscsi/iscsi_target_core.h
index 1eccb2ac7d02..adc87de0362b 100644
--- a/include/target/iscsi/iscsi_target_core.h
+++ b/include/target/iscsi/iscsi_target_core.h
@@ -580,6 +580,7 @@ struct iscsi_conn {
 	struct ahash_request	*conn_tx_hash;
 	/* Used for scheduling TX and RX connection kthreads */
 	cpumask_var_t		conn_cpumask;
+	cpumask_var_t		allowed_cpumask;
 	unsigned int		conn_rx_reset_cpumask:1;
 	unsigned int		conn_tx_reset_cpumask:1;
 	/* list_head of struct iscsi_cmd for this connection */
@@ -878,6 +879,7 @@ struct iscsit_global {
 	/* Thread Set bitmap pointer */
 	unsigned long		*ts_bitmap;
 	spinlock_t		ts_bitmap_lock;
+	cpumask_var_t		allowed_cpumask;
 	/* Used for iSCSI discovery session authentication */
 	struct iscsi_node_acl	discovery_acl;
 	struct iscsi_portal_group	*discovery_tpg;
@@ -898,29 +900,8 @@ static inline u32 session_get_next_ttt(struct iscsi_session *session)
 
 extern struct iscsi_cmd *iscsit_find_cmd_from_itt(struct iscsi_conn *, itt_t);
 
-static inline void iscsit_thread_check_cpumask(
-	struct iscsi_conn *conn,
-	struct task_struct *p,
-	int mode)
-{
-	/*
-	 * 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);
-}
+extern void iscsit_thread_check_cpumask(struct iscsi_conn *conn,
+					struct task_struct *p,
+					int mode);
+
 #endif /* ISCSI_TARGET_CORE_H */
-- 
2.17.1

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

[Zou Mingzhe]

Hi, christie

Have you started reviewing this patch?

Mingzhe

在 2022/3/1 15:55, mingzhe.zou@easystack.cn 写道:
> 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          | 77 +++++++++++++++++++-
>   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, 120 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
> index 2c54c5d8412d..6fe6a6bab3f4 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,62 @@ 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)
> +{
> +	/*
> +	 * The TX and RX threads maybe call iscsit_thread_check_cpumask()
> +	 * at the same time. The RX thread might be faster and return from
> +	 * iscsit_thread_reschedule() with conn_rx_reset_cpumask set to 0.
> +	 * Then the TX thread 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() again and set it to 0.
> +	 */
> +	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;
> +	} else {
> +		if (!conn->conn_rx_reset_cpumask)
> +			return;
> +	}
> +
> +	/*
> +	 * 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);
> +	if (mode == 1)
> +		conn->conn_tx_reset_cpumask = 0;
> +	else
> +		conn->conn_rx_reset_cpumask = 0;
> +}
> +EXPORT_SYMBOL(iscsit_thread_check_cpumask);
> +
>   int
>   iscsit_immediate_queue(struct iscsi_conn *conn, struct iscsi_cmd *cmd, int state)
>   {
> diff --git a/drivers/target/iscsi/iscsi_target_configfs.c b/drivers/target/iscsi/iscsi_target_configfs.c
> index 2a9de24a8bbe..0cedcfe207b5 100644
> --- a/drivers/target/iscsi/iscsi_target_configfs.c
> +++ b/drivers/target/iscsi/iscsi_target_configfs.c
> @@ -1127,8 +1127,40 @@ static ssize_t lio_target_wwn_lio_version_show(struct config_item *item,
>   
>   CONFIGFS_ATTR_RO(lio_target_wwn_, lio_version);
>   
> +static ssize_t lio_target_wwn_cpus_allowed_list_show(
> +		struct config_item *item, char *page)
> +{
> +	return sprintf(page, "%*pbl\n",
> +		       cpumask_pr_args(iscsit_global->allowed_cpumask));
> +}
> +
> +static ssize_t lio_target_wwn_cpus_allowed_list_store(
> +		struct config_item *item, const char *page, size_t count)
> +{
> +	int ret;
> +	char *orig;
> +	cpumask_t new_allowed_cpumask;
> +
> +	orig = kstrdup(page, GFP_KERNEL);
> +	if (!orig)
> +		return -ENOMEM;
> +
> +	cpumask_clear(&new_allowed_cpumask);
> +	ret = cpulist_parse(orig, &new_allowed_cpumask);
> +
> +	kfree(orig);
> +	if (ret != 0)
> +		return ret;
> +
> +	cpumask_copy(iscsit_global->allowed_cpumask, &new_allowed_cpumask);
> +	return count;
> +}
> +
> +CONFIGFS_ATTR(lio_target_wwn_, cpus_allowed_list);
> +
>   static struct configfs_attribute *lio_target_wwn_attrs[] = {
>   	&lio_target_wwn_attr_lio_version,
> +	&lio_target_wwn_attr_cpus_allowed_list,
>   	NULL,
>   };
>   
> diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c
> index 1a9c50401bdb..9c01fb864585 100644
> --- a/drivers/target/iscsi/iscsi_target_login.c
> +++ b/drivers/target/iscsi/iscsi_target_login.c
> @@ -1129,8 +1129,15 @@ static struct iscsi_conn *iscsit_alloc_conn(struct iscsi_np *np)
>   		goto free_conn_ops;
>   	}
>   
> +	if (!zalloc_cpumask_var(&conn->allowed_cpumask, GFP_KERNEL)) {
> +		pr_err("Unable to allocate conn->allowed_cpumask\n");
> +		goto free_conn_cpumask;
> +	}
> +
>   	return conn;
>   
> +free_conn_cpumask:
> +	free_cpumask_var(conn->conn_cpumask);
>   free_conn_ops:
>   	kfree(conn->conn_ops);
>   put_transport:
> @@ -1142,6 +1149,7 @@ static struct iscsi_conn *iscsit_alloc_conn(struct iscsi_np *np)
>   
>   void iscsit_free_conn(struct iscsi_conn *conn)
>   {
> +	free_cpumask_var(conn->allowed_cpumask);
>   	free_cpumask_var(conn->conn_cpumask);
>   	kfree(conn->conn_ops);
>   	iscsit_put_transport(conn->conn_transport);
> diff --git a/include/target/iscsi/iscsi_target_core.h b/include/target/iscsi/iscsi_target_core.h
> index 1eccb2ac7d02..adc87de0362b 100644
> --- a/include/target/iscsi/iscsi_target_core.h
> +++ b/include/target/iscsi/iscsi_target_core.h
> @@ -580,6 +580,7 @@ struct iscsi_conn {
>   	struct ahash_request	*conn_tx_hash;
>   	/* Used for scheduling TX and RX connection kthreads */
>   	cpumask_var_t		conn_cpumask;
> +	cpumask_var_t		allowed_cpumask;
>   	unsigned int		conn_rx_reset_cpumask:1;
>   	unsigned int		conn_tx_reset_cpumask:1;
>   	/* list_head of struct iscsi_cmd for this connection */
> @@ -878,6 +879,7 @@ struct iscsit_global {
>   	/* Thread Set bitmap pointer */
>   	unsigned long		*ts_bitmap;
>   	spinlock_t		ts_bitmap_lock;
> +	cpumask_var_t		allowed_cpumask;
>   	/* Used for iSCSI discovery session authentication */
>   	struct iscsi_node_acl	discovery_acl;
>   	struct iscsi_portal_group	*discovery_tpg;
> @@ -898,29 +900,8 @@ static inline u32 session_get_next_ttt(struct iscsi_session *session)
>   
>   extern struct iscsi_cmd *iscsit_find_cmd_from_itt(struct iscsi_conn *, itt_t);
>   
> -static inline void iscsit_thread_check_cpumask(
> -	struct iscsi_conn *conn,
> -	struct task_struct *p,
> -	int mode)
> -{
> -	/*
> -	 * 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);
> -}
> +extern void iscsit_thread_check_cpumask(struct iscsi_conn *conn,
> +					struct task_struct *p,
> +					int mode);
> +
>   #endif /* ISCSI_TARGET_CORE_H */

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

[Mike Christie]

On 3/1/22 1:55 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.
> 

Reviewed-by: Mike Christie <michael.christie@oracle.com>

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

[Martin K. Petersen]

Mingzhe,

> 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.

Applied to 5.18/scsi-staging, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

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

[Martin K. Petersen]

On Tue, 1 Mar 2022 15:55:00 +0800, 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.
> 
> [...]

Applied to 5.18/scsi-queue, thanks!

[1/1] target: add iscsi/cpus_allowed_list in configfs
      https://git.kernel.org/mkp/scsi/c/d72d827f2f26

-- 
Martin K. Petersen	Oracle Linux Engineering