[PATCH] tcp: export symbol tcp_set_congestion_control

[PATCH] tcp: export symbol tcp_set_congestion_control

[Mingbao Sun]

From: Mingbao Sun <tyler.sun@dell.com>

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 <tyler.sun@dell.com>
---
 net/ipv4/tcp_cong.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/ipv4/tcp_cong.c b/net/ipv4/tcp_cong.c
index db5831e6c136..5d77f3e7278e 100644
--- a/net/ipv4/tcp_cong.c
+++ b/net/ipv4/tcp_cong.c
@@ -383,6 +383,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

Re: [PATCH] tcp: export symbol tcp_set_congestion_control

[Christoph Hellwig]

Please submit this together with the actual user(s) in a series.
Patches to just export random symbols are a no-go.

Re: [PATCH] tcp: export symbol tcp_set_congestion_control

[Mingbao Sun]

On Thu, 10 Mar 2022 15:11:35 +0100
Christoph Hellwig <hch@lst.de> wrote:

> Please submit this together with the actual user(s) in a series.
> Patches to just export random symbols are a no-go.

Got it.
many thanks for informing.

BTW:
could you give me the answer to the following questions?
1. Against which repo should I prepare and test this series of patches?
   netdev or nvme?

2. what's the recipients for this series of patches?
   netdev or nvme or both?

Re: [PATCH] tcp: export symbol tcp_set_congestion_control

[Christoph Hellwig]

On Thu, Mar 10, 2022 at 11:03:00PM +0800, Mingbao Sun wrote:
> On Thu, 10 Mar 2022 15:11:35 +0100
> Christoph Hellwig <hch@lst.de> wrote:
> 
> > Please submit this together with the actual user(s) in a series.
> > Patches to just export random symbols are a no-go.
> 
> Got it.
> many thanks for informing.
> 
> BTW:
> could you give me the answer to the following questions?
> 1. Against which repo should I prepare and test this series of patches?
>    netdev or nvme?

I think for the initial RFC either is fine, just clearly state which.

> 2. what's the recipients for this series of patches?
>    netdev or nvme or both?

both.

Re: [PATCH] tcp: export symbol tcp_set_congestion_control

[Jakub Kicinski]

On Thu, 10 Mar 2022 21:48:30 +0800 Mingbao Sun wrote:
> 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.

Do you happen to have a reference to the commit which removed
kernel_setsockopt and the justification?  My knee jerk reaction
would the that's a better path than allowing in-kernel socket users 
to poke at internal functions even if that works as of today.

Re: [PATCH] tcp: export symbol tcp_set_congestion_control

[Mingbao Sun]

On Thu, 10 Mar 2022 12:48:25 -0800
Jakub Kicinski <kuba@kernel.org> wrote:

> On Thu, 10 Mar 2022 21:48:30 +0800 Mingbao Sun wrote:
> > 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.  
> 
> Do you happen to have a reference to the commit which removed
> kernel_setsockopt and the justification?  My knee jerk reaction
> would the that's a better path than allowing in-kernel socket users 
> to poke at internal functions even if that works as of today.

FYI
(Sorry for putting URLs in the mail):

kernel_setsockopt disappeared from kernel v5.8,
and all the relevant users have switched to
dedicated small functions.

The mail thread:
https://lists.archive.carbon60.com/linux/kernel/3712394

The commit:
https://github.com/torvalds/linux/commit/5a892ff2facb4548c17c05931ed899038a0da63e

Re: [PATCH] tcp: export symbol tcp_set_congestion_control

[Christoph Hellwig]

On Thu, Mar 10, 2022 at 12:48:25PM -0800, Jakub Kicinski wrote:
> On Thu, 10 Mar 2022 21:48:30 +0800 Mingbao Sun wrote:
> > 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.
> 
> Do you happen to have a reference to the commit which removed
> kernel_setsockopt and the justification?  My knee jerk reaction
> would the that's a better path than allowing in-kernel socket users 
> to poke at internal functions even if that works as of today.

This was part of the set_fs() removal. Back then we decided we'd rather
have type-safe APIs for in-kernel users, which in total was a major
removal of code lines.

Re: [PATCH] tcp: export symbol tcp_set_congestion_control

[Jakub Kicinski]

On Fri, 11 Mar 2022 08:19:30 +0100 Christoph Hellwig wrote:
> On Thu, Mar 10, 2022 at 12:48:25PM -0800, Jakub Kicinski wrote:
> > On Thu, 10 Mar 2022 21:48:30 +0800 Mingbao Sun wrote:  
> > > 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.  
> > 
> > Do you happen to have a reference to the commit which removed
> > kernel_setsockopt and the justification?  My knee jerk reaction
> > would the that's a better path than allowing in-kernel socket users 
> > to poke at internal functions even if that works as of today.  
> 
> This was part of the set_fs() removal. Back then we decided we'd rather
> have type-safe APIs for in-kernel users, which in total was a major
> removal of code lines.

I see, thanks. I guess no point speculating, we can revisit if
any bugs due to direct callers actually materialize.