| Message ID | 20191103012112.12212-2-aaptel@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4,1/6] cifs: sort interface list by speed | expand |
сб, 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. > > 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
> -----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
Hi, Tom Talpey <ttalpey@microsoft.com> writes: > 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. Note that the way the list is used has been altered in a later patch here: https://lore.kernel.org/linux-cifs/20191120161559.30295-2-aaptel@suse.com/T/#u Cheers,
> -----Original Message----- > From: Aurélien Aptel <aaptel@suse.com> > Sent: Tuesday, November 26, 2019 11:54 AM > To: Tom Talpey <ttalpey@microsoft.com>; Pavel Shilovsky > <piastryyy@gmail.com> > Cc: linux-cifs <linux-cifs@vger.kernel.org>; Steve French > <smfrench@gmail.com> > Subject: RE: [EXTERNAL] Re: [PATCH v4 1/6] cifs: sort interface list by speed > > Hi, > > Tom Talpey <ttalpey@microsoft.com> writes: > > 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. > > Note that the way the list is used has been altered in a later patch Hmm, that patch does help by taking RSS into account, but it does not take RDMA into consideration. The logic for that may be more complex, as it would need to determine whether an RDMA mount was desired, and an adapter available. And it looks like the code will try the RDMA addresses from other transport protocols. I also think it is more than a little too "heroic". Retrying seems unnecessary, doesn't the code refresh the list from time to time and reattempt connections? That's what the Windows client does, anyway. I would suggest avoiding any kind of retry except on the very first connection, and even then, many admins will want a fast-fail. Three TCP retries can take a long time! Tom. > here: > > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.ker > nel.org%2Flinux-cifs%2F20191120161559.30295-2- > aaptel%40suse.com%2FT%2F%23u&data=02%7C01%7Cttalpey%40micros > oft.com%7Cf76a99bd50ea4fbf4f7908d772914946%7C72f988bf86f141af91ab2 > d7cd011db47%7C1%7C0%7C637103840632982128&sdata=OJxqwDoV8C > CYNo69cgR9nwmc1xGR22s8J9Y9GDiV74M%3D&reserved=0 > > Cheers, > -- > Aurélien Aptel / SUSE Labs Samba Team > GPG: 1839 CB5F 9F5B FB9B AA97 8C99 03C8 A49B 521B D5D3 > SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE > GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)
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;
New channels are going to be opened by walking the list sequentially, so by sorting it we will connect to the fastest interfaces first. Signed-off-by: Aurelien Aptel <aaptel@suse.com> --- fs/cifs/smb2ops.c | 11 +++++++++++ 1 file changed, 11 insertions(+)