Connection sharing in SMB multichannel

Connection sharing in SMB multichannel

[Shyam Prasad N]

Hi all,

I wanted to revisit the way we do a few things while doing
multichannel mounts in cifs.ko:

1.
The way connections are organized today, the connections of primary
channels of sessions can be shared among different sessions and their
channels. However, connections to secondary channels are not shared.
i.e. created with nosharesock.
Is there a reason why we have it that way?
We could have a pool of connections for a particular server. When new
channels are to be created for a session, we could simply pick
connections from this pool.
Another approach could be not to share sockets for any of the channels
of multichannel mounts. This way, multichannel would implicitly mean
nosharesock. Assuming that multichannel is being used for performance
reasons, this would actually make a lot of sense. Each channel would
create new connection to the server, and take advantage of number of
interfaces and RSS capabilities of server interfaces.
I'm planning to take the latter approach for now, since it's easier.
Please let me know about your opinions on this.

2.
Today, the interface list for a server hangs off the session struct. Why?
Doesn't it make more sense to hang it off the server struct? With my
recent changes to query the interface list from the server
periodically, each tcon is querying this and keeping the results in
the session struct.
I plan to move this to the server struct too. And avoid having to
query this too many times unnecessarily. Please let me know if you see
a reason not to do this.

3.
I saw that there was a bug in iface_cmp, where we did not do full
comparison of addresses to match them.
Fixed it here:
https://github.com/sprasad-microsoft/smb3-kernel-client/commit/cef2448dc43d1313571e21ce8283bccacf01978e.patch

@Tom Talpey Was this your concern with iface_cmp?

4.
I also feel that the way an interface is selected today for
multichannel will not scale.
We keep selecting the fastest server interface, if it supports RSS.
IMO, we should be distributing the requests among the server
interfaces, based on the interface speed adveritsed.
Something on these lines:
https://github.com/sprasad-microsoft/smb3-kernel-client/commit/ebe1ac3426111a872d19fea41de365b1b3aca0fe.patch

