Re: [PATCH v2 00/13] introduce fail-safe PMD

[Neil Horman <nhorman@tuxdriver.com>]

On Fri, Mar 10, 2017 at 10:13:32AM +0100, Gaëtan Rivet wrote:
> On Thu, Mar 09, 2017 at 09:15:14AM +0000, Bruce Richardson wrote:
> > On Wed, Mar 08, 2017 at 11:54:02AM -0500, Neil Horman wrote:
> > > On Wed, Mar 08, 2017 at 04:15:33PM +0100, Gaetan Rivet wrote:
> > > > This PMD intercepts and manages Ethernet device removal events issued by
> > > > slave PMDs and re-initializes them transparently when brought back so that
> > > > existing applications do not need to be modified to benefit from true
> > > > hot-plugging support.
> > > >
> > > > The stacked PMD approach shares many similarities with the bonding PMD but
> > > > with a different purpose. While bonding provides the ability to group
> > > > several links into a single logical device for enhanced throughput and
> > > > supports fail-over at link level, this one manages the sudden disappearance
> > > > of the underlying device; it guarantees applications face a valid device in
> > > > working order at all times.
> > > >
> > > Why not just add this feature to the bonding pmd then?  A bond is perfectly
> > > capable of handling the trivial case of a single underlying device, and adding
> > > an option to make the underly slave 'persistent' seem both much simpler in terms
> > > of implementation and code size, than adding an entire new pmd, along with its
> > > supporting code.
> > > 
> > > Neil
> > > 
> 
> @Neil
> I don't know if you saw my answer to Bruce on the matter [1], it
> partially adresses your point.
> 
I did, and I think it asserts your points, but doesn't really address mine.  See
below. 

> > +1
> > I don't like the idea of having multiple PMDs in DPDK to handle
> > combining multiple other devices into one.
> > 
> > /Bruce
> 
> I understand the concern. Let's first put aside for the moment the link
> grouping, which is only part of the fail-safe PMD function.
> 
> The fail-safe PMD at its core, provides an alternative paradigm, a new
> proposal for a hot-plug functionality in a lightweight form-factor from a
> user standpoint.
> 
Ok, but lets be clear here, you're duplicating alot of functionality.  And for
that privlidge, there will be an additional 4000 lines of code to maintain.

> The central question that I would like to tackle is this: why should we
> require from our users declaring a bonding device to have hot-plug support?
> 
We'll, strictly speaking, I suppose we don't have to require it.  But by that
same token, we don't need to do it in a separate PMD either, there are lots of
other options.

