Linux-NVME Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] nvme-tcp: Remove access to sk->sk_ll_usec
@ 2019-10-10 15:06 Sebastian Andrzej Siewior
  2019-10-10 15:07 ` Christoph Hellwig
  0 siblings, 1 reply; 11+ messages in thread
From: Sebastian Andrzej Siewior @ 2019-10-10 15:06 UTC (permalink / raw)
  To: linux-nvme
  Cc: Keith Busch, Jens Axboe, tglx, Christoph Hellwig, Sagi Grimberg

The access to sk->sk_ll_usec should be hidden behind
CONFIG_NET_RX_BUSY_POLL.
This shouldn't be required because this can also be set via
	setsockopt(,, SO_BUSY_POLL, &one, )

Remove access to sk_ll_usec behind CONFIG_NET_RX_BUSY_POLL.

Fixes: 1a9460cef5711 ("nvme-tcp: support simple polling")
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/nvme/host/tcp.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 385a5212c10f1..dbad6327e84bc 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -1386,7 +1386,6 @@ static int nvme_tcp_alloc_queue(struct nvme_ctrl *nctrl,
 	queue->sock->sk->sk_data_ready = nvme_tcp_data_ready;
 	queue->sock->sk->sk_state_change = nvme_tcp_state_change;
 	queue->sock->sk->sk_write_space = nvme_tcp_write_space;
-	queue->sock->sk->sk_ll_usec = 1;
 	write_unlock_bh(&queue->sock->sk->sk_callback_lock);
 
 	return 0;
-- 
2.23.0


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] nvme-tcp: Remove access to sk->sk_ll_usec
  2019-10-10 15:06 [PATCH] nvme-tcp: Remove access to sk->sk_ll_usec Sebastian Andrzej Siewior
@ 2019-10-10 15:07 ` Christoph Hellwig
  2019-10-10 15:10   ` Sebastian Andrzej Siewior
  0 siblings, 1 reply; 11+ messages in thread
From: Christoph Hellwig @ 2019-10-10 15:07 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: Sagi Grimberg, linux-nvme, Jens Axboe, Keith Busch, tglx,
	Christoph Hellwig

On Thu, Oct 10, 2019 at 05:06:22PM +0200, Sebastian Andrzej Siewior wrote:
> The access to sk->sk_ll_usec should be hidden behind
> CONFIG_NET_RX_BUSY_POLL.
> This shouldn't be required because this can also be set via
> 	setsockopt(,, SO_BUSY_POLL, &one, )
> 
> Remove access to sk_ll_usec behind CONFIG_NET_RX_BUSY_POLL.

The changelog doesn't match the patch, which just removes the assignment
unconditionally.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] nvme-tcp: Remove access to sk->sk_ll_usec
  2019-10-10 15:07 ` Christoph Hellwig
@ 2019-10-10 15:10   ` Sebastian Andrzej Siewior
  2019-10-10 15:14     ` Christoph Hellwig
  0 siblings, 1 reply; 11+ messages in thread
From: Sebastian Andrzej Siewior @ 2019-10-10 15:10 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Keith Busch, Jens Axboe, tglx, Sagi Grimberg, linux-nvme

On 2019-10-10 17:07:19 [+0200], Christoph Hellwig wrote:
> The changelog doesn't match the patch, which just removes the assignment
> unconditionally.

I said that it has to be behind CONFIG_.* _but_ shouldn't be needed due
to the setsockopt() thingy. Isn't this working? If not I can put it
behind the ifdef. Be aware that the setsockopt() returns an error if
this polling is not available.

