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

[Simon Horman <horms@kernel.org>]

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;
>  }
>  

...