RE: [EXTERNAL] Re: [PATCH v4 1/6] cifs: sort interface list by speed

[Tom Talpey <ttalpey@microsoft.com>]

> -----Original Message-----
> From: linux-cifs-owner@vger.kernel.org <linux-cifs-owner@vger.kernel.org> On
> Behalf Of Pavel Shilovsky
> Sent: Monday, November 25, 2019 4:29 PM
> To: Aurelien Aptel <aaptel@suse.com>
> Cc: linux-cifs <linux-cifs@vger.kernel.org>; Steve French
> <smfrench@gmail.com>
> Subject: [EXTERNAL] Re: [PATCH v4 1/6] cifs: sort interface list by speed
> 
> сб, 2 нояб. 2019 г. в 18:21, Aurelien Aptel <aaptel@suse.com>:
> >
> > New channels are going to be opened by walking the list sequentially,
> > so by sorting it we will connect to the fastest interfaces first.

Sorting by speed is definitely appropriate, but sorting the other
multichannel attributes is just as important. For example, if the
RDMA attribute is set, the address should actually be excluded
for non-RDMA connections (a second, non-RDMA entry is included,
if the interface supports both). And, the RSS attribute indicates a
"better" destination than non-RSS for a given speed, because it is
more efficient at local traffic management.

Suggest a followon effort to take advantage of these, by excluding
ineligible paths, and raising the sort priority of others.

Tom.

> > Signed-off-by: Aurelien Aptel <aaptel@suse.com>
> > ---
> >  fs/cifs/smb2ops.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> > index cd55af9b7cc5..ea634581791a 100644
> > --- a/fs/cifs/smb2ops.c
> > +++ b/fs/cifs/smb2ops.c
> > @@ -10,6 +10,7 @@
> >  #include <linux/falloc.h>
> >  #include <linux/scatterlist.h>
> >  #include <linux/uuid.h>
> > +#include <linux/sort.h>
> >  #include <crypto/aead.h>
> >  #include "cifsglob.h"
> >  #include "smb2pdu.h"
> > @@ -558,6 +559,13 @@ parse_server_interfaces(struct
> network_interface_info_ioctl_rsp *buf,
> >         return rc;
> >  }
> >
> > +static int compare_iface(const void *ia, const void *ib)
> > +{
> > +       const struct cifs_server_iface *a = (struct cifs_server_iface *)ia;
> > +       const struct cifs_server_iface *b = (struct cifs_server_iface *)ib;
> > +
> > +       return a->speed == b->speed ? 0 : (a->speed > b->speed ? -1 : 1);
> > +}
> >
> >  static int
> >  SMB3_request_interfaces(const unsigned int xid, struct cifs_tcon *tcon)
> > @@ -587,6 +595,9 @@ SMB3_request_interfaces(const unsigned int xid,
> struct cifs_tcon *tcon)
> >         if (rc)
> >                 goto out;
> >
> > +       /* sort interfaces from fastest to slowest */
> > +       sort(iface_list, iface_count, sizeof(*iface_list), compare_iface, NULL);
> > +
> >         spin_lock(&ses->iface_lock);
> >         kfree(ses->iface_list);
> >         ses->iface_list = iface_list;
> > --
> > 2.16.4
> >
> 
> Looks good at the first glance, thanks!
> 
> @Steve, you may add
> 
> Acked-by: Pavel Shilovsky <pshilov@microsoft.com>
> 
> to this and other patches in the series.
> 
> --
> Best regards,
> Pavel Shilovsky