Sebastian

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] nvme-tcp: Remove access to sk->sk_ll_usec
  2019-10-10 15:10   ` Sebastian Andrzej Siewior
@ 2019-10-10 15:14     ` Christoph Hellwig
  2019-10-10 15:29       ` Sebastian Andrzej Siewior
  0 siblings, 1 reply; 11+ messages in thread
From: Christoph Hellwig @ 2019-10-10 15:14 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: Sagi Grimberg, linux-nvme, Jens Axboe, Keith Busch, tglx,
	Christoph Hellwig

On Thu, Oct 10, 2019 at 05:10:23PM +0200, Sebastian Andrzej Siewior wrote:
> On 2019-10-10 17:07:19 [+0200], Christoph Hellwig wrote:
> > The changelog doesn't match the patch, which just removes the assignment
> > unconditionally.
> 
> I said that it has to be behind CONFIG_.* _but_ shouldn't be needed due
> to the setsockopt() thingy. Isn't this working? If not I can put it
> behind the ifdef. Be aware that the setsockopt() returns an error if
> this polling is not available.

I didn't really understand that comment, or how it applies to the
nvme code, given that it never calls setsockopt with SO_BUSY_POLL.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] nvme-tcp: Remove access to sk->sk_ll_usec
  2019-10-10 15:14     ` Christoph Hellwig
@ 2019-10-10 15:29       ` Sebastian Andrzej Siewior
  2019-10-10 15:29         ` Christoph Hellwig
  0 siblings, 1 reply; 11+ messages in thread
From: Sebastian Andrzej Siewior @ 2019-10-10 15:29 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Keith Busch, Jens Axboe, tglx, Sagi Grimberg, linux-nvme

On 2019-10-10 17:14:44 [+0200], Christoph Hellwig wrote:
> I didn't really understand that comment, or how it applies to the
> nvme code, given that it never calls setsockopt with SO_BUSY_POLL.

So that part of the code is never accessed from userland?

Sebastian

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] nvme-tcp: Remove access to sk->sk_ll_usec
  2019-10-10 15:29       ` Sebastian Andrzej Siewior
@ 2019-10-10 15:29         ` Christoph Hellwig
  2019-10-10 15:34           ` [PATCH v2] nvme-tcp: Initialize sk->sk_ll_usec only with NET_RX_BUSY_POLL Sebastian Andrzej Siewior
  0 siblings, 1 reply; 11+ messages in thread
From: Christoph Hellwig @ 2019-10-10 15:29 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: Sagi Grimberg, linux-nvme, Jens Axboe, Keith Busch, tglx,
	Christoph Hellwig

On Thu, Oct 10, 2019 at 05:29:24PM +0200, Sebastian Andrzej Siewior wrote:
> On 2019-10-10 17:14:44 [+0200], Christoph Hellwig wrote:
> > I didn't really understand that comment, or how it applies to the
> > nvme code, given that it never calls setsockopt with SO_BUSY_POLL.
> 
> So that part of the code is never accessed from userland?

No, the code uses sockets from kernel space.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2] nvme-tcp: Initialize sk->sk_ll_usec only with NET_RX_BUSY_POLL
  2019-10-10 15:29         ` Christoph Hellwig
@ 2019-10-10 15:34           ` Sebastian Andrzej Siewior
  2019-10-10 17:08             ` Wunderlich, Mark
  2019-10-14  7:16             ` Christoph Hellwig
  0 siblings, 2 replies; 11+ messages in thread
From: Sebastian Andrzej Siewior @ 2019-10-10 15:34 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Keith Busch, Jens Axboe, tglx, Sagi Grimberg, linux-nvme

The access to sk->sk_ll_usec should be hidden behind
CONFIG_NET_RX_BUSY_POLL like the definition of sk_ll_usec.

Put access to ->sk_ll_usec behind CONFIG_NET_RX_BUSY_POLL.

Fixes: 1a9460cef5711 ("nvme-tcp: support simple polling")
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
v1…v2: Hide it behind CONFIG_NET_RX_BUSY_POLL instead removing it.

 drivers/nvme/host/tcp.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 385a5212c10f1..72059561b7f2a 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -1386,7 +1386,9 @@ static int nvme_tcp_alloc_queue(struct nvme_ctrl *nctrl,
 	queue->sock->sk->sk_data_ready = nvme_tcp_data_ready;
 	queue->sock->sk->sk_state_change = nvme_tcp_state_change;
 	queue->sock->sk->sk_write_space = nvme_tcp_write_space;
+#ifdef CONFIG_NET_RX_BUSY_POLL
 	queue->sock->sk->sk_ll_usec = 1;
+#endif
 	write_unlock_bh(&queue->sock->sk->sk_callback_lock);
 
 	return 0;
-- 
2.23.0


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH v2] nvme-tcp: Initialize sk->sk_ll_usec only with NET_RX_BUSY_POLL
  2019-10-10 15:34           ` [PATCH v2] nvme-tcp: Initialize sk->sk_ll_usec only with NET_RX_BUSY_POLL Sebastian Andrzej Siewior
