* [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 related [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 related [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, other threads:[~2019-10-14 15:06 UTC | newest]
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
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).