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

Zou Mingzhe
2022-03-01T07:35:10Z

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