@ 2019-10-10 17:08             ` Wunderlich, Mark
  2019-10-10 17:19               ` Christoph Hellwig
  2019-10-14  7:16             ` Christoph Hellwig
  1 sibling, 1 reply; 11+ messages in thread
From: Wunderlich, Mark @ 2019-10-10 17:08 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior, Christoph Hellwig
  Cc: Keith Busch, Jens Axboe, tglx, Sagi Grimberg, linux-nvme

Should there also be a check here to honor any non-zero value of sk_ll_usec ( via sysctl_net_busy_read ), before setting here to hard coded default of 1?
Or maybe best to have this as a separate patch to consider?

+#ifdef CONFIG_NET_RX_BUSY_POLL
	If (!queue->sock->sk->sk_ll_usec)
 		queue->sock->sk->sk_ll_usec = 1;
+#endif

-----Original Message-----
From: Linux-nvme <linux-nvme-bounces@lists.infradead.org> On Behalf Of Sebastian Andrzej Siewior
Sent: Thursday, October 10, 2019 8:34 AM
To: Christoph Hellwig <hch@lst.de>
Cc: Keith Busch <kbusch@kernel.org>; Jens Axboe <axboe@fb.com>; tglx@linutronix.de; Sagi Grimberg <sagi@grimberg.me>; linux-nvme@lists.infradead.org
Subject: [PATCH v2] nvme-tcp: Initialize sk->sk_ll_usec only with NET_RX_BUSY_POLL

The access to sk->sk_ll_usec should be hidden behind CONFIG_NET_RX_BUSY_POLL like the definition of sk_ll_usec.

Put access to ->sk_ll_usec behind CONFIG_NET_RX_BUSY_POLL.

Fixes: 1a9460cef5711 ("nvme-tcp: support simple polling")
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
v1…v2: Hide it behind CONFIG_NET_RX_BUSY_POLL instead removing it.

 drivers/nvme/host/tcp.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c index 385a5212c10f1..72059561b7f2a 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -1386,7 +1386,9 @@ static int nvme_tcp_alloc_queue(struct nvme_ctrl *nctrl,
 	queue->sock->sk->sk_data_ready = nvme_tcp_data_ready;
 	queue->sock->sk->sk_state_change = nvme_tcp_state_change;
 	queue->sock->sk->sk_write_space = nvme_tcp_write_space;
+#ifdef CONFIG_NET_RX_BUSY_POLL
 	queue->sock->sk->sk_ll_usec = 1;
+#endif
 	write_unlock_bh(&queue->sock->sk->sk_callback_lock);
 
 	return 0;
--
2.23.0


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] nvme-tcp: Initialize sk->sk_ll_usec only with NET_RX_BUSY_POLL
  2019-10-10 17:08             ` Wunderlich, Mark
