| Message ID | 20220217074511.9696-1-mingzhe.zou@easystack.cn |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] target: add iscsi/cpus_allowed_list in configfs | expand |
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 */
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 */
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.
在 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. >
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 */