Re: [PATCH v2 1/2] nvmet-tcp: support specifying the congestion-control

[Mingbao Sun <sunmingbao@tom.com>]

On Wed, 9 Mar 2022 07:15:41 +0100
Christoph Hellwig <hch@lst.de> wrote:

> On Wed, Mar 09, 2022 at 01:37:11PM +0800, Mingbao Sun wrote:
> > +		if (port->nport->tcp_congestion) {
> > +			icsk_new = inet_csk(newsock->sk);
> > +			if (icsk_new->icsk_ca_ops != icsk->icsk_ca_ops) {
> > +				pr_warn("congestion abnormal: expected %s, actual %s.\n",
> > +					icsk->icsk_ca_ops->name,
> > +					icsk_new->icsk_ca_ops->name);
> > +			}
> > +		}  
> 
> What is the point of having this code?

Well, this could happen in certain circumstances.
Take the result from my test as an example:

    - The congestion of the listening socket of the target was set to
      ‘dctcp’.

    - But the congestion of the socket of the host side was set to
      ‘cubic’.

    - Then the congestion of the socket of the new connection at the
      target side would automatically be altered to ‘dctcp-reno’.

In case tcp_congestion was explicitly set for the target, it can be
supposed that the user attaches great importance to performance.
So we’d better make the users aware that the system is not working
in the way they expect. 

Thus the checking and warning was added here.

> 
> > +	if (nport->tcp_congestion) {
> > +		strncpy(ca_name, nport->tcp_congestion, TCP_CA_NAME_MAX-1);
> > +		optval = KERNEL_SOCKPTR(ca_name);
> > +		ret = sock_common_setsockopt(port->sock, IPPROTO_TCP,
> > +					     TCP_CONGESTION, optval,
> > +					     strlen(ca_name));
> > +		if (ret) {
> > +			pr_err("failed to set port socket's congestion to %s: %d\n",
> > +			       ca_name, ret);
> > +			goto err_sock;
> > +		}
> > +	}  
> 
> Same comment as for the host side.

This will be handled as the host side in the next version.