[PATCH] cifs: avoid a kmalloc in smb2_send_recv/SendReceive2 for the common case

[PATCH] cifs: avoid a kmalloc in smb2_send_recv/SendReceive2 for the common case

[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 <lsahlber-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 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

RE: [PATCH] cifs: avoid a kmalloc in smb2_send_recv/SendReceive2 for the common case

[Pavel Shilovskiy]

2017-11-20 20:08 GMT-08:00 Ronnie Sahlberg <lsahlber-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> 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 <lsahlber-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  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.

In both cases (as one patch or two), you can add my

Reviewed-by: Pavel Shilovsky <pshilov-0li6OtcxBFHby3iVrkZq2A@public.gmane.org>

--
Best regards,
Pavel Shilovsky

Re: [PATCH] cifs: avoid a kmalloc in smb2_send_recv/SendReceive2 for the common case

[Leif Sahlberg]

----- Original Message -----
> From: "Pavel Shilovskiy" <pshilov-0li6OtcxBFHby3iVrkZq2A@public.gmane.org>
> To: "Ronnie Sahlberg" <lsahlber-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>, "Steve French" <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: "linux-cifs" <linux-cifs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
> 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 <lsahlber-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> > 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 <lsahlber-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >  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 <pshilov-0li6OtcxBFHby3iVrkZq2A@public.gmane.org>
> 
> --
> Best regards,
> Pavel Shilovsky
>