LinuxLists.cc - [PATCH v2 1/3] tcp: export symbol tcp_set_congestion

2022-03-11 15:54:39

Subject: [PATCH v2 1/3] tcp: export symbol tcp_set_congestion_control

From: Mingbao Sun <[email protected]>

congestion-control could have a noticeable impaction on the
performance of TCP-based communications. This is of course true
to NVMe/TCP in the kernel.

Different congestion-controls (e.g., cubic, dctcp) are suitable for
different scenarios. Proper adoption of congestion control would
benefit the performance. On the contrary, the performance could be
destroyed.

So to gain excellent performance against different network
environments, NVMe/TCP tends to support specifying the
congestion-control.

This means NVMe/TCP (a kernel user) needs to set the congestion-control
of its TCP sockets.

Since the kernel API 'kernel_setsockopt' was removed, and since the
function ‘tcp_set_congestion_control’ is just the real underlying guy
handling this job, so it makes sense to get it exported.

Signed-off-by: Mingbao Sun <[email protected]>
---
net/ipv4/tcp_cong.c | 19 +++++++++++++++----
1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/tcp_cong.c b/net/ipv4/tcp_cong.c
index db5831e6c136..1d6a23e42f7d 100644
--- a/net/ipv4/tcp_cong.c
+++ b/net/ipv4/tcp_cong.c
@@ -344,10 +344,20 @@ int tcp_set_allowed_congestion_control(char *val)
return ret;
}

-/* Change congestion control for socket. If load is false, then it is the
- * responsibility of the caller to call tcp_init_congestion_control or
- * tcp_reinit_congestion_control (if the current congestion control was
- * already initialized.
+/**
+ * tcp_set_congestion_control - set a sock's congestion control
+ * @sk: the sock.
+ * @name: the desired congestion control.
+ * @load: whether to load the required module in case not loaded.
+ * @cap_net_admin: indicating if the caller have the CAP_NET_ADMIN.
+ *
+ * Returns 0 or an error.
+ *
+ * Must be called on a locked sock.
+ *
+ * If load is false, then it is the responsibility of the caller to call
+ * tcp_init_congestion_control or tcp_reinit_congestion_control (if the
+ * current congestion control was already initialized).
*/
int tcp_set_congestion_control(struct sock *sk, const char *name, bool load,
bool cap_net_admin)
@@ -383,6 +393,7 @@ int tcp_set_congestion_control(struct sock *sk, const char *name, bool load,
rcu_read_unlock();
return err;
}
+EXPORT_SYMBOL_GPL(tcp_set_congestion_control);

/* Slow start is used when congestion window is no greater than the slow start
* threshold. We base on RFC2581 and also handle stretch ACKs properly.
--
2.26.2

2022-03-11 20:55:43

by Mingbao Sun

[permalink] [raw]

Subject: [PATCH v2 3/3] nvmet-tcp: support specifying the congestion-control

From: Mingbao Sun <[email protected]>

congestion-control could have a noticeable impaction on the
performance of TCP-based communications. This is of course true
to NVMe_over_TCP.

Different congestion-controls (e.g., cubic, dctcp) are suitable for
different scenarios. Proper adoption of congestion control would benefit
the performance. On the contrary, the performance could be destroyed.

Though we can specify the congestion-control of NVMe_over_TCP via
writing '/proc/sys/net/ipv4/tcp_congestion_control', but this also
changes the congestion-control of all the future TCP sockets that
have not been explicitly assigned the congestion-control, thus bringing
potential impaction on their performance.

So it makes sense to make NVMe_over_TCP support specifying the
congestion-control. And this commit addresses the target side.

Implementation approach:
the following new file entry was created for user to specify the
congestion-control of each nvmet port.
'/sys/kernel/config/nvmet/ports/X/tcp_congestion'
Then later in nvmet_tcp_add_port, the specified congestion-control
would be applied to the listening socket of the nvmet port.

Signed-off-by: Mingbao Sun <[email protected]>
---
drivers/nvme/target/configfs.c | 42 ++++++++++++++++++++++++++++++++++
drivers/nvme/target/nvmet.h | 1 +
drivers/nvme/target/tcp.c | 13 +++++++++++
3 files changed, 56 insertions(+)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index 091a0ca16361..7b7d95f6f582 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -11,6 +11,7 @@
#include <linux/ctype.h>
#include <linux/pci.h>
#include <linux/pci-p2pdma.h>
+#include <net/tcp.h>

#include "nvmet.h"

@@ -222,6 +223,45 @@ static ssize_t nvmet_addr_trsvcid_store(struct config_item *item,

CONFIGFS_ATTR(nvmet_, addr_trsvcid);

+static ssize_t nvmet_tcp_congestion_show(struct config_item *item,
+ char *page)
+{
+ struct nvmet_port *port = to_nvmet_port(item);
+
+ return snprintf(page, PAGE_SIZE, "%s\n",
+ port->tcp_congestion ? port->tcp_congestion : "");
+}
+
+static ssize_t nvmet_tcp_congestion_store(struct config_item *item,
+ const char *page, size_t count)
+{
+ struct nvmet_port *port = to_nvmet_port(item);
+ int len;
+ char *buf;
+
+ len = strcspn(page, "\n");
+ if (!len)
+ return -EINVAL;
+
+ if (nvmet_is_port_enabled(port, __func__))
+ return -EACCES;
+
+ buf = kmemdup_nul(page, len, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+ if (strlen(buf) >= TCP_CA_NAME_MAX) {
+ kfree(buf);
+ return -EINVAL;
+ }
+
+ kfree(port->tcp_congestion);
+ port->tcp_congestion = buf;
+
+ return count;
+}
+
+CONFIGFS_ATTR(nvmet_, tcp_congestion);
+
static ssize_t nvmet_param_inline_data_size_show(struct config_item *item,
char *page)
{
@@ -1597,6 +1637,7 @@ static void nvmet_port_release(struct config_item *item)
list_del(&port->global_entry);

kfree(port->ana_state);
+ kfree(port->tcp_congestion);
kfree(port);
}

@@ -1605,6 +1646,7 @@ static struct configfs_attribute *nvmet_port_attrs[] = {
&nvmet_attr_addr_treq,
&nvmet_attr_addr_traddr,
&nvmet_attr_addr_trsvcid,
+ &nvmet_attr_tcp_congestion,
&nvmet_attr_addr_trtype,
&nvmet_attr_param_inline_data_size,
#ifdef CONFIG_BLK_DEV_INTEGRITY
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 69637bf8f8e1..76a57c4c3456 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -145,6 +145,7 @@ struct nvmet_port {
struct config_group ana_groups_group;
struct nvmet_ana_group ana_default_group;
enum nvme_ana_state *ana_state;
+ const char *tcp_congestion;
void *priv;
bool enabled;
int inline_data_size;
diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
index 83ca577f72be..311383c6d7da 100644
--- a/drivers/nvme/target/tcp.c
+++ b/drivers/nvme/target/tcp.c
@@ -1741,6 +1741,19 @@ static int nvmet_tcp_add_port(struct nvmet_port *nport)
if (so_priority > 0)
sock_set_priority(port->sock->sk, so_priority);

+ if (nport->tcp_congestion) {
+ lock_sock(port->sock->sk);
+ ret = tcp_set_congestion_control(port->sock->sk,
+ nport->tcp_congestion,
+ true, true);
+ release_sock(port->sock->sk);
+ if (ret) {
+ pr_err("failed to set port socket's congestion to %s: %d\n",
+ nport->tcp_congestion, ret);
+ goto err_sock;
+ }
+ }
+
ret = kernel_bind(port->sock, (struct sockaddr *)&port->addr,
sizeof(port->addr));
if (ret) {
--
2.26.2

2022-03-11 22:41:18

by Mingbao Sun

[permalink] [raw]

Subject: [PATCH v2 2/3] nvme-tcp: support specifying the congestion-control

From: Mingbao Sun <[email protected]>

congestion-control could have a noticeable impaction on the
performance of TCP-based communications. This is of course true
to NVMe_over_TCP.

Different congestion-controls (e.g., cubic, dctcp) are suitable for
different scenarios. Proper adoption of congestion control would benefit
the performance. On the contrary, the performance could be destroyed.

Though we can specify the congestion-control of NVMe_over_TCP via
writing '/proc/sys/net/ipv4/tcp_congestion_control', but this also
changes the congestion-control of all the future TCP sockets that
have not been explicitly assigned the congestion-control, thus bringing
potential impaction on their performance.

So it makes sense to make NVMe_over_TCP support specifying the
congestion-control. And this commit addresses the host side.

Implementation approach:
a new option called 'tcp_congestion' was created in fabrics opt_tokens
for 'nvme connect' command to passed in the congestion-control
specified by the user.
Then later in nvme_tcp_alloc_queue, the specified congestion-control
would be applied to the relevant sockets of the host side.

Signed-off-by: Mingbao Sun <[email protected]>
---
drivers/nvme/host/fabrics.c | 18 ++++++++++++++++++
drivers/nvme/host/fabrics.h | 2 ++
drivers/nvme/host/tcp.c | 17 ++++++++++++++++-
3 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index ee79a6d639b4..ecd1ec4e473a 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -10,6 +10,7 @@
#include <linux/mutex.h>
#include <linux/parser.h>
#include <linux/seq_file.h>
+#include <net/tcp.h>
#include "nvme.h"
#include "fabrics.h"

@@ -548,6 +549,7 @@ static const match_table_t opt_tokens = {
{ NVMF_OPT_TOS, "tos=%d" },
{ NVMF_OPT_FAIL_FAST_TMO, "fast_io_fail_tmo=%d" },
{ NVMF_OPT_DISCOVERY, "discovery" },
+ { NVMF_OPT_TCP_CONGESTION, "tcp_congestion=%s" },
{ NVMF_OPT_ERR, NULL }
};

@@ -829,6 +831,21 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
case NVMF_OPT_DISCOVERY:
opts->discovery_nqn = true;
break;
+ case NVMF_OPT_TCP_CONGESTION:
+ p = match_strdup(args);
+ if (!p) {
+ ret = -ENOMEM;
+ goto out;
+ }
+ if (strlen(p) >= TCP_CA_NAME_MAX) {
+ ret = -EINVAL;
+ kfree(p);
+ goto out;
+ }
+
+ kfree(opts->tcp_congestion);
+ opts->tcp_congestion = p;
+ break;
default:
pr_warn("unknown parameter or missing value '%s' in ctrl creation request\n",
p);
@@ -947,6 +964,7 @@ void nvmf_free_options(struct nvmf_ctrl_options *opts)
kfree(opts->subsysnqn);
kfree(opts->host_traddr);
kfree(opts->host_iface);
+ kfree(opts->tcp_congestion);
kfree(opts);
}
EXPORT_SYMBOL_GPL(nvmf_free_options);
diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
index c3203ff1c654..25fdc169949d 100644
--- a/drivers/nvme/host/fabrics.h
+++ b/drivers/nvme/host/fabrics.h
@@ -68,6 +68,7 @@ enum {
NVMF_OPT_FAIL_FAST_TMO = 1 << 20,
NVMF_OPT_HOST_IFACE = 1 << 21,
NVMF_OPT_DISCOVERY = 1 << 22,
+ NVMF_OPT_TCP_CONGESTION = 1 << 23,
};

/**
@@ -117,6 +118,7 @@ struct nvmf_ctrl_options {
unsigned int nr_io_queues;
unsigned int reconnect_delay;
bool discovery_nqn;
+ const char *tcp_congestion;
bool duplicate_connect;
unsigned int kato;
struct nvmf_host *host;
diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 10fc45d95b86..8491f96a39e5 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -1487,6 +1487,20 @@ static int nvme_tcp_alloc_queue(struct nvme_ctrl *nctrl,
if (nctrl->opts->tos >= 0)
ip_sock_set_tos(queue->sock->sk, nctrl->opts->tos);

+ if (nctrl->opts->mask & NVMF_OPT_TCP_CONGESTION) {
+ lock_sock(queue->sock->sk);
+ ret = tcp_set_congestion_control(queue->sock->sk,
+ nctrl->opts->tcp_congestion,
+ true, true);
+ release_sock(queue->sock->sk);
+ if (ret) {
+ dev_err(nctrl->device,
+ "failed to set TCP congestion to %s: %d\n",
+ nctrl->opts->tcp_congestion, ret);
+ goto err_sock;
+ }
+ }
+
/* Set 10 seconds timeout for icresp recvmsg */
queue->sock->sk->sk_rcvtimeo = 10 * HZ;

