Re: [PATCH 1/2] Convert target drivers to use sbitmap
On Tue, Jun 12, 2018 at 04:32:03PM +, Bart Van Assche wrote: > On Tue, 2018-06-12 at 09:15 -0700, Matthew Wilcox wrote: > > On Tue, Jun 12, 2018 at 03:22:42PM +, Bart Van Assche wrote: > > > Please introduce functions in the target core for allocating and freeing > > > a tag > > > instead of spreading the knowledge of how to allocate and free tags over > > > all > > > target drivers. > > > > I can't without doing an unreasonably large amount of work on drivers that > > I have no way to test. Some of the drivers have the se_cmd already; some > > of them don't. I'd be happy to introduce a common function for freeing > > a tag. > > Which target drivers are you referring to? If you are referring to the sbp > driver: > I think that driver is dead and can be removed from the kernel tree. I even > don't > know whether that driver ever has had any users other than the developer of > that > driver. For example tcm_fc: tag = sbitmap_queue_get(&se_sess->sess_tag_pool, &cpu); if (tag < 0) goto busy; cmd = &((struct ft_cmd *)se_sess->sess_cmd_map)[tag]; or qla2xxx: tag = sbitmap_queue_get(&se_sess->sess_tag_pool, &cpu); if (tag < 0) return NULL; cmd = &((struct qla_tgt_cmd *)se_sess->sess_cmd_map)[tag]; The core doesn't know at what offset from the pointer to store the tag & cpu. Only the individual drivers know their cmd layout. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/2] Convert target drivers to use sbitmap
On Tue, 2018-06-12 at 09:15 -0700, Matthew Wilcox wrote: > On Tue, Jun 12, 2018 at 03:22:42PM +, Bart Van Assche wrote: > > On Tue, 2018-05-15 at 09:00 -0700, Matthew Wilcox wrote: > > > diff --git a/drivers/scsi/qla2xxx/qla_target.c > > > b/drivers/scsi/qla2xxx/qla_target.c > > > index 025dc2d3f3de..cdf671c2af61 100644 > > > --- a/drivers/scsi/qla2xxx/qla_target.c > > > +++ b/drivers/scsi/qla2xxx/qla_target.c > > > @@ -3719,7 +3719,8 @@ void qlt_free_cmd(struct qla_tgt_cmd *cmd) > > > return; > > > } > > > cmd->jiffies_at_free = get_jiffies_64(); > > > - percpu_ida_free(&sess->se_sess->sess_tag_pool, cmd->se_cmd.map_tag); > > > + sbitmap_queue_clear(&sess->se_sess->sess_tag_pool, cmd->se_cmd.map_tag, > > > + cmd->se_cmd.map_cpu); > > > } > > > EXPORT_SYMBOL(qlt_free_cmd); > > > > Please introduce functions in the target core for allocating and freeing a > > tag > > instead of spreading the knowledge of how to allocate and free tags over all > > target drivers. > > I can't without doing an unreasonably large amount of work on drivers that > I have no way to test. Some of the drivers have the se_cmd already; some > of them don't. I'd be happy to introduce a common function for freeing > a tag. Which target drivers are you referring to? If you are referring to the sbp driver: I think that driver is dead and can be removed from the kernel tree. I even don't know whether that driver ever has had any users other than the developer of that driver. > > This looks weird to me. Shouldn't target code ignore signals instead of > > causing > > tag allocation to fail if a signal is received? > > It's what the current code did: > > - if (signal_pending_state(state, current)) { > - tag = -ERESTARTSYS; > - break; > - } > > and the current callers literally indicate that they want signals: > > drivers/infiniband/ulp/isert/ib_isert.c:cmd = > iscsit_allocate_cmd(conn, TASK_INTERRUPTIBLE); > drivers/target/iscsi/iscsi_target.c:cmd = iscsit_allocate_cmd(conn, > TASK_INTERRUPTIBLE); Right, the iSCSI target driver uses signals to wake up threads (see also the send_sig() calls in the iSCSI target code). Bart. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/2] Convert target drivers to use sbitmap
On Tue, Jun 12, 2018 at 03:22:42PM +, Bart Van Assche wrote: > On Tue, 2018-05-15 at 09:00 -0700, Matthew Wilcox wrote: > > diff --git a/drivers/scsi/qla2xxx/qla_target.c > > b/drivers/scsi/qla2xxx/qla_target.c > > index 025dc2d3f3de..cdf671c2af61 100644 > > --- a/drivers/scsi/qla2xxx/qla_target.c > > +++ b/drivers/scsi/qla2xxx/qla_target.c > > @@ -3719,7 +3719,8 @@ void qlt_free_cmd(struct qla_tgt_cmd *cmd) > > return; > > } > > cmd->jiffies_at_free = get_jiffies_64(); > > - percpu_ida_free(&sess->se_sess->sess_tag_pool, cmd->se_cmd.map_tag); > > + sbitmap_queue_clear(&sess->se_sess->sess_tag_pool, cmd->se_cmd.map_tag, > > + cmd->se_cmd.map_cpu); > > } > > EXPORT_SYMBOL(qlt_free_cmd); > > Please introduce functions in the target core for allocating and freeing a tag > instead of spreading the knowledge of how to allocate and free tags over all > target drivers. I can't without doing an unreasonably large amount of work on drivers that I have no way to test. Some of the drivers have the se_cmd already; some of them don't. I'd be happy to introduce a common function for freeing a tag. > > +int iscsit_wait_for_tag(struct se_session *se_sess, int state, int *cpup) > > +{ > > + int tag = -1; > > + DEFINE_WAIT(wait); > > + struct sbq_wait_state *ws; > > + > > + if (state == TASK_RUNNING) > > + return tag; > > + > > + ws = &se_sess->sess_tag_pool.ws[0]; > > + for (;;) { > > + prepare_to_wait_exclusive(&ws->wait, &wait, state); > > + if (signal_pending_state(state, current)) > > + break; > > This looks weird to me. Shouldn't target code ignore signals instead of > causing > tag allocation to fail if a signal is received? It's what the current code did: - if (signal_pending_state(state, current)) { - tag = -ERESTARTSYS; - break; - } and the current callers literally indicate that they want signals: drivers/infiniband/ulp/isert/ib_isert.c:cmd = iscsit_allocate_cmd(conn, TASK_INTERRUPTIBLE); drivers/target/iscsi/iscsi_target.c:cmd = iscsit_allocate_cmd(conn, TASK_INTERRUPTIBLE); (etc) ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/2] Convert target drivers to use sbitmap
On Tue, 2018-05-15 at 09:00 -0700, Matthew Wilcox wrote: > diff --git a/drivers/scsi/qla2xxx/qla_target.c > b/drivers/scsi/qla2xxx/qla_target.c > index 025dc2d3f3de..cdf671c2af61 100644 > --- a/drivers/scsi/qla2xxx/qla_target.c > +++ b/drivers/scsi/qla2xxx/qla_target.c > @@ -3719,7 +3719,8 @@ void qlt_free_cmd(struct qla_tgt_cmd *cmd) > return; > } > cmd->jiffies_at_free = get_jiffies_64(); > - percpu_ida_free(&sess->se_sess->sess_tag_pool, cmd->se_cmd.map_tag); > + sbitmap_queue_clear(&sess->se_sess->sess_tag_pool, cmd->se_cmd.map_tag, > + cmd->se_cmd.map_cpu); > } > EXPORT_SYMBOL(qlt_free_cmd); Please introduce functions in the target core for allocating and freeing a tag instead of spreading the knowledge of how to allocate and free tags over all target drivers. > +int iscsit_wait_for_tag(struct se_session *se_sess, int state, int *cpup) > +{ > + int tag = -1; > + DEFINE_WAIT(wait); > + struct sbq_wait_state *ws; > + > + if (state == TASK_RUNNING) > + return tag; > + > + ws = &se_sess->sess_tag_pool.ws[0]; > + for (;;) { > + prepare_to_wait_exclusive(&ws->wait, &wait, state); > + if (signal_pending_state(state, current)) > + break; This looks weird to me. Shouldn't target code ignore signals instead of causing tag allocation to fail if a signal is received? > + schedule(); > + tag = sbitmap_queue_get(&se_sess->sess_tag_pool, cpup); > + } > + > + finish_wait(&ws->wait, &wait); > + return tag; > +} Thanks, Bart. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/2] Convert target drivers to use sbitmap
On Mon, Jun 11, 2018 at 07:18:55PM -0600, Jens Axboe wrote: > Are you going to push this further? I really think we should. Yes, I'll resubmit it tomorrow. Sorry for dropping the ball on this one. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/2] Convert target drivers to use sbitmap
On 5/15/18 10:11 AM, Jens Axboe wrote: > On 5/15/18 10:00 AM, Matthew Wilcox wrote: >> From: Matthew Wilcox >> >> The sbitmap and the percpu_ida perform essentially the same task, >> allocating tags for commands. Since the sbitmap is more used than >> the percpu_ida, convert the percpu_ida users to the sbitmap API. > > It should also be the same performance as percpu_ida in light use, and > performs much better at > 50% utilization of the tag space. I think > that's better justification than "more used than". > >> diff --git a/drivers/target/iscsi/iscsi_target_util.c >> b/drivers/target/iscsi/iscsi_target_util.c >> index 4435bf374d2d..28bcffae609f 100644 >> --- a/drivers/target/iscsi/iscsi_target_util.c >> +++ b/drivers/target/iscsi/iscsi_target_util.c >> @@ -17,7 +17,7 @@ >> >> **/ >> >> #include >> -#include >> +#include >> #include /* ipv6_addr_equal() */ >> #include >> #include >> @@ -147,6 +147,28 @@ void iscsit_free_r2ts_from_list(struct iscsi_cmd *cmd) >> spin_unlock_bh(&cmd->r2t_lock); >> } >> >> +int iscsit_wait_for_tag(struct se_session *se_sess, int state, int *cpup) >> +{ >> +int tag = -1; >> +DEFINE_WAIT(wait); >> +struct sbq_wait_state *ws; >> + >> +if (state == TASK_RUNNING) >> +return tag; >> + >> +ws = &se_sess->sess_tag_pool.ws[0]; >> +for (;;) { >> +prepare_to_wait_exclusive(&ws->wait, &wait, state); >> +if (signal_pending_state(state, current)) >> +break; >> +schedule(); >> +tag = sbitmap_queue_get(&se_sess->sess_tag_pool, cpup); >> +} >> + >> +finish_wait(&ws->wait, &wait); >> +return tag; >> +} > > Seems like that should be: > > > ws = &se_sess->sess_tag_pool.ws[0]; > for (;;) { > prepare_to_wait_exclusive(&ws->wait, &wait, state); > if (signal_pending_state(state, current)) > break; > tag = sbitmap_queue_get(&se_sess->sess_tag_pool, cpup); > if (tag != -1) > break; > schedule(); > } > > finish_wait(&ws->wait, &wait); > return tag; > >> /* >> * May be called from software interrupt (timer) context for allocating >> * iSCSI NopINs. >> @@ -155,9 +177,11 @@ struct iscsi_cmd *iscsit_allocate_cmd(struct iscsi_conn >> *conn, int state) >> { >> struct iscsi_cmd *cmd; >> struct se_session *se_sess = conn->sess->se_sess; >> -int size, tag; >> +int size, tag, cpu; >> >> -tag = percpu_ida_alloc(&se_sess->sess_tag_pool, state); >> +tag = sbitmap_queue_get(&se_sess->sess_tag_pool, &cpu); >> +if (tag < 0) >> +tag = iscsit_wait_for_tag(se_sess, state, &cpu); >> if (tag < 0) >> return NULL; > > Might make sense to just roll the whole thing into iscsi_get_tag(), that > would be cleaner. > > sbitmap should provide a helper for that, but we can do that cleanup > later. That would encapsulate things like the per-cpu caching hint too, > for instance. > > Rest looks fine to me. Are you going to push this further? I really think we should. -- Jens Axboe ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/2] Convert target drivers to use sbitmap
Hi Matthew, I love your patch! Perhaps something to improve: [auto build test WARNING on linus/master] [also build test WARNING on v4.17-rc5 next-20180516] [cannot apply to target/master] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Matthew-Wilcox/Use-sbitmap-instead-of-percpu_ida/20180516-143658 reproduce: # apt-get install sparse make ARCH=x86_64 allmodconfig make C=1 CF=-D__CHECK_ENDIAN__ sparse warnings: (new ones prefixed by >>) >> drivers/target/iscsi/iscsi_target_util.c:150:5: sparse: symbol >> 'iscsit_wait_for_tag' was not declared. Should it be static? drivers/target/iscsi/iscsi_target_util.c:1174:31: sparse: expression using sizeof(void) drivers/target/iscsi/iscsi_target_util.c:1174:31: sparse: expression using sizeof(void) Please review and possibly fold the followup patch. --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/2] Convert target drivers to use sbitmap
On 5/15/18 10:11 AM, Jens Axboe wrote: > On 5/15/18 10:00 AM, Matthew Wilcox wrote: >> From: Matthew Wilcox >> >> The sbitmap and the percpu_ida perform essentially the same task, >> allocating tags for commands. Since the sbitmap is more used than >> the percpu_ida, convert the percpu_ida users to the sbitmap API. > > It should also be the same performance as percpu_ida in light use, and > performs much better at > 50% utilization of the tag space. I think > that's better justification than "more used than". Had to search long and hard for the perf numbers I did for percpu_ida on higher utilization, but here it is: https://lkml.org/lkml/2014/4/22/553 -- Jens Axboe ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/2] Convert target drivers to use sbitmap
On 5/15/18 10:00 AM, Matthew Wilcox wrote: > From: Matthew Wilcox > > The sbitmap and the percpu_ida perform essentially the same task, > allocating tags for commands. Since the sbitmap is more used than > the percpu_ida, convert the percpu_ida users to the sbitmap API. It should also be the same performance as percpu_ida in light use, and performs much better at > 50% utilization of the tag space. I think that's better justification than "more used than". > diff --git a/drivers/target/iscsi/iscsi_target_util.c > b/drivers/target/iscsi/iscsi_target_util.c > index 4435bf374d2d..28bcffae609f 100644 > --- a/drivers/target/iscsi/iscsi_target_util.c > +++ b/drivers/target/iscsi/iscsi_target_util.c > @@ -17,7 +17,7 @@ > > **/ > > #include > -#include > +#include > #include /* ipv6_addr_equal() */ > #include > #include > @@ -147,6 +147,28 @@ void iscsit_free_r2ts_from_list(struct iscsi_cmd *cmd) > spin_unlock_bh(&cmd->r2t_lock); > } > > +int iscsit_wait_for_tag(struct se_session *se_sess, int state, int *cpup) > +{ > + int tag = -1; > + DEFINE_WAIT(wait); > + struct sbq_wait_state *ws; > + > + if (state == TASK_RUNNING) > + return tag; > + > + ws = &se_sess->sess_tag_pool.ws[0]; > + for (;;) { > + prepare_to_wait_exclusive(&ws->wait, &wait, state); > + if (signal_pending_state(state, current)) > + break; > + schedule(); > + tag = sbitmap_queue_get(&se_sess->sess_tag_pool, cpup); > + } > + > + finish_wait(&ws->wait, &wait); > + return tag; > +} Seems like that should be: ws = &se_sess->sess_tag_pool.ws[0]; for (;;) { prepare_to_wait_exclusive(&ws->wait, &wait, state); if (signal_pending_state(state, current)) break; tag = sbitmap_queue_get(&se_sess->sess_tag_pool, cpup); if (tag != -1) break; schedule(); } finish_wait(&ws->wait, &wait); return tag; > /* > * May be called from software interrupt (timer) context for allocating > * iSCSI NopINs. > @@ -155,9 +177,11 @@ struct iscsi_cmd *iscsit_allocate_cmd(struct iscsi_conn > *conn, int state) > { > struct iscsi_cmd *cmd; > struct se_session *se_sess = conn->sess->se_sess; > - int size, tag; > + int size, tag, cpu; > > - tag = percpu_ida_alloc(&se_sess->sess_tag_pool, state); > + tag = sbitmap_queue_get(&se_sess->sess_tag_pool, &cpu); > + if (tag < 0) > + tag = iscsit_wait_for_tag(se_sess, state, &cpu); > if (tag < 0) > return NULL; Might make sense to just roll the whole thing into iscsi_get_tag(), that would be cleaner. sbitmap should provide a helper for that, but we can do that cleanup later. That would encapsulate things like the per-cpu caching hint too, for instance. Rest looks fine to me. -- Jens Axboe ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization