search for: sbitmap_queue_get

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

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

...a->tgt.sess_lock, flags); @@ -4215,9 +4217,9 @@ static struct qla_tgt_cmd *qlt_get_tag(scsi_qla_host_t *vha, { struct se_session *se_sess = sess->se_sess; struct qla_tgt_cmd *cmd; - int tag; + int tag, cpu; - tag = percpu_ida_alloc(&se_sess->sess_tag_pool, TASK_RUNNING); + tag = sbitmap_queue_get(&se_sess->sess_tag_pool, &cpu); if (tag < 0) return NULL; @@ -4230,6 +4232,7 @@ static struct qla_tgt_cmd *qlt_get_tag(scsi_qla_host_t *vha, qlt_incr_num_pend_cmds(vha); cmd->vha = vha; cmd->se_cmd.map_tag = tag; + cmd->se_cmd.map_cpu = cpu; cmd->sess = sess...

From: Matthew Wilcox <mawilcox at microsoft.com> This is a pretty rough-and-ready conversion of the target drivers from using percpu_ida to sbitmap. It compiles; I don't have a target setup, so it's completely untested. I haven't tried to do anything particularly clever here, so it's possible that, for example, the wait queue in iscsi_target_util could be more clever, like

...t;> + 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...

Removing the percpu_ida code nets over 400 lines of removal. It's not as spectacular as deleting an entire architecture, but it's still a worthy reduction in lines of code. Untested due to lack of hardware and not understanding how to set up a target platform. Changes from v1: - Fixed bugs pointed out by Jens in iscsit_wait_for_tag() - Abstracted out tag freeing as requested by Bart

Removing the percpu_ida code nets over 400 lines of removal. It's not as spectacular as deleting an entire architecture, but it's still a worthy reduction in lines of code. Untested due to lack of hardware and not understanding how to set up a target platform. Changes from v1: - Fixed bugs pointed out by Jens in iscsit_wait_for_tag() - Abstracted out tag freeing as requested by Bart

On Tue, 2018-06-12 at 09:15 -0700, Matthew Wilcox wrote: > On Tue, Jun 12, 2018 at 03:22:42PM +0000, 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 > > > ---

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

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

...ges; > int tag, cpu; > @@ -582,15 +587,14 @@ static void vhost_scsi_complete_cmd_work(struct vhost_work *work) > pr_err("Unable to locate active struct vhost_scsi_nexus\n"); > return ERR_PTR(-EIO); > } > - se_sess = tv_nexus->tvn_se_sess; > > - tag = sbitmap_queue_get(&se_sess->sess_tag_pool, &cpu); > + tag = sbitmap_get(&svq->scsi_tags, 0, false); > if (tag < 0) { > pr_err("Unable to obtain tag for vhost_scsi_cmd\n"); > return ERR_PTR(-ENOMEM); > } After this change, cpu is uninitialized. > > -...