@@ -2650,7 +2664,8 @@ static struct nvmf_transport_ops nvme_tcp_transport = {
NVMF_OPT_HOST_TRADDR | NVMF_OPT_CTRL_LOSS_TMO |
NVMF_OPT_HDR_DIGEST | NVMF_OPT_DATA_DIGEST |
NVMF_OPT_NR_WRITE_QUEUES | NVMF_OPT_NR_POLL_QUEUES |
- NVMF_OPT_TOS | NVMF_OPT_HOST_IFACE,
+ NVMF_OPT_TOS | NVMF_OPT_HOST_IFACE |
+ NVMF_OPT_TCP_CONGESTION,
.create_ctrl = nvme_tcp_create_ctrl,
};

--
2.26.2

2022-03-14 06:52:04

by Sagi Grimberg

[permalink] [raw]

Subject: Re: [PATCH v2 2/3] nvme-tcp: support specifying the congestion-control

> From: Mingbao Sun <[email protected]>

Hey Mingbao,

> congestion-control could have a noticeable impaction on the
> performance of TCP-based communications. This is of course true
> to NVMe_over_TCP.
>
> Different congestion-controls (e.g., cubic, dctcp) are suitable for
> different scenarios. Proper adoption of congestion control would benefit
> the performance. On the contrary, the performance could be destroyed.
>
> Though we can specify the congestion-control of NVMe_over_TCP via
> writing '/proc/sys/net/ipv4/tcp_congestion_control', but this also
> changes the congestion-control of all the future TCP sockets that
> have not been explicitly assigned the congestion-control, thus bringing
> potential impaction on their performance.
>
> So it makes sense to make NVMe_over_TCP support specifying the
> congestion-control. And this commit addresses the host side.

