All of lore.kernel.org
 help / color / mirror / Atom feed
From: Matthew Wilcox <willy@infradead.org>
To: Bart Van Assche <Bart.VanAssche@wdc.com>
Cc: "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
	"virtualization@lists.linux-foundation.org" 
	<virtualization@lists.linux-foundation.org>,
	"kent.overstreet@gmail.com" <kent.overstreet@gmail.com>,
	"linux1394-devel@lists.sourceforge.net" 
	<linux1394-devel@lists.sourceforge.net>,
	"jgross@suse.com" <jgross@suse.com>,
	"axboe@kernel.dk" <axboe@kernel.dk>,
	"linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>,
	"qla2xxx-upstream@qlogic.com" <qla2xxx-upstream@qlogic.com>,
	"target-devel@vger.kernel.org" <target-devel@vger.kernel.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"mawilcox@microsoft.com" <mawilcox@microsoft.com>
Subject: Re: [PATCH 1/2] Convert target drivers to use sbitmap
Date: Tue, 12 Jun 2018 09:15:26 -0700	[thread overview]
Message-ID: <20180612161526.GE19433@bombadil.infradead.org> (raw)
In-Reply-To: <da5220a5ed4bed210c31a7517389e787a3b1a01f.camel@wdc.com>

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
> > --- 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)

WARNING: multiple messages have this Message-ID (diff)
From: Matthew Wilcox <willy@infradead.org>
To: Bart Van Assche <Bart.VanAssche@wdc.com>
Cc: "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
	"virtualization@lists.linux-foundation.org"
	<virtualization@lists.linux-foundation.org>,
	"kent.overstreet@gmail.com" <kent.overstreet@gmail.com>,
	"linux1394-devel@lists.sourceforge.net"
	<linux1394-devel@lists.sourceforge.net>,
	"jgross@suse.com" <jgross@suse.com>,
	"axboe@kernel.dk" <axboe@kernel.dk>,
	"linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>,
	"qla2xxx-upstream@qlogic.com" <qla2xxx-upstream@qlogic.com>,
	"target-devel@vger.kernel.org" <target-devel@vger.kernel.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"mawilcox@microsoft.com" <mawilcox@microsoft.com>
Subject: Re: [PATCH 1/2] Convert target drivers to use sbitmap
Date: Tue, 12 Jun 2018 16:15:26 +0000	[thread overview]
Message-ID: <20180612161526.GE19433@bombadil.infradead.org> (raw)
In-Reply-To: <da5220a5ed4bed210c31a7517389e787a3b1a01f.camel@wdc.com>

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
> > --- 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)

WARNING: multiple messages have this Message-ID (diff)
From: Matthew Wilcox <willy@infradead.org>
To: Bart Van Assche <Bart.VanAssche@wdc.com>
Cc: "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
	"virtualization@lists.linux-foundation.org"
	<virtualization@lists.linux-foundation.org>,
	"kent.overstreet@gmail.com" <kent.overstreet@gmail.com>,
	"linux1394-devel@lists.sourceforge.net"
	<linux1394-devel@lists.sourceforge.net>,
	"jgross@suse.com" <jgross@suse.com>,
	"axboe@kernel.dk" <axboe@kernel.dk>,
	"linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>,
	"qla2xxx-upstream@qlogic.com" <qla2xxx-upstream@qlogic.com>,
	"target-devel@vger.kernel.org" <target-devel@vger.kernel.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"mawilcox@microsoft.com" <mawilcox@microsoft.com>
Subject: [1/2] Convert target drivers to use sbitmap
Date: Tue, 12 Jun 2018 09:15:26 -0700	[thread overview]
Message-ID: <20180612161526.GE19433@bombadil.infradead.org> (raw)

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
> > --- 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)
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

  parent reply	other threads:[~2018-06-12 16:15 UTC|newest]

