Message-ID: <20250625163009.7b3a9ae1@kernel.org>
Date: Wed, 25 Jun 2025 16:30:09 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>
Cc: allison.henderson@...cle.com, netdev@...r.kernel.org,
 linux-rdma@...r.kernel.org, rds-devel@....oracle.com, tj@...nel.org,
 andrew@...n.ch, hannes@...xchg.org, mkoutny@...e.com,
 cgroups@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v4.1] rds: Expose feature parameters via sysfs
 (and ELF)

On Mon, 23 Jun 2025 11:51:36 -0400 Konrad Rzeszutek Wilk wrote:
> With the value of 'supported' in them. In the future this value
> could change to say 'deprecated' or have other values (for example
> different versions) or can be runtime changed.

I'm curious how this theoretical 'deprecated' value would work
in context of uAPI which can never regress..

> The choice to use sysfs and this particular way is modeled on the
> filesystems usage exposing their features.
> 
> Alternative solution such as exposing one file ('features') with
> each feature enumerated (which cgroup does) is a bit limited in
> that it does not provide means to provide extra content in the future
> for each feature. For example if one of the features had three
> modes and one wanted to set a particular one at runtime - that
> does not exist in cgroup (albeit it can be implemented but it would
> be quite hectic to have just one single attribute).
> 
> Another solution of using an ioctl to expose a bitmask has the
> disadvantage of being less flexible in the future and while it can
> have a bit of supported/unsupported, it is not clear how one would
> change modes or expose versions. It is most certainly feasible
> but it can get seriously complex fast.
> 
> As such this mechanism offers the basic support we require
> now and offers the flexibility for the future.
> 
> Lastly, we also utilize the ELF note macro to expose these via
> so that applications that have not yet initialized RDS transport
> can inspect the kernel module to see if they have the appropiate
> support and choose an alternative protocol if they wish so.

Looks like this paragraph had a line starting with #, presumably
talking about the ELF note and it got eaten by git? Please fix.


FWIW to me this series has a strong whiff of "we have an OOT module
which has much more functionality and want to support a degraded /
upstream-only mode in the user space stack". I'm probably over-
-interpreting, and you could argue this will help you make real
use of the upstream RDS. I OTOH would argue that it's a technical
solution to a non-technical problem of not giving upstreaming 
sufficient priority; I'd prefer to see code flowing upstream _first_ 
and then worry about compatibility.

$ git log --oneline --since='6 months ago' -- net/rds/ 
433dce0692a0 rds: Correct spelling
6e307a873d30 rds: Correct endian annotation of port and addr assignments
5bccdc51f90c replace strncpy with strscpy_pad
c50d295c37f2 rds: Use nested-BH locking for rds_page_remainder
0af5928f358c rds: Acquire per-CPU pointer within BH disabled section
aaaaa6639cf5 rds: Disable only bottom halves in rds_page_remainder_alloc()
357660d7596b Merge git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net
5c70eb5c593d net: better track kernel sockets lifetime
c451715d78e3 net/rds: Replace deprecated strncpy() with strscpy_pad()
7f5611cbc487 rds: sysctl: rds_tcp_{rcv,snd}buf: avoid using current->nsproxy
$

IOW applying this patch is a bit of a leap of faith that RDS
upstreaming will restart. I don't have anything against the patch
per se, but neither do I have much faith in this. So if v5 is taking 
a long time to get applied it will be because its waiting for DaveM or
Paolo to take it.

Powered by blists - more mailing lists