Thanks for this patchset.

Generally, I'm not opposed to allow users to customize what they want
to do, but in order to add something like this we need a few
justifications.

1. Can you please provide your measurements that support your claims?

2. Can you please provide a real, existing use-case where this provides
true, measureable value? And more specifically, please clarify how the
use-case needs a local tuning for nvme-tcp that would not hold for
other tcp streams that are running on the host (and vice-versa).

3. There are quite a few of TCP tuning knobs that will affect how
nvme-tcp performs, just like any TCP application that running on Linux.
However, Application level TCP tuning is not widespread at all, what
makes nvme-tcp special to allow this, and why the TCP congestion is more
important than other tuning knobs? I am not supportive of exporting
all or some TCP level knobs as a local shadow for sysctl.

Adding tunables, especially ones that are address niche use-cases in
nature, can easily become a slippery slope for a set of rarely touched
code and interface we are left stuck with for a long time...

But while this feels a bit random to me, I'm not objecting to add this
to the driver. I just want to make sure that this is something that is
a) really required and b) does not backfire on us nor the user.

> Implementation approach:
> a new option called 'tcp_congestion' was created in fabrics opt_tokens
> for 'nvme connect' command to passed in the congestion-control
> specified by the user.
> Then later in nvme_tcp_alloc_queue, the specified congestion-control
> would be applied to the relevant sockets of the host side.

