diff mbox series

tcp: export symbol tcp_set_congestion_control

Message ID 20220310134830.130818-1-sunmingbao@tom.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series tcp: export symbol tcp_set_congestion_control | expand

Checks

Context Check Description
netdev/tree_selection success Guessed tree name to be net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix warning Target tree name not specified in the subject
netdev/cover_letter success Single patches do not need cover letters
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 4 this patch: 4
netdev/cc_maintainers success CCed 6 of 6 maintainers
netdev/build_clang success Errors and warnings before: 18 this patch: 18
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 9 this patch: 9
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 7 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Mingbao Sun March 10, 2022, 1:48 p.m. UTC
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(+)

Comments

Christoph Hellwig March 10, 2022, 2:11 p.m. UTC | #1
Please submit this together with the actual user(s) in a series.
Patches to just export random symbols are a no-go.
Mingbao Sun March 10, 2022, 3:03 p.m. UTC | #2
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?
Christoph Hellwig March 10, 2022, 3:05 p.m. UTC | #3
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.
Jakub Kicinski March 10, 2022, 8:48 p.m. UTC | #4
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.
Mingbao Sun March 11, 2022, 1:29 a.m. UTC | #5
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
Christoph Hellwig March 11, 2022, 7:19 a.m. UTC | #6
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.
Jakub Kicinski March 11, 2022, 4:18 p.m. UTC | #7
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.
diff mbox series

Patch

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.