@ 2019-10-10 17:19               ` Christoph Hellwig
  0 siblings, 0 replies; 11+ messages in thread
From: Christoph Hellwig @ 2019-10-10 17:19 UTC (permalink / raw)
  To: Wunderlich, Mark
  Cc: Sagi Grimberg, Sebastian Andrzej Siewior, linux-nvme, Jens Axboe,
	Keith Busch, tglx, Christoph Hellwig

On Thu, Oct 10, 2019 at 05:08:27PM +0000, Wunderlich, Mark wrote:
> Should there also be a check here to honor any non-zero value of sk_ll_usec ( via sysctl_net_busy_read ), before setting here to hard coded default of 1?
> Or maybe best to have this as a separate patch to consider?

Let's keep that separate.  If you want to enhance that area, we probably
shouldn't build nvme_tcp_poll at all without CONFIG_NET_RX_BUSY_POLL as
well.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] nvme-tcp: Initialize sk->sk_ll_usec only with NET_RX_BUSY_POLL
  2019-10-10 15:34           ` [PATCH v2] nvme-tcp: Initialize sk->sk_ll_usec only with NET_RX_BUSY_POLL Sebastian Andrzej Siewior
  2019-10-10 17:08             ` Wunderlich, Mark
@ 2019-10-14  7:16             ` Christoph Hellwig
  2019-10-14 15:06               ` Keith Busch
  1 sibling, 1 reply; 11+ messages in thread
From: Christoph Hellwig @ 2019-10-14  7:16 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: Sagi Grimberg, linux-nvme, Jens Axboe, Keith Busch, tglx,
	Christoph Hellwig

On Thu, Oct 10, 2019 at 05:34:12PM +0200, Sebastian Andrzej Siewior wrote:
> The access to sk->sk_ll_usec should be hidden behind
> CONFIG_NET_RX_BUSY_POLL like the definition of sk_ll_usec.
> 
> Put access to ->sk_ll_usec behind CONFIG_NET_RX_BUSY_POLL.
> 
> Fixes: 1a9460cef5711 ("nvme-tcp: support simple polling")
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] nvme-tcp: Initialize sk->sk_ll_usec only with NET_RX_BUSY_POLL
  2019-10-14  7:16             ` Christoph Hellwig
@ 2019-10-14 15:06               ` Keith Busch
  0 siblings, 0 replies; 11+ messages in thread
From: Keith Busch @ 2019-10-14 15:06 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Jens Axboe, Sebastian Andrzej Siewior, tglx, Sagi Grimberg, linux-nvme

On Mon, Oct 14, 2019 at 09:16:51AM +0200, Christoph Hellwig wrote:
> On Thu, Oct 10, 2019 at 05:34:12PM +0200, Sebastian Andrzej Siewior wrote:
> > The access to sk->sk_ll_usec should be hidden behind
> > CONFIG_NET_RX_BUSY_POLL like the definition of sk_ll_usec.
> > 
> > Put access to ->sk_ll_usec behind CONFIG_NET_RX_BUSY_POLL.
> > 
> > Fixes: 1a9460cef5711 ("nvme-tcp: support simple polling")
> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> 
> Looks good,
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Thanks, applied to nvme-5.4.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, back to index

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-10 15:06 [PATCH] nvme-tcp: Remove access to sk->sk_ll_usec Sebastian Andrzej Siewior
2019-10-10 15:07 ` Christoph Hellwig
2019-10-10 15:10   ` Sebastian Andrzej Siewior
2019-10-10 15:14     ` Christoph Hellwig
2019-10-10 15:29       ` Sebastian Andrzej Siewior
2019-10-10 15:29         ` Christoph Hellwig
2019-10-10 15:34           ` [PATCH v2] nvme-tcp: Initialize sk->sk_ll_usec only with NET_RX_BUSY_POLL Sebastian Andrzej Siewior
2019-10-10 17:08             ` Wunderlich, Mark
2019-10-10 17:19               ` Christoph Hellwig
2019-10-14  7:16             ` Christoph Hellwig
2019-10-14 15:06               ` Keith Busch

Linux-NVME Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-nvme/0 linux-nvme/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-nvme linux-nvme/ https://lore.kernel.org/linux-nvme \
		linux-nvme@lists.infradead.org
	public-inbox-index linux-nvme

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-nvme


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git