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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 10 Mar 2022 19:06:36 +0800
From:   Mingbao Sun <sunmingbao@....com>
To:     Christoph Hellwig <hch@....de>
Cc:     Keith Busch <kbusch@...nel.org>, Jens Axboe <axboe@...com>,
        Sagi Grimberg <sagi@...mberg.me>,
        Chaitanya Kulkarni <kch@...dia.com>,
        linux-nvme@...ts.infradead.org, linux-kernel@...r.kernel.org,
        tyler.sun@...l.com, ping.gan@...l.com, yanxiu.cai@...l.com,
        libin.zhang@...l.com, ao.sun@...l.com
Subject: Re: [PATCH v2 1/2] nvmet-tcp: support specifying the
 congestion-control

On Thu, 10 Mar 2022 09:38:11 +0100
Christoph Hellwig <hch@....de> wrote:

> On Wed, Mar 09, 2022 at 05:52:03PM +0800, Mingbao Sun wrote:
> > On Wed, 9 Mar 2022 07:15:41 +0100
> > Christoph Hellwig <hch@....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.   
> 
> A warning message really seems very severe for a condition like this.
> Maybe the better interface is a way to figure out which congestion
> control algorithm is in use by reading a sysfs file.

Well, a target could have a great number of TCP sockets.

I feel it’s not proper to create a sysfs entry for each socket.
And for those sockets that do not have the exception of
congestion-control, it’s merely a waste of resources.

Also, since these sockets generate and die dynamically, the info
exported via fs may even do not have the opportunity to be seen by
the user.

Anyway, if you insist that the checking and warning here is not proper,
I can remove it.