| Message ID | 165852114280.11403.7277687995924103645.stgit@manet.1015granger.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Put struct nfsd4_copy on a diet | expand |
Hi Chuck, A general question: what's the rule for deciding whether to allocate statically or dynamically? I thought that "small" structures it's better to preallocate (statically) for performance reasons. Or is the idea here that copy_notify/copy are rare operations that instead they should be allocated dynamically and so that other operations doesn't consume more memory in nfsd4_op structure? On Fri, Jul 22, 2022 at 4:35 PM Chuck Lever <chuck.lever@oracle.com> wrote: > > struct nfsd4_copy_notify is part of struct nfsd4_op, which resides > in an 8-element array. > > sizeof(struct nfsd4_op): > Before: /* size: 2208, cachelines: 35, members: 5 */ > After: /* size: 1696, cachelines: 27, members: 5 */ > > Signed-off-by: Chuck Lever <chuck.lever@oracle.com> > --- > fs/nfsd/nfs4proc.c | 4 ++-- > fs/nfsd/nfs4xdr.c | 12 ++++++++++-- > fs/nfsd/xdr4.h | 4 ++-- > 3 files changed, 14 insertions(+), 6 deletions(-) > > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c > index 3895eb52d2b1..22c5ccb83d20 100644 > --- a/fs/nfsd/nfs4proc.c > +++ b/fs/nfsd/nfs4proc.c > @@ -1953,9 +1953,9 @@ nfsd4_copy_notify(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, > /* For now, only return one server address in cpn_src, the > * address used by the client to connect to this server. > */ > - cn->cpn_src.nl4_type = NL4_NETADDR; > + cn->cpn_src->nl4_type = NL4_NETADDR; > status = nfsd4_set_netaddr((struct sockaddr *)&rqstp->rq_daddr, > - &cn->cpn_src.u.nl4_addr); > + &cn->cpn_src->u.nl4_addr); > WARN_ON_ONCE(status); > if (status) { > nfs4_put_cpntf_state(nn, cps); > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c > index 358b3338c4cc..335431199077 100644 > --- a/fs/nfsd/nfs4xdr.c > +++ b/fs/nfsd/nfs4xdr.c > @@ -1952,10 +1952,17 @@ nfsd4_decode_copy_notify(struct nfsd4_compoundargs *argp, > { > __be32 status; > > + cn->cpn_src = svcxdr_tmpalloc(argp, sizeof(*cn->cpn_src)); > + if (cn->cpn_src == NULL) > + return nfserrno(-ENOMEM); /* XXX: jukebox? */ > + cn->cpn_dst = svcxdr_tmpalloc(argp, sizeof(*cn->cpn_dst)); > + if (cn->cpn_dst == NULL) > + return nfserrno(-ENOMEM); /* XXX: jukebox? */ > + > status = nfsd4_decode_stateid4(argp, &cn->cpn_src_stateid); > if (status) > return status; > - return nfsd4_decode_nl4_server(argp, &cn->cpn_dst); > + return nfsd4_decode_nl4_server(argp, cn->cpn_dst); > } > > static __be32 > @@ -4898,7 +4905,8 @@ nfsd4_encode_copy_notify(struct nfsd4_compoundres *resp, __be32 nfserr, > > *p++ = cpu_to_be32(1); > > - return nfsd42_encode_nl4_server(resp, &cn->cpn_src); > + nfserr = nfsd42_encode_nl4_server(resp, cn->cpn_src); > + return nfserr; > } > > static __be32 > diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h > index 6e6a89008ce1..f253fc3f4708 100644 > --- a/fs/nfsd/xdr4.h > +++ b/fs/nfsd/xdr4.h > @@ -595,13 +595,13 @@ struct nfsd4_offload_status { > struct nfsd4_copy_notify { > /* request */ > stateid_t cpn_src_stateid; > - struct nl4_server cpn_dst; > + struct nl4_server *cpn_dst; > > /* response */ > stateid_t cpn_cnr_stateid; > u64 cpn_sec; > u32 cpn_nsec; > - struct nl4_server cpn_src; > + struct nl4_server *cpn_src; > }; > > struct nfsd4_op { > >
> On Jul 25, 2022, at 10:19 AM, Olga Kornievskaia <aglo@umich.edu> wrote: > > Hi Chuck, > > A general question: what's the rule for deciding whether to allocate > statically or dynamically? I thought that "small" structures it's > better to preallocate (statically) for performance reasons. Or is the > idea here that copy_notify/copy are rare operations that instead they > should be allocated dynamically and so that other operations doesn't > consume more memory in nfsd4_op structure? tl;dr: the latter. But it's not an issue of memory consumption, it's that this whole struct is cleared with memset(0) for each incoming RPC. nfsd4_op is a union -- it takes the size of the largest args struct in that union. Before copy offload was introduced, that was about 250 bytes. After, it is ~10x larger. There are 8 nfsd4_op structs in an array in nfsd4_compoundargs. The nfsd4_compoundargs structure is cleared by svc_generic_init_request() before each NFSv4 RPC is executed. So, in this case, the problem is 8 x 2KB = ~17KB to clear on each RPC call for an operation that is quite infrequently requested. In NFSD, typically the execution context is amenable to GFP_KERNEL, so it's generally OK to allocate dynamically for infrequently requested operations. -- Chuck Lever
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c index 3895eb52d2b1..22c5ccb83d20 100644 --- a/fs/nfsd/nfs4proc.c +++ b/fs/nfsd/nfs4proc.c @@ -1953,9 +1953,9 @@ nfsd4_copy_notify(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, /* For now, only return one server address in cpn_src, the * address used by the client to connect to this server. */ - cn->cpn_src.nl4_type = NL4_NETADDR; + cn->cpn_src->nl4_type = NL4_NETADDR; status = nfsd4_set_netaddr((struct sockaddr *)&rqstp->rq_daddr, - &cn->cpn_src.u.nl4_addr); + &cn->cpn_src->u.nl4_addr); WARN_ON_ONCE(status); if (status) { nfs4_put_cpntf_state(nn, cps); diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c index 358b3338c4cc..335431199077 100644 --- a/fs/nfsd/nfs4xdr.c +++ b/fs/nfsd/nfs4xdr.c @@ -1952,10 +1952,17 @@ nfsd4_decode_copy_notify(struct nfsd4_compoundargs *argp, { __be32 status; + cn->cpn_src = svcxdr_tmpalloc(argp, sizeof(*cn->cpn_src)); + if (cn->cpn_src == NULL) + return nfserrno(-ENOMEM); /* XXX: jukebox? */ + cn->cpn_dst = svcxdr_tmpalloc(argp, sizeof(*cn->cpn_dst)); + if (cn->cpn_dst == NULL) + return nfserrno(-ENOMEM); /* XXX: jukebox? */ + status = nfsd4_decode_stateid4(argp, &cn->cpn_src_stateid); if (status) return status; - return nfsd4_decode_nl4_server(argp, &cn->cpn_dst); + return nfsd4_decode_nl4_server(argp, cn->cpn_dst); } static __be32 @@ -4898,7 +4905,8 @@ nfsd4_encode_copy_notify(struct nfsd4_compoundres *resp, __be32 nfserr, *p++ = cpu_to_be32(1); - return nfsd42_encode_nl4_server(resp, &cn->cpn_src); + nfserr = nfsd42_encode_nl4_server(resp, cn->cpn_src); + return nfserr; } static __be32 diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h index 6e6a89008ce1..f253fc3f4708 100644 --- a/fs/nfsd/xdr4.h +++ b/fs/nfsd/xdr4.h @@ -595,13 +595,13 @@ struct nfsd4_offload_status { struct nfsd4_copy_notify { /* request */ stateid_t cpn_src_stateid; - struct nl4_server cpn_dst; + struct nl4_server *cpn_dst; /* response */ stateid_t cpn_cnr_stateid; u64 cpn_sec; u32 cpn_nsec; - struct nl4_server cpn_src; + struct nl4_server *cpn_src; }; struct nfsd4_op {
struct nfsd4_copy_notify is part of struct nfsd4_op, which resides in an 8-element array. sizeof(struct nfsd4_op): Before: /* size: 2208, cachelines: 35, members: 5 */ After: /* size: 1696, cachelines: 27, members: 5 */ Signed-off-by: Chuck Lever <chuck.lever@oracle.com> --- fs/nfsd/nfs4proc.c | 4 ++-- fs/nfsd/nfs4xdr.c | 12 ++++++++++-- fs/nfsd/xdr4.h | 4 ++-- 3 files changed, 14 insertions(+), 6 deletions(-)