mbox series

[net-next,0/6] net/ncsi: Allow enabling multiple packages & channels

Message ID 20181018035917.19413-1-sam@mendozajonas.com
Headers show
Series net/ncsi: Allow enabling multiple packages & channels | expand

Message

Sam Mendoza-Jonas Oct. 18, 2018, 3:59 a.m. UTC 
This series extends the NCSI driver to configure multiple packages
and/or channels simultaneously. Since the RFC series this includes a few
extra changes to fix areas in the driver that either made this harder or
were roadblocks due to deviations from the NCSI specification.

Patches 1 & 2 fix two issues where the driver made assumptions about the
capabilities of the NCSI topology.
Patches 3 & 4 change some internal semantics slightly to make multi-mode
easier.
Patch 5 introduces a cleaner way of reconfiguring the NCSI configuration
and keeping track of channel states.
Patch 6 implements the main multi-package/multi-channel configuration,
configured via the Netlink interface.

Readers who have an interesting NCSI setup - especially multi-package
with HWA - please test! I think I've covered all permutations but I
don't have infinite hardware to test on.

Samuel Mendoza-Jonas (6):
  net/ncsi: Don't enable all channels when HWA available
  net/ncsi: Probe single packages to avoid conflict
  net/ncsi: Don't deselect package in suspend if active
  net/ncsi: Don't mark configured channels inactive
  net/ncsi: Reset channel state in ncsi_start_dev()
  net/ncsi: Configure multi-package, multi-channel modes with failover

 include/uapi/linux/ncsi.h |  15 ++
 net/ncsi/internal.h       |  19 +-
 net/ncsi/ncsi-aen.c       |  63 ++++--
 net/ncsi/ncsi-manage.c    | 445 +++++++++++++++++++++++++-------------
 net/ncsi/ncsi-netlink.c   | 229 +++++++++++++++++---
 net/ncsi/ncsi-rsp.c       |   2 +-
 6 files changed, 570 insertions(+), 203 deletions(-)

Comments

David Miller Oct. 18, 2018, 10:56 p.m. UTC | #1 
From: Samuel Mendoza-Jonas <sam@mendozajonas.com>
Date: Thu, 18 Oct 2018 14:59:11 +1100

> This series extends the NCSI driver to configure multiple packages
> and/or channels simultaneously. Since the RFC series this includes a few
> extra changes to fix areas in the driver that either made this harder or
> were roadblocks due to deviations from the NCSI specification.
> 
> Patches 1 & 2 fix two issues where the driver made assumptions about the
> capabilities of the NCSI topology.
> Patches 3 & 4 change some internal semantics slightly to make multi-mode
> easier.
> Patch 5 introduces a cleaner way of reconfiguring the NCSI configuration
> and keeping track of channel states.
> Patch 6 implements the main multi-package/multi-channel configuration,
> configured via the Netlink interface.
> 
> Readers who have an interesting NCSI setup - especially multi-package
> with HWA - please test! I think I've covered all permutations but I
> don't have infinite hardware to test on.

This doesn't apply cleanly to net-next.  Does it depend upon changes
applied elsewhere?  You must always make that explicit.

Also, please explain this locking in ncsi_reset_dev():

