On 5/15/18 10:00 AM, Matthew Wilcox wrote:> From: Matthew Wilcox <mawilcox at microsoft.com> > > 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 <linux/list.h> > -#include <linux/percpu_ida.h> > +#include <linux/sched/signal.h> > #include <net/ipv6.h> /* ipv6_addr_equal() */ > #include <scsi/scsi_tcq.h> > #include <scsi/iscsi_proto.h> > @@ -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
On 5/15/18 10:11 AM, Jens Axboe wrote:> On 5/15/18 10:00 AM, Matthew Wilcox wrote: >> From: Matthew Wilcox <mawilcox at microsoft.com> >> >> 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
On 5/15/18 10:11 AM, Jens Axboe wrote:> On 5/15/18 10:00 AM, Matthew Wilcox wrote: >> From: Matthew Wilcox <mawilcox at microsoft.com> >> >> 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 <linux/list.h> >> -#include <linux/percpu_ida.h> >> +#include <linux/sched/signal.h> >> #include <net/ipv6.h> /* ipv6_addr_equal() */ >> #include <scsi/scsi_tcq.h> >> #include <scsi/iscsi_proto.h> >> @@ -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