Thread overview: 56+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-15 16:00 [PATCH 0/2] Use sbitmap instead of percpu_ida Matthew Wilcox
2018-05-15 16:00 ` Matthew Wilcox
2018-05-15 16:00 ` Matthew Wilcox
2018-05-15 16:00 ` [PATCH 1/2] Convert target drivers to use sbitmap Matthew Wilcox
2018-05-15 16:00 ` Matthew Wilcox
2018-05-15 16:00   ` [1/2] " Matthew Wilcox
2018-05-15 16:00   ` [PATCH 1/2] " Matthew Wilcox
2018-05-15 16:00   ` Matthew Wilcox
2018-05-15 16:11   ` Jens Axboe
2018-05-15 16:11     ` [1/2] " Jens Axboe
2018-05-15 16:11     ` [PATCH 1/2] " Jens Axboe
2018-05-15 16:20     ` Jens Axboe
2018-05-15 16:20     ` Jens Axboe
2018-05-15 16:20       ` [1/2] " Jens Axboe
2018-05-15 16:20       ` [PATCH 1/2] " Jens Axboe
2018-06-12  1:18     ` Jens Axboe
2018-06-12  1:18       ` [1/2] " Jens Axboe
2018-06-12  1:18       ` [PATCH 1/2] " Jens Axboe
2018-06-12  3:06       ` Matthew Wilcox
2018-06-12  3:06         ` [1/2] " Matthew Wilcox
2018-06-12  3:06         ` [PATCH 1/2] " Matthew Wilcox
2018-06-12  3:06         ` Matthew Wilcox
2018-05-15 16:11   ` Jens Axboe
2018-05-16  5:54   ` Felipe Balbi
2018-05-16  5:54     ` [1/2] " Felipe Balbi
2018-05-16  5:54     ` [PATCH 1/2] " Felipe Balbi
2018-05-16 12:47   ` kbuild test robot
2018-05-16 12:47   ` kbuild test robot
2018-05-16 12:47     ` [1/2] " kbuild test robot
2018-05-16 12:47     ` [PATCH 1/2] " kbuild test robot
2018-05-16 12:47   ` [RFC PATCH] iscsit_wait_for_tag() can be static kbuild test robot
2018-05-16 12:47   ` kbuild test robot
2018-05-16 12:47     ` [RFC] " kbuild test robot
2018-05-16 12:47     ` [RFC PATCH] " kbuild test robot
2018-05-16 12:47     ` kbuild test robot
2018-06-12 15:22   ` [PATCH 1/2] Convert target drivers to use sbitmap Bart Van Assche
2018-06-12 15:22     ` [1/2] " Bart Van Assche
2018-06-12 15:22     ` [PATCH 1/2] " Bart Van Assche
2018-06-12 15:22     ` Bart Van Assche
2018-06-12 16:15     ` Matthew Wilcox
2018-06-12 16:15     ` Matthew Wilcox [this message]
2018-06-12 16:15       ` [1/2] " Matthew Wilcox
2018-06-12 16:15       ` [PATCH 1/2] " Matthew Wilcox
2018-06-12 16:15       ` Matthew Wilcox
2018-06-12 16:32       ` Bart Van Assche
2018-06-12 16:32         ` [1/2] " Bart Van Assche
2018-06-12 16:32         ` [PATCH 1/2] " Bart Van Assche
2018-06-12 16:32         ` Bart Van Assche
2018-06-12 18:08         ` Matthew Wilcox
2018-06-12 18:08           ` [1/2] " Matthew Wilcox
2018-06-12 18:08           ` [PATCH 1/2] " Matthew Wilcox
2018-06-12 18:08           ` Matthew Wilcox
2018-05-15 16:00 ` [PATCH 2/2] Remove percpu_ida Matthew Wilcox
2018-05-15 16:00 ` Matthew Wilcox
2018-05-15 16:00   ` [2/2] " Matthew Wilcox
2018-05-15 16:00   ` [PATCH 2/2] " Matthew Wilcox

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180612161526.GE19433@bombadil.infradead.org \
    --to=willy@infradead.org \
    --cc=Bart.VanAssche@wdc.com \
    --cc=axboe@kernel.dk \
    --cc=jgross@suse.com \
    --cc=kent.overstreet@gmail.com \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-scsi@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=linux1394-devel@lists.sourceforge.net \
    --cc=mawilcox@microsoft.com \
    --cc=netdev@vger.kernel.org \
    --cc=qla2xxx-upstream@qlogic.com \
    --cc=target-devel@vger.kernel.org \
    --cc=virtualization@lists.linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.