+	NCSI_FOR_EACH_PACKAGE(ndp, np) {
+		NCSI_FOR_EACH_CHANNEL(np, nc) {
+			spin_lock_irqsave(&nc->lock, flags);
+			enabled = nc->monitor.enabled;
+			state = nc->state;
+			spin_unlock_irqrestore(&nc->lock, flags);
+
+			if (enabled)
+				ncsi_stop_channel_monitor(nc);
+			if (state == NCSI_CHANNEL_ACTIVE) {
+				active = nc;
+				break;
+			}

Is that really protecting anything?

Right after you drop np->lock those two values can change, the state
of the 'nc' can change such that it isn't NCSI_CHANNEL_ACTIVE anymore
etc.

At best this locking makes sure thatn enabled and state are consistent
with respect to eachother, only.  It doesn't guarantee anything about
the stability of the state of the object at all, and it can change
right from under you.
Sam Mendoza-Jonas Oct. 18, 2018, 11:05 p.m. UTC | #2 
On Thu, 2018-10-18 at 15:56 -0700, David Miller wrote:
> From: Samuel Mendoza-Jonas <sam@mendozajonas.com>
> Date: Thu, 18 Oct 2018 14:59:11 +1100
> 
> > This series extends the NCSI driver to configure multiple packages
> > and/or channels simultaneously. Since the RFC series this includes a few
> > extra changes to fix areas in the driver that either made this harder or
> > were roadblocks due to deviations from the NCSI specification.
> > 
> > Patches 1 & 2 fix two issues where the driver made assumptions about the
> > capabilities of the NCSI topology.
> > Patches 3 & 4 change some internal semantics slightly to make multi-mode
> > easier.
> > Patch 5 introduces a cleaner way of reconfiguring the NCSI configuration
> > and keeping track of channel states.
> > Patch 6 implements the main multi-package/multi-channel configuration,
> > configured via the Netlink interface.
> > 
> > Readers who have an interesting NCSI setup - especially multi-package
> > with HWA - please test! I think I've covered all permutations but I
> > don't have infinite hardware to test on.
> 
> This doesn't apply cleanly to net-next.  Does it depend upon changes
> applied elsewhere?  You must always make that explicit.

Ah, my mistake; I hadn't updated my net-next branch recently enough and
missed Vijay's OEM command patch. Will rebase.

> 
> Also, please explain this locking in ncsi_reset_dev():
> 
> +       NCSI_FOR_EACH_PACKAGE(ndp, np) {
> +               NCSI_FOR_EACH_CHANNEL(np, nc) {
> +                       spin_lock_irqsave(&nc->lock, flags);
> +                       enabled = nc->monitor.enabled;
> +                       state = nc->state;
> +                       spin_unlock_irqrestore(&nc->lock, flags);
> +
> +                       if (enabled)
> +                               ncsi_stop_channel_monitor(nc);
> +                       if (state == NCSI_CHANNEL_ACTIVE) {
> +                               active = nc;
> +                               break;
> +                       }
> 
> Is that really protecting anything?
> 
> Right after you drop np->lock those two values can change, the state
> of the 'nc' can change such that it isn't NCSI_CHANNEL_ACTIVE anymore
> etc.
> 
> At best this locking makes sure thatn enabled and state are consistent
> with respect to eachother, only.  It doesn't guarantee anything about
> the stability of the state of the object at all, and it can change
> right from under you.

And you've caught this correctly, I'll fix this up in the rebase to
protect actually checking the channel/monitor state.

Thanks,
Sam
Justin.Lee1@Dell.com Oct. 19, 2018, 9:38 p.m. UTC | #3 
Hi Sam,

When I run the testing (enable multi-package (package 0 and 1) and 
multi-channel (channel 0 and 1 on both packages)), I see there are two channels
with TX enable.

When I unplug/plug network cable, it seems the behavior is to keep one TX per
package, is it the behavior you expect? All channels are behind the same eth2,
I would expect there is only one active TX.

I create the below file to expose driver's status. RX and TX are printing 
from the follow variables.
nc->modes[NCSI_MODE_ENABLE].enable,
nc->modes[NCSI_MODE_TX_ENABLE].enable,

cat /sys/kernel/debug/ncsi_protocol/ncsi_device_status;
IFIDX IFNAME NAME   PID CID RX TX MP MC WP WC PS RU CR NQ
=========================================================
  2   eth2   ncsi0  000 000 1  1  1  1  1  1  1  1  1  0 
  2   eth2   ncsi1  000 001 1  0  1  1  1  1  1  1  1  0 
  2   eth2   ncsi2  001 000 1  1  1  1  1  1  1  1  1  0 
  2   eth2   ncsi3  001 001 1  0  1  1  1  1  1  1  1  0 
=========================================================
M:  Multi-mode      P:  Package ID
W:  Whitelist       C:  Channel ID
PS: Poll Status
RU: Running
CR: Carrier OK
NQ: Queue Stopped

Thanks,
Justin


From: Samuel Mendoza-Jonas <sam@mendozajonas.com>
Date: Thu, 18 Oct 2018 14:59:11 +1100

> This series extends the NCSI driver to configure multiple packages
> and/or channels simultaneously. Since the RFC series this includes a few
> extra changes to fix areas in the driver that either made this harder or
> were roadblocks due to deviations from the NCSI specification.
> 
> Patches 1 & 2 fix two issues where the driver made assumptions about the
> capabilities of the NCSI topology.
> Patches 3 & 4 change some internal semantics slightly to make multi-mode
> easier.
> Patch 5 introduces a cleaner way of reconfiguring the NCSI configuration
> and keeping track of channel states.
> Patch 6 implements the main multi-package/multi-channel configuration,
> configured via the Netlink interface.
> 
> Readers who have an interesting NCSI setup - especially multi-package
> with HWA - please test! I think I've covered all permutations but I
> don't have infinite hardware to test on.
Sam Mendoza-Jonas Oct. 22, 2018, 10:24 p.m. UTC | #4 
On Fri, 2018-10-19 at 21:38 +0000, Justin.Lee1@Dell.com wrote:
> Hi Sam,
> 
> When I run the testing (enable multi-package (package 0 and 1) and 
> multi-channel (channel 0 and 1 on both packages)), I see there are two channels
> with TX enable.
> 
> When I unplug/plug network cable, it seems the behavior is to keep one TX per
> package, is it the behavior you expect? All channels are behind the same eth2,
> I would expect there is only one active TX.
> 
> I create the below file to expose driver's status. RX and TX are printing 
> from the follow variables.
> nc->modes[NCSI_MODE_ENABLE].enable,
> nc->modes[NCSI_MODE_TX_ENABLE].enable,
> 
> cat /sys/kernel/debug/ncsi_protocol/ncsi_device_status;
> IFIDX IFNAME NAME   PID CID RX TX MP MC WP WC PS RU CR NQ
> =========================================================
>   2   eth2   ncsi0  000 000 1  1  1  1  1  1  1  1  1  0 
>   2   eth2   ncsi1  000 001 1  0  1  1  1  1  1  1  1  0 
>   2   eth2   ncsi2  001 000 1  1  1  1  1  1  1  1  1  0 
>   2   eth2   ncsi3  001 001 1  0  1  1  1  1  1  1  1  0 
> =========================================================
> M:  Multi-mode      P:  Package ID
> W:  Whitelist       C:  Channel ID
> PS: Poll Status
> RU: Running
> CR: Carrier OK
> NQ: Queue Stopped

Hi Justin,

Thanks for testing; this is probably an oversight in
ncsi_channel_is_tx(), where channels from a package other than the
current one could be ignored when checking for the current channel's link
status. I'll update this to be more robust.

Sam

> 
> Thanks,
> Justin
> 
> 
> From: Samuel Mendoza-Jonas <sam@mendozajonas.com>
> Date: Thu, 18 Oct 2018 14:59:11 +1100
> 
> > This series extends the NCSI driver to configure multiple packages
> > and/or channels simultaneously. Since the RFC series this includes a few
> > extra changes to fix areas in the driver that either made this harder or
> > were roadblocks due to deviations from the NCSI specification.
> > 
> > Patches 1 & 2 fix two issues where the driver made assumptions about the
> > capabilities of the NCSI topology.
> > Patches 3 & 4 change some internal semantics slightly to make multi-mode
> > easier.
> > Patch 5 introduces a cleaner way of reconfiguring the NCSI configuration
> > and keeping track of channel states.
> > Patch 6 implements the main multi-package/multi-channel configuration,
> > configured via the Netlink interface.
> > 
> > Readers who have an interesting NCSI setup - especially multi-package
> > with HWA - please test! I think I've covered all permutations but I
> > don't have infinite hardware to test on.
> 
>