> I took some time to illustrate a few modes of operation:
> 
> Fig. 1
> 
>    .-------------.
>    | application |
>    `------.------'
>           |
>      .----'-----.---------. <------ init, conf, Rx/Tx
>      |          |         |
>      |      .---|--.------|--. <--- conf, link check, Rx/Tx
>      |      |   |  |      |  |
>      v      |   v  v      v  v
> .---------. | .-------. .------.
> | bonding | | | ixgbe | | mlx4 |
> `----.----' | `-------' `------'
>      |      |
>      `------'
> 
> Typical link fail-over.
> 
> 
> Fig. 2
> 
>  .-------------.
>  | application |
>  `------.------'
>         | <-------- init, conf, Rx/Tx
>         v
>   .-----------.
>   | fail-safe |
>   `-----.-----'
>         |
>     .---'----. <--- init, conf, dev check, Rx/Tx
>     |        |
>     v        v
> .-------. .------.
> | ixgbe | | mlx4 |
> `-------' `------'
> 
> Typical automatic hot-plug handling with device fail-over.
> 
> 
> Fig. 3
> 
>    .-------------.
>    | application |
>    `------.------'
>           |
>      .----'-----.-------------. <---------- init, conf, Rx/Tx
>      |          |             |
>      |      .---|--.----------|--. <------- conf, link check, Rx/Tx
>      |      |   |  |          |  |
>      v      |   v  v          v  v
> .---------. | .-----------. .-----------.
> | bonding | | | fail-safe | | fail-safe |
> `----.----' | `-----.-----' `-----.-----'
>      |      |       |             | <------ init, conf, dev check, Rx/Tx
>      `------'       v             v
>                 .-------.      .------.
>                 | ixgbe |      | mlx4 |
>                 `-------'      `------'
> 
> Combination to provide link fail-over with automatic hot-plug handling.
> 
> 
> Fig. 4
> 
>    .-------------.
>    | application |
>    `------.------'
>           |
>      .----'-----.-------------. <---------- init, conf, Rx/Tx
>      |          |             |
>      |      .---|--.----------|--. <------- conf, link check, Rx/Tx
>      |      |   |  |          |  |
>      v      |   v  v          v  v
> .---------. | .-----------. .-----------.
> | bonding | | | fail-safe | | fail-safe |
> `----.----' | `-----.-----' `-----.-----'
>      |      |       |             | <------- init, conf, dev check, Rx/Tx
>      `------'       |             |
>              .------'---.     .---'------.
>              |          |     |          |
>              v          v     v          v
>       .--------. .--------. .--------. .--------.
>       | mlx4 1 | | mlx4 2 | | mlx4 1 | | mlx4 2 |
>       | port 1 | | port 1 | | port 2 | | port 2 |
>       `--------' `--------' `--------' `--------'
> 
> Complex use case with link fail-over at port level and automatic hot-plug
> handling with device fail-over.

Yes, I think we all understand the purpose of your PMD - its there to provide a
placeholder device that can respond to application requests in a sane manner,
until such time as real hardware is put in its place via a hot plug/failure.  Thats all
well and good, I'm saying there are better ways to go about this that can
provide the same functionality without having to add an extra 4k lines of code
to the project, many of which already exist.

> 1. LSC vs. RMV
> 
>  A link status change is a valid state for a device. It calls for
>  specific responses, e.g. a link switch in a bonding device, without
>  losing the general configuration of the port.
> 
>  The removal of a device calls for more than pausing operations and
>  switching an active device. The party responsible for initializing the
>  device should take care of closing it properly. If this party also
>  wants to be able to restore the device if it was plugged back in, it
>  would need be able to initialize it back and reconfigure its previous
>  state.
> 
>  As we can see that in [Fig. 1], this responsibility lies upon the
>  application.
> 
Again, yes, I think we all see the benefit of centralizing hot plug operations,
no one is disagreing with that, its the code/functional duplication that is
concerning.

> 2. Bonding and link availability
> 
>  The hot-plug functionality is not a core function of the bonding PMD.
>  It is only interested in knowing if the link is active or not.
> 
Currently, yes.  The suggestion was that you augment the bonding driver so that
hot plug is a core function of bonding.

>  Adding the device persistence to the bonding PMD would mean adding the
>  ability to flexibly parse device definitions to cope with plug-ins in
>  evolving busses (PCI hot-plug could mean changing bus addresses), being
>  able to emulate the EAL and the ether layer and to properly store the
>  device configuration.  This means formally describing the life of a
>  device in a DPDK application from start to finish.
> 
Which seems to me to be exactly what your PMD does.  I don't see why its
fundamentally harder to do that in an existing pmd, than it is in a new one.

>  All of this hot-plug support, in order for the bonding to be aware of
>  the status of some of its links. This seems like scope-creep.
> 
But one of your primary use cases is bonding.  Its the nature of code to add
features as time goes on.

> 3. Fail-safe and hot-plug support
> 
>  An attach / detach (pseudo-hotplug) API exists in DPDK. The main  problem
> of this API is that it does not offer reacting to a device  plug-out, only
> triggering a device detaching from an application. This  is a completely
> different approach from an application standpoint.
> 
>  The fail-safe PMD offers an out-of-the-box implementation of a newly
>  proposed hot-plug API [2]. It allows a seamless integration to users
>  for device removal support in their applications, without requiring
>  evolutions [Fig. 2]. This flexibility allows it to be used as part of
>  a bond [Fig. 3], [Fig. 4], while current bonding does not allow for
>  detaching devices, even if it were to be considered hot-plug. There is
>  no reason however to require declaring a bonding device to be able to
>  use it, from a user perspective this seems backward.
> 
>  Both bonding and fail-safe PMDs deal with enslaving devices and
>  acting upon their state changing. The bonding function performs at
>  link level, while the hot-plug function deals with the device itself.
>  I do not think this similarity justify merging both functions.
>  Maintenance would be easier with clear, simpler functions implemented
>  in separate PMDs.
> 

My suggestion to you would be to re-architect this in the following way:

1) Augment the null pmd to accept arbitrary parameters.  Paremeters which are
not explicitly recognized are stored.  Create an api call via the ethdev api to
retrieve said arguments.

2) Augment the bonding driver so that, upon configuration, null drivers can be
added to the bond, and marked as replaceable in some fashion

3) Port your hotplug implementation into the bonding code (or make it a separate
library, not sure which yet).  On a hot plug event trigger, react by querying
the replaceable interfaces to see if the hot plugged nic is meant to replace an
existing one.  If a match is found, detach the null pmd instance, and add the
hot plugged interface.

The intent here is to provide you with the functionality that you have (which is
a good feature), without having to support an entire new pmd that replicates a
great deal of existing code an functionality.

Regards
Neil

> [1]: http://dpdk.org/ml/archives/dev/2017-March/059446.html
> [2]: http://dpdk.org/ml/archives/dev/2017-March/059217.html
> 
> -- 
> Gaëtan Rivet
> 6WIND
>