From mboxrd@z Thu Jan 1 00:00:00 1970 From: Leif Sahlberg Subject: Re: [PATCH] cifs: avoid a kmalloc in smb2_send_recv/SendReceive2 for the common case Date: Tue, 21 Nov 2017 19:18:43 -0500 (EST) Message-ID: <1876027630.29126264.1511309923566.JavaMail.zimbra@redhat.com> References: <20171121040807.8364-1-lsahlber@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: Steve French , linux-cifs To: Pavel Shilovskiy Return-path: In-Reply-To: Sender: linux-cifs-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-ID: ----- Original Message ----- > From: "Pavel Shilovskiy" > To: "Ronnie Sahlberg" , "Steve French" > Cc: "linux-cifs" > Sent: Wednesday, 22 November, 2017 11:06:38 AM > Subject: RE: [PATCH] cifs: avoid a kmalloc in smb2_send_recv/SendReceive2 for the common case > > > > 2017-11-20 20:08 GMT-08:00 Ronnie Sahlberg : > > In both functions, use an array of 8 (arbitrary but should be big enough > > for all current uses) iov and avoid having to kmalloc the array > > for the common case. > > > > If 8 is too small, then fall back to the original behaviour and use > > kmalloc/kfree. > > > > This should not change any behaviour but should save us a tiny amount of > > cpu cycles. > > > > Signed-off-by: Ronnie Sahlberg > > --- > > fs/cifs/transport.c | 33 +++++++++++++++++++++++---------- > > 1 file changed, 23 insertions(+), 10 deletions(-) > > > > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c > > index e678307bb7a0..510f41a435c8 100644 > > --- a/fs/cifs/transport.c > > +++ b/fs/cifs/transport.c > > @@ -38,6 +38,9 @@ > > #include "cifsproto.h" > > #include "cifs_debug.h" > > > > +/* Max number of iovectors we can use off the stack when sending requests. > > */ > > +#define CIFS_MAX_IOV_SIZE 8 > > + > > void > > cifs_wake_up_task(struct mid_q_entry *mid) > > { > > @@ -803,12 +806,16 @@ SendReceive2(const unsigned int xid, struct cifs_ses > > *ses, > > const int flags, struct kvec *resp_iov) > > { > > struct smb_rqst rqst; > > - struct kvec *new_iov; > > + struct kvec s_iov[CIFS_MAX_IOV_SIZE], *new_iov; > > int rc; > > > > - new_iov = kmalloc(sizeof(struct kvec) * (n_vec + 1), GFP_KERNEL); > > - if (!new_iov) > > - return -ENOMEM; > > + if (n_vec + 1 > CIFS_MAX_IOV_SIZE) { > > + new_iov = kmalloc(sizeof(struct kvec) * (n_vec + 1), > > + GFP_KERNEL); > > + if (!new_iov) > > + return -ENOMEM; > > + } else > > + new_iov = s_iov; > > > > /* 1st iov is a RFC1001 length followed by the rest of the packet > > */ > > memcpy(new_iov + 1, iov, (sizeof(struct kvec) * n_vec)); > > @@ -823,7 +830,8 @@ SendReceive2(const unsigned int xid, struct cifs_ses > > *ses, > > rqst.rq_nvec = n_vec + 1; > > > > rc = cifs_send_recv(xid, ses, &rqst, resp_buf_type, flags, > > resp_iov); > > - kfree(new_iov); > > + if (n_vec + 1 > CIFS_MAX_IOV_SIZE) > > + kfree(new_iov); > > return rc; > > } > > > > @@ -834,15 +842,19 @@ smb2_send_recv(const unsigned int xid, struct > > cifs_ses *ses, > > const int flags, struct kvec *resp_iov) > > { > > struct smb_rqst rqst; > > - struct kvec *new_iov; > > + struct kvec s_iov[CIFS_MAX_IOV_SIZE], *new_iov; > > int rc; > > int i; > > __u32 count; > > __be32 rfc1002_marker; > > > > - new_iov = kmalloc(sizeof(struct kvec) * (n_vec + 1), GFP_KERNEL); > > - if (!new_iov) > > - return -ENOMEM; > > + if (n_vec + 1 > CIFS_MAX_IOV_SIZE) { > > + new_iov = kmalloc(sizeof(struct kvec) * (n_vec + 1), > > + GFP_KERNEL); > > + if (!new_iov) > > + return -ENOMEM; > > + } else > > + new_iov = s_iov; > > > > /* 1st iov is an RFC1002 Session Message length */ > > memcpy(new_iov + 1, iov, (sizeof(struct kvec) * n_vec)); > > @@ -861,7 +873,8 @@ smb2_send_recv(const unsigned int xid, struct cifs_ses > > *ses, > > rqst.rq_nvec = n_vec + 1; > > > > rc = cifs_send_recv(xid, ses, &rqst, resp_buf_type, flags, > > resp_iov); > > - kfree(new_iov); > > + if (n_vec + 1 > CIFS_MAX_IOV_SIZE) > > + kfree(new_iov); > > return rc; > > } > > > > -- > > 2.13.3 > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in > > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > Thanks for the patch! > > Should we split it into two and mark the 1st part for stable? This would > address possible performance degradation in previous kernels because > SendReceive2() was changed to use kmalloc() several kernels ago. I didn't > measure a performance difference, so not sure if it is worth the effort. Don't know. I leave that to Steve. It is low risk and probably improves performance a little bit, but it is not a bug so ... > > In both cases (as one patch or two), you can add my > > Reviewed-by: Pavel Shilovsky > > -- > Best regards, > Pavel Shilovsky >