> On Mon, May 13, 2024 at 02:53:42PM +0200, Håkon Bugge wrote:
> For most entry points to RDS, we call memalloc_noio_{save,restore} in
> a parenthetic fashion when enabled by the module parameter force_noio.
>
> We skip the calls to memalloc_noio_{save,restore} in rds_ioctl(), as
> no memory allocations are executed in this function or its callees.
>
> The reason we execute memalloc_noio_{save,restore} in rds_poll(), is
> due to the following call chain:
>
> rds_poll()
> poll_wait()
> __pollwait()
> poll_get_entry()
> __get_free_page(GFP_KERNEL)
>
> The function rds_setsockopt() allocates memory in its callee's
> rds_get_mr() and rds_get_mr_for_dest(). Hence, we need
> memalloc_noio_{save,restore} in rds_setsockopt().
>
> In rds_getsockopt(), we have rds_info_getsockopt() that allocates
> memory. Hence, we need memalloc_noio_{save,restore} in
> rds_getsockopt().
>
> All the above, in order to conditionally enable RDS to become a block I/O
> device.
>
> Signed-off-by: Håkon Bugge <haakon.bugge@xxxxxxxxxx>
>
> Hi Håkon,
>
> Some minor feedback from my side.
>
> ---
> net/rds/af_rds.c | 60 +++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 57 insertions(+), 3 deletions(-)
>
> diff --git a/net/rds/af_rds.c b/net/rds/af_rds.c
> index 8435a20968ef5..a89d192aabc0b 100644
> --- a/net/rds/af_rds.c
> +++ b/net/rds/af_rds.c
> @@ -37,10 +37,16 @@
> #include <linux/in.h>
> #include <linux/ipv6.h>
> #include <linux/poll.h>
> +#include <linux/sched/mm.h>
> #include <net/sock.h>
>
> #include "rds.h"
>
> +bool rds_force_noio;
> +EXPORT_SYMBOL(rds_force_noio);
>
> rds_force_noio seems to be only used within this file.
> I wonder if it should it be static and not EXPORTed?
>
> Flagged by Sparse.
Hi Simon,
You are quite right. Had an earlier version where the symbol was used in several files, but in this version, static is the right choice. Fixed in v2.
> +module_param_named(force_noio, rds_force_noio, bool, 0444);
> +MODULE_PARM_DESC(force_noio, "Force the use of GFP_NOIO (Y/N)");
> +
> /* this is just used for stats gathering :/ */
> static DEFINE_SPINLOCK(rds_sock_lock);
> static unsigned long rds_sock_count;
> @@ -60,6 +66,10 @@ static int rds_release(struct socket *sock)
> {
> struct sock *sk = sock->sk;
> struct rds_sock *rs;
> + unsigned int noio_flags;
>
> Please consider using reverse xmas tree order - longest line to shortest -
> for local variable declarations in Networking code.
>
> This tool can be of assistance: https://github.com/ecree-solarflare/xmastree
Will fix.
>
> +
> + if (rds_force_noio)
> + noio_flags = memalloc_noio_save();
>
> if (!sk)
> goto out;
>
> ...
>
> @@ -324,6 +346,8 @@ static int rds_cancel_sent_to(struct rds_sock *rs, sockptr_t optval, int len)
>
> rds_send_drop_to(rs, &sin6);
> out:
> + if (rds_force_noio)
> + noio_flags = memalloc_noio_save();
>
> noio_flags appears to be set but otherwise unused in this function.
Bummer. C/P error. This should be the restore. Fixed in v2. Will add W=1 for builds in the future :-)
Thxs, Håkon