Specifically to the interface, I'm wandering if this is the right
interface... The user is used to sysctl with the semantics that it
provides, wouldn't it be better to expose the exact same interface
just for nvme-tcp sockets?

Something like sysctl nvme.tcp_congestion_control ?

2022-03-14 10:10:43

by Sagi Grimberg

[permalink] [raw]

On Tue, 29 Mar 2022 10:46:08 +0300
Sagi Grimberg <[email protected]> wrote:

> >> As I said, TCP can be tuned in various ways, congestion being just one
> >> of them. I'm sure you can find a workload where rmem/wmem will make
> >> a difference.
> >
> > agree.
> > but the difference for the knob of rmem/wmem is:
> > we could enlarge rmem/wmem for NVMe/TCP via sysctl,
> > and it would not bring downside to any other sockets whose
> > rmem/wmem are not explicitly specified.
>
> It can most certainly affect them, positively or negatively, depends
> on the use-case.

Agree.
Your saying is rigorous.

> >> In addition, based on my knowledge, application specific TCP level
> >> tuning (like congestion) is not really a common thing to do. So why in
> >> nvme-tcp?
> >>
> >> So to me at least, it is not clear why we should add it to the driver.
> >
> > As mentioned in the commit message, though we can specify the
> > congestion-control of NVMe_over_TCP via sysctl or writing
> > '/proc/sys/net/ipv4/tcp_congestion_control', but this also
> > changes the congestion-control of all the future TCP sockets on
> > the same host that have not been explicitly assigned the
> > congestion-control, thus bringing potential impaction on their
> > performance.
> >
> > For example:
> >
> > A server in a data-center with the following 2 NICs:
> >
> > - NIC_fron-end, for interacting with clients through WAN
> > (high latency, ms-level)
> >
> > - NIC_back-end, for interacting with NVMe/TCP target through LAN
> > (low latency, ECN-enabled, ideal for dctcp)
> >
> > This server interacts with clients (handling requests) via the fron-end
> > network and accesses the NVMe/TCP storage via the back-end network.
> > This is a normal use case, right?
> >
> > For the client devices, we can’t determine their congestion-control.
> > But normally it’s cubic by default (per the CONFIG_DEFAULT_TCP_CONG).
> > So if we change the default congestion control on the server to dctcp
> > on behalf of the NVMe/TCP traffic of the LAN side, it could at the
> > same time change the congestion-control of the front-end sockets
> > to dctcp while the congestion-control of the client-side is cubic.
> > So this is an unexpected scenario.
> >
> > In addition, distributed storage products like the following also have
> > the above problem:
> >
> > - The product consists of a cluster of servers.
> >
> > - Each server serves clients via its front-end NIC
> > (WAN, high latency).
> >
> > - All servers interact with each other via NVMe/TCP via back-end NIC
> > (LAN, low latency, ECN-enabled, ideal for dctcp).
>
> Separate networks are still not application (nvme-tcp) specific and as
> mentioned, we have a way to control that. IMO, this still does not
> qualify as solid justification to add this to nvme-tcp.
>
> What do others think?

Well, per the fact that the approach (‘ip route …’) proposed
by Jakub could largely fit the per link requirement on
congestion-control, so the usefulness of this patchset is really
not so significant.

So here I terminate all the threads of this patchset.

At last, many thanks to all of you for reviewing this patchset.

2022-03-31 05:14:04

by Mingbao Sun

[permalink] [raw]

Subject: Re: [PATCH v2 2/3] nvme-tcp: support specifying the congestion-control

2022-03-31 05:15:21

by Mingbao Sun

[permalink] [raw]

Subject: Re: [PATCH v2 2/3] nvme-tcp: support specifying the congestion-control

2022-03-31 05:51:26

by Mingbao Sun

[permalink] [raw]

Subject: Re: [PATCH v2 2/3] nvme-tcp: support specifying the congestion-control

OOOPS, my mail service has got some issues.
Sorry for the duplicated emails.