Re: [RFC PATCH v1 21/57] sunrpc: Remove PAGE_SIZE compile-time constant assumption

[Jeff Layton <jlayton@kernel.org>]

On Wed, 2024-10-16 at 10:47 -0400, Chuck Lever wrote:
> On Wed, Oct 16, 2024 at 03:42:12PM +0100, Ryan Roberts wrote:
> > + Chuck Lever, Jeff Layton
> > 
> > This was a rather tricky series to get the recipients correct for and my script
> > did not realize that "supporter" was a pseudonym for "maintainer" so you were
> > missed off the original post. Appologies!
> > 
> > More context in cover letter:
> > https://lore.kernel.org/all/20241014105514.3206191-1-ryan.roberts@arm.com/
> > 
> > 
> > On 14/10/2024 11:58, Ryan Roberts wrote:
> > > To prepare for supporting boot-time page size selection, refactor code
> > > to remove assumptions about PAGE_SIZE being compile-time constant. Code
> > > intended to be equivalent when compile-time page size is active.
> > > 
> > > Updated array sizes in various structs to contain enough entries for the
> > > smallest supported page size.
> > > 
> > > Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
> > > ---
> > > 
> > > ***NOTE***
> > > Any confused maintainers may want to read the cover note here for context:
> > > https://lore.kernel.org/all/20241014105514.3206191-1-ryan.roberts@arm.com/
> > > 
> > >  include/linux/sunrpc/svc.h      | 8 +++++---
> > >  include/linux/sunrpc/svc_rdma.h | 4 ++--
> > >  include/linux/sunrpc/svcsock.h  | 2 +-
> > >  3 files changed, 8 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
> > > index a7d0406b9ef59..dda44018b8f36 100644
> > > --- a/include/linux/sunrpc/svc.h
> > > +++ b/include/linux/sunrpc/svc.h
> > > @@ -160,6 +160,8 @@ extern u32 svc_max_payload(const struct svc_rqst *rqstp);
> > >   */
> > >  #define RPCSVC_MAXPAGES		((RPCSVC_MAXPAYLOAD+PAGE_SIZE-1)/PAGE_SIZE \
> > >  				+ 2 + 1)
> > > +#define RPCSVC_MAXPAGES_MAX	((RPCSVC_MAXPAYLOAD+PAGE_SIZE_MIN-1)/PAGE_SIZE_MIN \
> > > +				+ 2 + 1)
> 
> There is already a "MAX" in the name, so adding this new macro seems
> superfluous to me. Can we get away with simply updating the
> "RPCSVC_MAXPAGES" macro, instead of adding this new one?
> 

+1 that was my thinking too. This is mostly just used to size arrays,
so we might as well just change the existing macro.

With 64k pages we probably wouldn't need arrays as long as these will
be. Fixing those array sizes to be settable at runtime though is not a
trivial project though.

> 
> > >  /*
> > >   * The context of a single thread, including the request currently being
> > > @@ -190,14 +192,14 @@ struct svc_rqst {
> > >  	struct xdr_stream	rq_res_stream;
> > >  	struct page		*rq_scratch_page;
> > >  	struct xdr_buf		rq_res;
> > > -	struct page		*rq_pages[RPCSVC_MAXPAGES + 1];
> > > +	struct page		*rq_pages[RPCSVC_MAXPAGES_MAX + 1];
> > >  	struct page *		*rq_respages;	/* points into rq_pages */
> > >  	struct page *		*rq_next_page; /* next reply page to use */
> > >  	struct page *		*rq_page_end;  /* one past the last page */
> > >  
> > >  	struct folio_batch	rq_fbatch;
> > > -	struct kvec		rq_vec[RPCSVC_MAXPAGES]; /* generally useful.. */
> > > -	struct bio_vec		rq_bvec[RPCSVC_MAXPAGES];
> > > +	struct kvec		rq_vec[RPCSVC_MAXPAGES_MAX]; /* generally useful.. */
> > > +	struct bio_vec		rq_bvec[RPCSVC_MAXPAGES_MAX];
> > >  
> > >  	__be32			rq_xid;		/* transmission id */
> > >  	u32			rq_prog;	/* program number */
> > > diff --git a/include/linux/sunrpc/svc_rdma.h b/include/linux/sunrpc/svc_rdma.h
> > > index d33bab33099ab..7c6441e8d6f7a 100644
> > > --- a/include/linux/sunrpc/svc_rdma.h
> > > +++ b/include/linux/sunrpc/svc_rdma.h
> > > @@ -200,7 +200,7 @@ struct svc_rdma_recv_ctxt {
> > >  	struct svc_rdma_pcl	rc_reply_pcl;
> > >  
> > >  	unsigned int		rc_page_count;
> > > -	struct page		*rc_pages[RPCSVC_MAXPAGES];
> > > +	struct page		*rc_pages[RPCSVC_MAXPAGES_MAX];
> > >  };
> > >  
> > >  /*
> > > @@ -242,7 +242,7 @@ struct svc_rdma_send_ctxt {
> > >  	void			*sc_xprt_buf;
> > >  	int			sc_page_count;
> > >  	int			sc_cur_sge_no;
> > > -	struct page		*sc_pages[RPCSVC_MAXPAGES];
> > > +	struct page		*sc_pages[RPCSVC_MAXPAGES_MAX];
> > >  	struct ib_sge		sc_sges[];
> > >  };
> > >  
> > > diff --git a/include/linux/sunrpc/svcsock.h b/include/linux/sunrpc/svcsock.h
> > > index 7c78ec6356b92..6c6bcc82685a3 100644
> > > --- a/include/linux/sunrpc/svcsock.h
> > > +++ b/include/linux/sunrpc/svcsock.h
> > > @@ -40,7 +40,7 @@ struct svc_sock {
> > >  
> > >  	struct completion	sk_handshake_done;
> > >  
> > > -	struct page *		sk_pages[RPCSVC_MAXPAGES];	/* received data */
> > > +	struct page *		sk_pages[RPCSVC_MAXPAGES_MAX];	/* received data */
> > >  };
> > >  
> > >  static inline u32 svc_sock_reclen(struct svc_sock *svsk)
> > 
> 

-- 
Jeff Layton <jlayton@kernel.org>