The above patch assigns weights to each interface (which is a function
of it's advertised speed). The weight is 1 for the interface that is
advertising minimum speed, and for any interface that does not support
RSS.
Please let me know if you have any opinions on this change.

====
Also, I did not find a good way to test out these changes yet i.e.
customize and change the QueryInterface response from the server on
successive requests.
So I've requested Steve not to take this into his branch yet.

I'm thinking I'll hard code the client code to generate different set
of dummy interfaces on every QueryInterface call.
Any ideas on how I can test this more easily will be appreciated.

-- 
Regards,
Shyam

Re: Connection sharing in SMB multichannel

[Aurélien Aptel]

Hey Shyam,

I remember thinking that channels should be part of the server too
when I started working on this but switched it up to session as I kept
working on it and finding it was the right choice.
I don't remember all the details so my comments will be a bit vague.

On Tue, Jan 10, 2023 at 10:16 AM Shyam Prasad N <nspmangalore@gmail.com> wrote:
> 1.
> The way connections are organized today, the connections of primary
> channels of sessions can be shared among different sessions and their
> channels. However, connections to secondary channels are not shared.
> i.e. created with nosharesock.
> Is there a reason why we have it that way?
> We could have a pool of connections for a particular server. When new
> channels are to be created for a session, we could simply pick
> connections from this pool.
> Another approach could be not to share sockets for any of the channels
> of multichannel mounts. This way, multichannel would implicitly mean
> nosharesock. Assuming that multichannel is being used for performance
> reasons, this would actually make a lot of sense. Each channel would
> create new connection to the server, and take advantage of number of
> interfaces and RSS capabilities of server interfaces.
> I'm planning to take the latter approach for now, since it's easier.
> Please let me know about your opinions on this.

First, in the abstract models, Channels are kept in the Session object.
https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-smb2/8174c219-2224-4009-b96a-06d84eccb3ae

Channels and sessions are intertwined. Channels signing keys depend on
the session it is connected to (See "3.2.5.3.1 Handling a New
Authentication" and "3.2.5.3.3 Handling Session Binding").
Think carefully on what should be done on disconnect/reconnect.
Especially if the channel is shared with multiple sessions.

Problem with the pool approach is mount options might require
different connections so sharing is not so easy. And reconnecting
might involve different fallbacks (dfs) for different sessions.

You should see the server struct as the "final destination". Once it's
picked we know it's going there.

> 2.
> Today, the interface list for a server hangs off the session struct. Why?
> Doesn't it make more sense to hang it off the server struct? With my
> recent changes to query the interface list from the server
> periodically, each tcon is querying this and keeping the results in
> the session struct.
> I plan to move this to the server struct too. And avoid having to
> query this too many times unnecessarily. Please let me know if you see
> a reason not to do this.

It's more convenient to have the interface list at the same place as
the channel list but it could be moved I suppose.
In the abstract model it's in the server apparently.

> 4.
> I also feel that the way an interface is selected today for
> multichannel will not scale.
> We keep selecting the fastest server interface, if it supports RSS.
> IMO, we should be distributing the requests among the server
> interfaces, based on the interface speed adveritsed.

RSS means the interface can process packets in parallel queues. The
problem is we don't know how many queues it has.
I'm not sure you can find an optimal algorithm for all NIC
vendor/driver combinations. Probably you need to do some tests with a
bunch of different HW or find someone knowledgeable.
From my small experience now at mellanox/nvidia I have yet to see less
than 8 rx/combined queues. You can get/set the number with ethtool
-l/-L.
I've set the max channel connection to 16 at the time but I still
don't know what large scale high-speed deployment of SMB look like.
For what it's worth, in the NVMe-TCP tests I'm doing at the moment and
the systems we use to test (fio reads with a 100gbs eth nic with 63 hw
queues, 96 cores cpu on the host&target, reading from a null block
target), we get diminishing returns around 24 parallel connections. I
don't know how transferable this data point is.

On that topic, for best performance, some possible future project
could be to assign steering rules on the client to force each channel
packet processing on different cpus and making sure the cpus are the
same as the demultiplex thread (avoids context switches and
contentions). See
https://www.kernel.org/doc/Documentation/networking/scaling.txt
(warning, not an easy read lol)

Cheers,

Re: Connection sharing in SMB multichannel

[Enzo Matsumiya]

Hi Shyam,

I 100% agree with your proposed changes. I came up with an idea and got
to draw a diagram a while ago that would handle multichannel connections
in a similar way you propose here.

https://exis.tech/cifs-multicore-multichannel.png

The main idea is to have the 'channels' (sockets) pool for a particular
server, but also create submission/receiving queues (SQ/RQ) (similar to NVMe
and io_uring) for each online CPU.  Then each CPU/queue is free to send
to any of the available channels, based on whatever algorithm is chosen
(RR, free channel, fastest NIC, etc).

I still haven't got time to design the receiving flow, but it shouldn't
change much from the reverse of the sending side.

Of course, there's a lot to improve/fix in that design, but something I
thought would enhance cifs multichannel performance a lot.

I've discussed this idea with Paulo a while back, and he already
provided me with great improvements/fixes for this.  Since I still
haven't got the time to work on it, I hope this to serve as inspiration.


Cheers,

Enzo

On 01/10, Shyam Prasad N wrote:
>Hi all,
>
>I wanted to revisit the way we do a few things while doing
>multichannel mounts in cifs.ko:
>
>1.
>The way connections are organized today, the connections of primary
>channels of sessions can be shared among different sessions and their
>channels. However, connections to secondary channels are not shared.
>i.e. created with nosharesock.
>Is there a reason why we have it that way?
>We could have a pool of connections for a particular server. When new
>channels are to be created for a session, we could simply pick
>connections from this pool.
>Another approach could be not to share sockets for any of the channels
>of multichannel mounts. This way, multichannel would implicitly mean
>nosharesock. Assuming that multichannel is being used for performance
>reasons, this would actually make a lot of sense. Each channel would
>create new connection to the server, and take advantage of number of
>interfaces and RSS capabilities of server interfaces.
>I'm planning to take the latter approach for now, since it's easier.
>Please let me know about your opinions on this.
>
>2.
>Today, the interface list for a server hangs off the session struct. Why?
>Doesn't it make more sense to hang it off the server struct? With my
>recent changes to query the interface list from the server
>periodically, each tcon is querying this and keeping the results in
>the session struct.
>I plan to move this to the server struct too. And avoid having to
>query this too many times unnecessarily. Please let me know if you see
>a reason not to do this.
>
>3.
>I saw that there was a bug in iface_cmp, where we did not do full
>comparison of addresses to match them.
>Fixed it here:
>https://github.com/sprasad-microsoft/smb3-kernel-client/commit/cef2448dc43d1313571e21ce8283bccacf01978e.patch
>
>@Tom Talpey Was this your concern with iface_cmp?
>
>4.
>I also feel that the way an interface is selected today for
>multichannel will not scale.
>We keep selecting the fastest server interface, if it supports RSS.
>IMO, we should be distributing the requests among the server
>interfaces, based on the interface speed adveritsed.
>Something on these lines:
>https://github.com/sprasad-microsoft/smb3-kernel-client/commit/ebe1ac3426111a872d19fea41de365b1b3aca0fe.patch
>
>The above patch assigns weights to each interface (which is a function
>of it's advertised speed). The weight is 1 for the interface that is
>advertising minimum speed, and for any interface that does not support
>RSS.
>Please let me know if you have any opinions on this change.
>
>====
>Also, I did not find a good way to test out these changes yet i.e.
>customize and change the QueryInterface response from the server on
>successive requests.
>So I've requested Steve not to take this into his branch yet.
>
>I'm thinking I'll hard code the client code to generate different set
>of dummy interfaces on every QueryInterface call.
>Any ideas on how I can test this more easily will be appreciated.
>
>-- 
>Regards,
>Shyam

Re: Connection sharing in SMB multichannel

[Paulo Alcantara]

Aurélien Aptel <aurelien.aptel@gmail.com> writes:

> Hey Shyam,
>
> I remember thinking that channels should be part of the server too
> when I started working on this but switched it up to session as I kept
> working on it and finding it was the right choice.
> I don't remember all the details so my comments will be a bit vague.
>
> On Tue, Jan 10, 2023 at 10:16 AM Shyam Prasad N <nspmangalore@gmail.com> wrote:
>> 1.
>> The way connections are organized today, the connections of primary
>> channels of sessions can be shared among different sessions and their
>> channels. However, connections to secondary channels are not shared.
>> i.e. created with nosharesock.
>> Is there a reason why we have it that way?
>> We could have a pool of connections for a particular server. When new
>> channels are to be created for a session, we could simply pick
>> connections from this pool.
>> Another approach could be not to share sockets for any of the channels
>> of multichannel mounts. This way, multichannel would implicitly mean
>> nosharesock. Assuming that multichannel is being used for performance
>> reasons, this would actually make a lot of sense. Each channel would
>> create new connection to the server, and take advantage of number of
>> interfaces and RSS capabilities of server interfaces.
>> I'm planning to take the latter approach for now, since it's easier.
>> Please let me know about your opinions on this.
>
> First, in the abstract models, Channels are kept in the Session object.
> https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-smb2/8174c219-2224-4009-b96a-06d84eccb3ae
>
> Channels and sessions are intertwined. Channels signing keys depend on
> the session it is connected to (See "3.2.5.3.1 Handling a New
> Authentication" and "3.2.5.3.3 Handling Session Binding").
> Think carefully on what should be done on disconnect/reconnect.
> Especially if the channel is shared with multiple sessions.
>
> Problem with the pool approach is mount options might require
> different connections so sharing is not so easy. And reconnecting
> might involve different fallbacks (dfs) for different sessions.

AFAICT, for a mount with N channels, whether DFS or not, there is a
wrong assumption that after failover, there will be N channels to be
reconnected.

There might be a case where the administrator disabled multichannel in
server settings, or decreased the number of available adapters, or if
the new DFS target doesn't support multichannel or has fewer channels.
In this case, cifs.ko would have stale channels trying to reconnect
indefinitely.

Please correct me if I missed something.

Re: Connection sharing in SMB multichannel

[Tom Talpey]

On 1/10/2023 8:00 AM, Aurélien Aptel wrote:
> Hey Shyam,
> 
> I remember thinking that channels should be part of the server too
> when I started working on this but switched it up to session as I kept
> working on it and finding it was the right choice.

Channels are absolutely a property of the session. Server multichannel
enables their use, but the decision to actually use them is per-session.

> I don't remember all the details so my comments will be a bit vague.
> 
> On Tue, Jan 10, 2023 at 10:16 AM Shyam Prasad N <nspmangalore@gmail.com> wrote:
>> 1.
>> The way connections are organized today, the connections of primary
>> channels of sessions can be shared among different sessions and their
>> channels. However, connections to secondary channels are not shared.
>> i.e. created with nosharesock.
>> Is there a reason why we have it that way?

That sounds wrong. Normally we want to share connections, to preserve
resources. Howe3ver, for certain greedy flows, it may be a good idea to
create dedicated channels (connections). This is a policy decision, and
should not be wired-in.

>> We could have a pool of connections for a particular server. When new
>> channels are to be created for a session, we could simply pick
>> connections from this pool.

Yes and no. There are similar reasons to avoid using pooled channels.
Performance is one obvious one - some connections may be faster (RDMA),
or tuned for specific advantages (lower latency, more secure). Selection
of channel should allow for certain types of filtering.

>> Another approach could be not to share sockets for any of the channels
>> of multichannel mounts. This way, multichannel would implicitly mean
>> nosharesock. Assuming that multichannel is being used for performance
>> reasons, this would actually make a lot of sense. Each channel would
>> create new connection to the server, and take advantage of number of
>> interfaces and RSS capabilities of server interfaces.
>> I'm planning to take the latter approach for now, since it's easier.
>> Please let me know about your opinions on this.
> 
> First, in the abstract models, Channels are kept in the Session object.
> https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-smb2/8174c219-2224-4009-b96a-06d84eccb3ae
> 
> Channels and sessions are intertwined. Channels signing keys depend on
> the session it is connected to (See "3.2.5.3.1 Handling a New
> Authentication" and "3.2.5.3.3 Handling Session Binding").
> Think carefully on what should be done on disconnect/reconnect.
> Especially if the channel is shared with multiple sessions.

Yes, absolutely. Shared channels require careful processing when a
disconnect is used. It's incredibly rude to close a shared connection.

> Problem with the pool approach is mount options might require
> different connections so sharing is not so easy. And reconnecting
> might involve different fallbacks (dfs) for different sessions.
> 
> You should see the server struct as the "final destination". Once it's
> picked we know it's going there.
> 
>> 2.
>> Today, the interface list for a server hangs off the session struct. Why?
>> Doesn't it make more sense to hang it off the server struct? With my
>> recent changes to query the interface list from the server
>> periodically, each tcon is querying this and keeping the results in
>> the session struct.
>> I plan to move this to the server struct too. And avoid having to
>> query this too many times unnecessarily. Please let me know if you see
>> a reason not to do this.

This makes sense, but only if the channel selection is sensibly
filterable. A non-RDMA mount should not be selecting an RDMA
channel. An encrypted share may want to prefer an adapter which
can support bus or CPU affinity. Etc.

> It's more convenient to have the interface list at the same place as
> the channel list but it could be moved I suppose.
> In the abstract model it's in the server apparently.
> 
>> 4.
>> I also feel that the way an interface is selected today for
>> multichannel will not scale.
>> We keep selecting the fastest server interface, if it supports RSS.
>> IMO, we should be distributing the requests among the server
>> interfaces, based on the interface speed adveritsed.

That's part of it, definitely. "Fastest" is hard to measure. Fast
as in latency? Bandwidth? CPU overhead?

> RSS means the interface can process packets in parallel queues. The
> problem is we don't know how many queues it has.

Yup. RSS is only a hint. It's not a metric.

> I'm not sure you can find an optimal algorithm for all NIC
> vendor/driver combinations. Probably you need to do some tests with a
> bunch of different HW or find someone knowledgeable.
>  From my small experience now at mellanox/nvidia I have yet to see less
> than 8 rx/combined queues. You can get/set the number with ethtool
> -l/-L.

RSS is only meaningful on the machine doing the receiving, in this
case, the server. The client has no clue how to optimize that. But
assuming a handful of RSS queues is a good idea. Personally I'd
avoid a number as large as 8. But over time, cores are increasing,
so it's a crapshoot.

> I've set the max channel connection to 16 at the time but I still
> don't know what large scale high-speed deployment of SMB look like.
> For what it's worth, in the NVMe-TCP tests I'm doing at the moment and
> the systems we use to test (fio reads with a 100gbs eth nic with 63 hw
> queues, 96 cores cpu on the host&target, reading from a null block
> target), we get diminishing returns around 24 parallel connections. I
> don't know how transferable this data point is.

16 seems high, especially on a gigabit interface. At hundreds of Gb,
well, that's possibly different. I'd avoid making too many assumptions
(guesses!).

Tom.

> On that topic, for best performance, some possible future project
> could be to assign steering rules on the client to force each channel
> packet processing on different cpus and making sure the cpus are the
> same as the demultiplex thread (avoids context switches and
> contentions). See
> https://www.kernel.org/doc/Documentation/networking/scaling.txt
> (warning, not an easy read lol)
> 
> Cheers,
> 

Re: Connection sharing in SMB multichannel

[Shyam Prasad N]

Hi Aurélien,

Thanks for the inputs.

On Tue, Jan 10, 2023 at 6:30 PM Aurélien Aptel <aurelien.aptel@gmail.com> wrote:
>
> Hey Shyam,
>
> I remember thinking that channels should be part of the server too
> when I started working on this but switched it up to session as I kept
> working on it and finding it was the right choice.
> I don't remember all the details so my comments will be a bit vague.

I'm not proposing to move the channels to per-server here, but to move
the connections to per-server. Each channel is associated with a
connection.
I think our use of server and connection interchangeably is causing
this confusion. :)

>
> On Tue, Jan 10, 2023 at 10:16 AM Shyam Prasad N <nspmangalore@gmail.com> wrote:
> > 1.
> > The way connections are organized today, the connections of primary
> > channels of sessions can be shared among different sessions and their
> > channels. However, connections to secondary channels are not shared.
> > i.e. created with nosharesock.
> > Is there a reason why we have it that way?
> > We could have a pool of connections for a particular server. When new
> > channels are to be created for a session, we could simply pick
> > connections from this pool.
> > Another approach could be not to share sockets for any of the channels
> > of multichannel mounts. This way, multichannel would implicitly mean
> > nosharesock. Assuming that multichannel is being used for performance
> > reasons, this would actually make a lot of sense. Each channel would
> > create new connection to the server, and take advantage of number of
> > interfaces and RSS capabilities of server interfaces.
> > I'm planning to take the latter approach for now, since it's easier.
> > Please let me know about your opinions on this.
>
> First, in the abstract models, Channels are kept in the Session object.
> https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-smb2/8174c219-2224-4009-b96a-06d84eccb3ae
>
> Channels and sessions are intertwined. Channels signing keys depend on
> the session it is connected to (See "3.2.5.3.1 Handling a New
> Authentication" and "3.2.5.3.3 Handling Session Binding").
> Think carefully on what should be done on disconnect/reconnect.
> Especially if the channel is shared with multiple sessions.

That's a good point.
But that affects even other cases like multiuser mounts and cases
where a session shares a socket. Doesn't it?
I think we call cifs_reconnect very generously today. We should give
this another look, IMO.

>
> Problem with the pool approach is mount options might require
> different connections so sharing is not so easy. And reconnecting
> might involve different fallbacks (dfs) for different sessions.
>
> You should see the server struct as the "final destination". Once it's
> picked we know it's going there.
>
> > 2.
> > Today, the interface list for a server hangs off the session struct. Why?
> > Doesn't it make more sense to hang it off the server struct? With my
> > recent changes to query the interface list from the server
> > periodically, each tcon is querying this and keeping the results in
> > the session struct.
> > I plan to move this to the server struct too. And avoid having to
> > query this too many times unnecessarily. Please let me know if you see
> > a reason not to do this.
>
> It's more convenient to have the interface list at the same place as
> the channel list but it could be moved I suppose.
> In the abstract model it's in the server apparently.

Yeah. It makes more sense to keep it on per-server basis.

>
> > 4.
> > I also feel that the way an interface is selected today for
> > multichannel will not scale.
> > We keep selecting the fastest server interface, if it supports RSS.
> > IMO, we should be distributing the requests among the server
> > interfaces, based on the interface speed adveritsed.
>
> RSS means the interface can process packets in parallel queues. The
> problem is we don't know how many queues it has.
> I'm not sure you can find an optimal algorithm for all NIC
> vendor/driver combinations. Probably you need to do some tests with a
> bunch of different HW or find someone knowledgeable.
> From my small experience now at mellanox/nvidia I have yet to see less
> than 8 rx/combined queues. You can get/set the number with ethtool
> -l/-L.
> I've set the max channel connection to 16 at the time but I still
> don't know what large scale high-speed deployment of SMB look like.
> For what it's worth, in the NVMe-TCP tests I'm doing at the moment and
> the systems we use to test (fio reads with a 100gbs eth nic with 63 hw
> queues, 96 cores cpu on the host&target, reading from a null block
> target), we get diminishing returns around 24 parallel connections. I
> don't know how transferable this data point is.
>
> On that topic, for best performance, some possible future project
> could be to assign steering rules on the client to force each channel
> packet processing on different cpus and making sure the cpus are the
> same as the demultiplex thread (avoids context switches and
> contentions). See
> https://www.kernel.org/doc/Documentation/networking/scaling.txt
> (warning, not an easy read lol)

Interesting. I'm guessing that this is the "send side scaling" that's
mentioned here:
https://learn.microsoft.com/en-us/previous-versions/windows/it-pro/windows-server-2012-r2-and-2012/hh997036(v=ws.11)

>
> Cheers,



-- 
Regards,
Shyam

Re: Connection sharing in SMB multichannel

[Shyam Prasad N]

On Tue, Jan 10, 2023 at 10:48 PM Enzo Matsumiya <ematsumiya@suse.de> wrote:
>
> Hi Shyam,
>
> I 100% agree with your proposed changes. I came up with an idea and got
> to draw a diagram a while ago that would handle multichannel connections
> in a similar way you propose here.
>
> https://exis.tech/cifs-multicore-multichannel.png
>
> The main idea is to have the 'channels' (sockets) pool for a particular
> server, but also create submission/receiving queues (SQ/RQ) (similar to NVMe
> and io_uring) for each online CPU.  Then each CPU/queue is free to send
> to any of the available channels, based on whatever algorithm is chosen
> (RR, free channel, fastest NIC, etc).
>
> I still haven't got time to design the receiving flow, but it shouldn't
> change much from the reverse of the sending side.
>
> Of course, there's a lot to improve/fix in that design, but something I
> thought would enhance cifs multichannel performance a lot.
>
> I've discussed this idea with Paulo a while back, and he already
> provided me with great improvements/fixes for this.  Since I still
> haven't got the time to work on it, I hope this to serve as inspiration.
>
>
> Cheers,
>
> Enzo

Hi Enzo,

This is some detailed analysis here.
I'm not sure that I fully understand the proposed changes though.
We should discuss this further.

>
> On 01/10, Shyam Prasad N wrote:
> >Hi all,
> >
> >I wanted to revisit the way we do a few things while doing
> >multichannel mounts in cifs.ko:
> >
> >1.
> >The way connections are organized today, the connections of primary
> >channels of sessions can be shared among different sessions and their
> >channels. However, connections to secondary channels are not shared.
> >i.e. created with nosharesock.
> >Is there a reason why we have it that way?
> >We could have a pool of connections for a particular server. When new
> >channels are to be created for a session, we could simply pick
> >connections from this pool.
> >Another approach could be not to share sockets for any of the channels
> >of multichannel mounts. This way, multichannel would implicitly mean
> >nosharesock. Assuming that multichannel is being used for performance
> >reasons, this would actually make a lot of sense. Each channel would
> >create new connection to the server, and take advantage of number of
> >interfaces and RSS capabilities of server interfaces.
> >I'm planning to take the latter approach for now, since it's easier.
> >Please let me know about your opinions on this.
> >
> >2.
> >Today, the interface list for a server hangs off the session struct. Why?
> >Doesn't it make more sense to hang it off the server struct? With my
> >recent changes to query the interface list from the server
> >periodically, each tcon is querying this and keeping the results in
> >the session struct.
> >I plan to move this to the server struct too. And avoid having to
> >query this too many times unnecessarily. Please let me know if you see
> >a reason not to do this.
> >
> >3.
> >I saw that there was a bug in iface_cmp, where we did not do full
> >comparison of addresses to match them.
> >Fixed it here:
> >https://github.com/sprasad-microsoft/smb3-kernel-client/commit/cef2448dc43d1313571e21ce8283bccacf01978e.patch
> >
> >@Tom Talpey Was this your concern with iface_cmp?
> >
> >4.
> >I also feel that the way an interface is selected today for
> >multichannel will not scale.
> >We keep selecting the fastest server interface, if it supports RSS.
> >IMO, we should be distributing the requests among the server
> >interfaces, based on the interface speed adveritsed.
> >Something on these lines:
> >https://github.com/sprasad-microsoft/smb3-kernel-client/commit/ebe1ac3426111a872d19fea41de365b1b3aca0fe.patch
> >
> >The above patch assigns weights to each interface (which is a function
> >of it's advertised speed). The weight is 1 for the interface that is
> >advertising minimum speed, and for any interface that does not support
> >RSS.
> >Please let me know if you have any opinions on this change.
> >
> >====
> >Also, I did not find a good way to test out these changes yet i.e.
> >customize and change the QueryInterface response from the server on
> >successive requests.
> >So I've requested Steve not to take this into his branch yet.
> >
> >I'm thinking I'll hard code the client code to generate different set
> >of dummy interfaces on every QueryInterface call.
> >Any ideas on how I can test this more easily will be appreciated.
> >
> >--
> >Regards,
> >Shyam



-- 
Regards,
Shyam

Re: Connection sharing in SMB multichannel

[Shyam Prasad N]

On Tue, Jan 10, 2023 at 11:11 PM Paulo Alcantara <pc@cjr.nz> wrote:
>
> Aurélien Aptel <aurelien.aptel@gmail.com> writes:
>
> > Hey Shyam,
> >
> > I remember thinking that channels should be part of the server too
> > when I started working on this but switched it up to session as I kept
> > working on it and finding it was the right choice.
> > I don't remember all the details so my comments will be a bit vague.
> >
> > On Tue, Jan 10, 2023 at 10:16 AM Shyam Prasad N <nspmangalore@gmail.com> wrote:
> >> 1.
> >> The way connections are organized today, the connections of primary
> >> channels of sessions can be shared among different sessions and their
> >> channels. However, connections to secondary channels are not shared.
> >> i.e. created with nosharesock.
> >> Is there a reason why we have it that way?
> >> We could have a pool of connections for a particular server. When new
> >> channels are to be created for a session, we could simply pick
> >> connections from this pool.
> >> Another approach could be not to share sockets for any of the channels
> >> of multichannel mounts. This way, multichannel would implicitly mean
> >> nosharesock. Assuming that multichannel is being used for performance
> >> reasons, this would actually make a lot of sense. Each channel would
> >> create new connection to the server, and take advantage of number of
> >> interfaces and RSS capabilities of server interfaces.
> >> I'm planning to take the latter approach for now, since it's easier.
> >> Please let me know about your opinions on this.
> >
> > First, in the abstract models, Channels are kept in the Session object.
> > https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-smb2/8174c219-2224-4009-b96a-06d84eccb3ae
> >
> > Channels and sessions are intertwined. Channels signing keys depend on
> > the session it is connected to (See "3.2.5.3.1 Handling a New
> > Authentication" and "3.2.5.3.3 Handling Session Binding").
> > Think carefully on what should be done on disconnect/reconnect.
> > Especially if the channel is shared with multiple sessions.
> >
> > Problem with the pool approach is mount options might require
> > different connections so sharing is not so easy. And reconnecting
> > might involve different fallbacks (dfs) for different sessions.
>
> AFAICT, for a mount with N channels, whether DFS or not, there is a
> wrong assumption that after failover, there will be N channels to be
> reconnected.
>
> There might be a case where the administrator disabled multichannel in
> server settings, or decreased the number of available adapters, or if
> the new DFS target doesn't support multichannel or has fewer channels.
> In this case, cifs.ko would have stale channels trying to reconnect
> indefinitely.
>
> Please correct me if I missed something.

Hi Paulo,

Your analysis is correct. We don't support scaling down the channel
count. Again, this was always the case.
I did have some changes planned to deal with that scenario too.
Haven't managed to implement it though.

-- 
Regards,
Shyam

Re: Connection sharing in SMB multichannel

[Shyam Prasad N]

Hi Tom,

Thanks for the review.
Please read my comments below.

On Wed, Jan 11, 2023 at 7:47 AM Tom Talpey <tom@talpey.com> wrote:
>
> On 1/10/2023 8:00 AM, Aurélien Aptel wrote:
> > Hey Shyam,
> >
> > I remember thinking that channels should be part of the server too
> > when I started working on this but switched it up to session as I kept
> > working on it and finding it was the right choice.
>
> Channels are absolutely a property of the session. Server multichannel
> enables their use, but the decision to actually use them is per-session.

Agreed. I'm not proposing to change that.
>
> > I don't remember all the details so my comments will be a bit vague.
> >
> > On Tue, Jan 10, 2023 at 10:16 AM Shyam Prasad N <nspmangalore@gmail.com> wrote:
> >> 1.
> >> The way connections are organized today, the connections of primary
> >> channels of sessions can be shared among different sessions and their
> >> channels. However, connections to secondary channels are not shared.
> >> i.e. created with nosharesock.
> >> Is there a reason why we have it that way?
>
> That sounds wrong. Normally we want to share connections, to preserve
> resources. Howe3ver, for certain greedy flows, it may be a good idea to
> create dedicated channels (connections). This is a policy decision, and
> should not be wired-in.

Yeah. We only share primary channels today with other sessions.
I plan to change this in two stages. First, will enforce nosharesock
for multichannel.
I'll submit another patch later to reuse existing connections, unless
the user has explicitly used nosharesock as a mount option.

>
> >> We could have a pool of connections for a particular server. When new
> >> channels are to be created for a session, we could simply pick
> >> connections from this pool.
>
> Yes and no. There are similar reasons to avoid using pooled channels.
> Performance is one obvious one - some connections may be faster (RDMA),
> or tuned for specific advantages (lower latency, more secure). Selection
> of channel should allow for certain types of filtering.

For users looking for performance, we could advise that they mount
with nosharesock, in addition to multichannel. That way, those
connections would be dedicated to only that session, and will not be
shared.
For others (maybe they're using multichannel for network tolerance),
we make use of connection pool.

>
> >> Another approach could be not to share sockets for any of the channels
> >> of multichannel mounts. This way, multichannel would implicitly mean
> >> nosharesock. Assuming that multichannel is being used for performance
> >> reasons, this would actually make a lot of sense. Each channel would
> >> create new connection to the server, and take advantage of number of
> >> interfaces and RSS capabilities of server interfaces.
> >> I'm planning to take the latter approach for now, since it's easier.
> >> Please let me know about your opinions on this.
> >
> > First, in the abstract models, Channels are kept in the Session object.
> > https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-smb2/8174c219-2224-4009-b96a-06d84eccb3ae
> >
> > Channels and sessions are intertwined. Channels signing keys depend on
> > the session it is connected to (See "3.2.5.3.1 Handling a New
> > Authentication" and "3.2.5.3.3 Handling Session Binding").
> > Think carefully on what should be done on disconnect/reconnect.
> > Especially if the channel is shared with multiple sessions.
>
> Yes, absolutely. Shared channels require careful processing when a
> disconnect is used. It's incredibly rude to close a shared connection.

I agree. But we do that today when sessions share a connection.

>
> > Problem with the pool approach is mount options might require
> > different connections so sharing is not so easy. And reconnecting
> > might involve different fallbacks (dfs) for different sessions.
> >
> > You should see the server struct as the "final destination". Once it's
> > picked we know it's going there.
> >
> >> 2.
> >> Today, the interface list for a server hangs off the session struct. Why?
> >> Doesn't it make more sense to hang it off the server struct? With my
> >> recent changes to query the interface list from the server
> >> periodically, each tcon is querying this and keeping the results in
> >> the session struct.
> >> I plan to move this to the server struct too. And avoid having to
> >> query this too many times unnecessarily. Please let me know if you see
> >> a reason not to do this.
>
> This makes sense, but only if the channel selection is sensibly
> filterable. A non-RDMA mount should not be selecting an RDMA
> channel. An encrypted share may want to prefer an adapter which
> can support bus or CPU affinity. Etc.

This is interesting. I should be able to do the RDMA filtering based
on whether the mount specified rdma as an option.
How do we know whether the server adapter can support bus or CPU
affinity? The only two capabilities that the server can advertise are
RSS_CAPABLE or RDMA_CAPABLE.
https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-smb2/fcd862d1-1b85-42df-92b1-e103199f531f

>
> > It's more convenient to have the interface list at the same place as
> > the channel list but it could be moved I suppose.
> > In the abstract model it's in the server apparently.
> >
> >> 4.
> >> I also feel that the way an interface is selected today for
> >> multichannel will not scale.
> >> We keep selecting the fastest server interface, if it supports RSS.
> >> IMO, we should be distributing the requests among the server
> >> interfaces, based on the interface speed adveritsed.
>
> That's part of it, definitely. "Fastest" is hard to measure. Fast
> as in latency? Bandwidth? CPU overhead?

Fast as in advertised speed in the query interface response i.e. bandwidth.

>
> > RSS means the interface can process packets in parallel queues. The
> > problem is we don't know how many queues it has.
>
> Yup. RSS is only a hint. It's not a metric.
>
> > I'm not sure you can find an optimal algorithm for all NIC
> > vendor/driver combinations. Probably you need to do some tests with a
> > bunch of different HW or find someone knowledgeable.
> >  From my small experience now at mellanox/nvidia I have yet to see less
> > than 8 rx/combined queues. You can get/set the number with ethtool
> > -l/-L.
>
> RSS is only meaningful on the machine doing the receiving, in this
> case, the server. The client has no clue how to optimize that. But
> assuming a handful of RSS queues is a good idea. Personally I'd
> avoid a number as large as 8. But over time, cores are increasing,
> so it's a crapshoot.

I could modify my patch to take an interleaved weighted round robin,
so that each new connection is done to the next server interface.
This would ensure that the number of connections to each server
interface would be evenly shared.

>
> > I've set the max channel connection to 16 at the time but I still
> > don't know what large scale high-speed deployment of SMB look like.
> > For what it's worth, in the NVMe-TCP tests I'm doing at the moment and
> > the systems we use to test (fio reads with a 100gbs eth nic with 63 hw
> > queues, 96 cores cpu on the host&target, reading from a null block
> > target), we get diminishing returns around 24 parallel connections. I
> > don't know how transferable this data point is.
>
> 16 seems high, especially on a gigabit interface. At hundreds of Gb,
> well, that's possibly different. I'd avoid making too many assumptions
> (guesses!).
>
> Tom.
>
> > On that topic, for best performance, some possible future project
> > could be to assign steering rules on the client to force each channel
> > packet processing on different cpus and making sure the cpus are the
> > same as the demultiplex thread (avoids context switches and
> > contentions). See
> > https://www.kernel.org/doc/Documentation/networking/scaling.txt
> > (warning, not an easy read lol)
> >
> > Cheers,
> >



-- 
Regards,
Shyam

Re: Connection sharing in SMB multichannel

[Tom Talpey]

On 1/10/2023 4:16 AM, Shyam Prasad N wrote:
> 3.
> I saw that there was a bug in iface_cmp, where we did not do full
> comparison of addresses to match them.
> Fixed it here:
> https://github.com/sprasad-microsoft/smb3-kernel-client/commit/cef2448dc43d1313571e21ce8283bccacf01978e.patch
> 
> @Tom Talpey Was this your concern with iface_cmp?

Took a look at this and I do have some comments.

Regarding the new address comparator cifs_ipaddr_cmp(), why
does it return a three-value result? It seems to prefer
AF_INET over AF_UNSPEC, and AF_INET6 over AF_INET. Won't
this result in selecting the same physical interface more
than once?

Also, it is comparing the entire contents of the sockaddrs,
including padding and scope id's, which have no meaning on
this end of the wire. That will lead to mismatch.


Regarding the interface comparator, which I'll requote here:

+/*
+ * compare two interfaces a and b
+ * return 0 if everything matches.

This is fine, assuming the address comparator is fixed. Matching
everything is the best result.

+ * return 1 if a has higher link speed, or rdma capable, or rss capable

I'm still uneasy about selecting link speed first. If the mount
specifies RDMA, I think RDMA should be the preferred parameter.
The code you propose would select an ordinary interface, if it
were one bps faster.

+ * return -1 otherwise.

Ok on this. :)

+ */
+static int
+iface_cmp(struct cifs_server_iface *a, struct cifs_server_iface *b)
+{
+	int cmp_ret = 0;
+
+	WARN_ON(!a || !b);
+	if (a->speed == b->speed) {
+		if (a->rdma_capable == b->rdma_capable) {
+			if (a->rss_capable == b->rss_capable) {

RSS is meaningless on an RDMA adapter. The RDMA kernel API uses
Queue Pairs to direct completion interrupts, totally different
and independent from RSS. It's only meaningful if rdma_capable
is false.

+				cmp_ret = cifs_ipaddr_cmp((struct sockaddr *) &a->sockaddr,
+						       (struct sockaddr *) &b->sockaddr);
+				if (!cmp_ret)
+					return 0;
+				else if (cmp_ret > 0)
+					return 1;

Again, I don't agree that the address family has anything to do
with preferring an interface. This should return -1.

+				else
+					return -1;
+			} else if (a->rss_capable > b->rss_capable)
+				return 1;

It's good to prefer an RSS-capable interfgace over non-RSS, but
again, only if the interface is not RDMA.

+			else
+				return -1;
+		} else if (a->rdma_capable > b->rdma_capable)
+			return 1;

And it's good to prefer RDMA over non-RDMA, but again, it's going
to have very strange results if the client starts sending traffic
over both interfaces for the same flow!

+		else
+			return -1;
+	} else if (a->speed > b->speed)
+		return 1;
+	else
+		return -1;
+}

And again, speeds are only one kind of tiebreaker. This code
only looks at the RSS and RDMA attributes when the speeds
match. The Windows client, for example, *always* prefers RDMA
if available. But Windows has no explicit RDMA mode, it always
starts with TCP. The Linux client is different, yet this code
doesn't seem to check.

Personally, I think that without an explicit -o rdma, the code
should attempt to connect to any discovered RDMA server
interfaces, via RDMA, and use them exclusively if they work.
Otherwise, it should stay on TCP.

OTOH, if -o rdma was specified, the client should ignore TCP,
or at least, not silently fall back to TCP.

In other words, these tests are still too simplistic, and too
likely to result in unexpected mixes of channels. Should we step
back and write a flowchart?

Tom.

Re: Connection sharing in SMB multichannel

[Shyam Prasad N]

Hi Tom,

As always, thanks for the detailed review. :)

On Wed, Jan 11, 2023 at 10:32 PM Tom Talpey <tom@talpey.com> wrote:
>
> On 1/10/2023 4:16 AM, Shyam Prasad N wrote:
> > 3.
> > I saw that there was a bug in iface_cmp, where we did not do full
> > comparison of addresses to match them.
> > Fixed it here:
> > https://github.com/sprasad-microsoft/smb3-kernel-client/commit/cef2448dc43d1313571e21ce8283bccacf01978e.patch
> >
> > @Tom Talpey Was this your concern with iface_cmp?
>
> Took a look at this and I do have some comments.
>
> Regarding the new address comparator cifs_ipaddr_cmp(), why
> does it return a three-value result? It seems to prefer
> AF_INET over AF_UNSPEC, and AF_INET6 over AF_INET. Won't
> this result in selecting the same physical interface more
> than once?

The idea is to return consistent values for a given pair, so that, for
interfaces with exactly same advertised speeds, they're ordered based
on address family first, then based on what memcmp returns.

>
> Also, it is comparing the entire contents of the sockaddrs,
> including padding and scope id's, which have no meaning on
> this end of the wire. That will lead to mismatch.

My familiarity with IPv6 is relatively low. Steve actually pointed me
to NFS client code which does a similar comparison. Let me redo this
part.

>
>
> Regarding the interface comparator, which I'll requote here:
>
> +/*
> + * compare two interfaces a and b
> + * return 0 if everything matches.
>
> This is fine, assuming the address comparator is fixed. Matching
> everything is the best result.
>
> + * return 1 if a has higher link speed, or rdma capable, or rss capable
>
> I'm still uneasy about selecting link speed first. If the mount
> specifies RDMA, I think RDMA should be the preferred parameter.
> The code you propose would select an ordinary interface, if it
> were one bps faster.

I could do that. But that only changes the order in which the
interfaces are ordered.
I think the place where you want this change more is where the
interface is actually getting selected for a new connection.
I'll do this.

>
> + * return -1 otherwise.
>
> Ok on this. :)
>
> + */
> +static int
> +iface_cmp(struct cifs_server_iface *a, struct cifs_server_iface *b)
> +{
> +       int cmp_ret = 0;
> +
> +       WARN_ON(!a || !b);
> +       if (a->speed == b->speed) {
> +               if (a->rdma_capable == b->rdma_capable) {
> +                       if (a->rss_capable == b->rss_capable) {
>
> RSS is meaningless on an RDMA adapter. The RDMA kernel API uses
> Queue Pairs to direct completion interrupts, totally different
> and independent from RSS. It's only meaningful if rdma_capable
> is false.

Ok. I was not sure about whether they can co-exist, and decided to
make this extra comparison anyway.
Good that you shared this info. But I don't think this code hurts.

>
> +                               cmp_ret = cifs_ipaddr_cmp((struct sockaddr *) &a->sockaddr,
> +                                                      (struct sockaddr *) &b->sockaddr);
> +                               if (!cmp_ret)
> +                                       return 0;
> +                               else if (cmp_ret > 0)
> +                                       return 1;
>
> Again, I don't agree that the address family has anything to do
> with preferring an interface. This should return -1.

As explained above, this just decides the ordering of the interface list.
The items on the head of the list do get slight preference, the list
is first sorted based on speed, then on capabilities, then on family,
then on the actual value.

>
> +                               else
> +                                       return -1;
> +                       } else if (a->rss_capable > b->rss_capable)
> +                               return 1;
>
> It's good to prefer an RSS-capable interfgace over non-RSS, but
> again, only if the interface is not RDMA.
>
> +                       else
> +                               return -1;
> +               } else if (a->rdma_capable > b->rdma_capable)
> +                       return 1;
>
> And it's good to prefer RDMA over non-RDMA, but again, it's going
> to have very strange results if the client starts sending traffic
> over both interfaces for the same flow!

I understand your concern on this. And I can fix this.

>
> +               else
> +                       return -1;
> +       } else if (a->speed > b->speed)
> +               return 1;
> +       else
> +               return -1;
> +}
>
> And again, speeds are only one kind of tiebreaker. This code
> only looks at the RSS and RDMA attributes when the speeds
> match. The Windows client, for example, *always* prefers RDMA
> if available. But Windows has no explicit RDMA mode, it always
> starts with TCP. The Linux client is different, yet this code
> doesn't seem to check.
>
> Personally, I think that without an explicit -o rdma, the code
> should attempt to connect to any discovered RDMA server
> interfaces, via RDMA, and use them exclusively if they work.
> Otherwise, it should stay on TCP.

So even for this case, no mixing?

>
> OTOH, if -o rdma was specified, the client should ignore TCP,
> or at least, not silently fall back to TCP.

Understood.

>
> In other words, these tests are still too simplistic, and too
> likely to result in unexpected mixes of channels. Should we step
> back and write a flowchart?

I think I understood your concerns.
I'll address all these concerns with the next version of my patch and
then check if we're on the same page.

>
> Tom.



-- 
Regards,
Shyam