Re: [PATCH 2/6] rds: Brute force GFP_NOIO

Simon Horman
2024-05-13T18:14:31Z

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@oracle.com>

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.

> +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

> +
> + 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.

Flagged by W=1 builds.

> return ret;
> }
>

...