Re: [PATCH 0/6 iwl-next] idpf: refactor virtchnl messages

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Alan Brady wrote:
> The motivation for this series has two primary goals. We want to enable
> support of multiple simultaneous messages and make the channel more
> robust. The way it works right now, the driver can only send and receive
> a single message at a time and if something goes really wrong, it can
> lead to data corruption and strange bugs.
> 
> This works by conceptualizing a send and receive as a "virtchnl
> transaction" (idpf_vc_xn) and introducing a "transaction manager"
> (idpf_vc_xn_manager). The vcxn_mngr will init a ring of transactions
> from which the driver will pop from a bitmap of free transactions to
> track in-flight messages. Instead of needing to handle a complicated
> send/recv for every a message, the driver now just needs to fill out a
> xn_params struct and hand it over to idpf_vc_xn_exec which will take
> care of all the messy bits. Once a message is sent and receives a reply,
> we leverage the completion API to signal the received buffer is ready to
> be used (assuming success, or an error code otherwise).
> 
> At a low-level, this implements the "sw cookie" field of the virtchnl
> message descriptor to enable this. We have 16 bits we can put whatever
> we want and the recipient is required to apply the same cookie to the
> reply for that message.  We use the first 8 bits as an index into the
> array of transactions to enable fast lookups and we use the second 8
> bits as a salt to make sure each cookie is unique for that message. As
> transactions are received in arbitrary order, it's possible to reuse a
> transaction index and the salt guards against index conflicts to make
> certain the lookup is correct. As a primitive example, say index 1 is
> used with salt 1. The message times out without receiving a reply so
> index 1 is renewed to be ready for a new transaction, we report the
> timeout, and send the message again. Since index 1 is free to be used
> again now, index 1 is again sent but now salt is 2. This time we do get
> a reply, however it could be that the reply is _actually_ for the
> previous send index 1 with salt 1.  Without the salt we would have no
> way of knowing for sure if it's the correct reply, but with we will know
> for certain.
> 
> Through this conversion we also get several other benefits. We can now
> more appropriately handle asynchronously sent messages by providing
> space for a callback to be defined. This notably allows us to handle MAC
> filter failures better; previously we could potentially have stale,
> failed filters in our list, which shouldn't really have a major impact
> but is obviously not correct. I also managed to remove slightly more
> lines than I added which is a win in my book.
> 
> Alan Brady (6):
>   idpf: implement virtchnl transaction manager
>   idpf: refactor vport virtchnl messages
>   idpf: refactor queue related virtchnl messages
>   idpf: refactor remaining virtchnl messages
>   idpf: refactor idpf_recv_mb_msg
>   idpf: cleanup virtchnl cruft
> 
>  drivers/net/ethernet/intel/idpf/idpf.h        |  192 +-
>  .../ethernet/intel/idpf/idpf_controlq_api.h   |    5 +
>  drivers/net/ethernet/intel/idpf/idpf_lib.c    |   29 +-
>  drivers/net/ethernet/intel/idpf/idpf_main.c   |    3 +-
>  drivers/net/ethernet/intel/idpf/idpf_vf_dev.c |    2 +-
>  .../net/ethernet/intel/idpf/idpf_virtchnl.c   | 1984 ++++++++---------
>  6 files changed, 1045 insertions(+), 1170 deletions(-)

Great improvement, +1.

This makes virtchan more robust during edge case conditions, more
scalable and the code cleaner: less open coded duplication across
every vc operation.

The code mostly matches what I am familiar with and we have extensive
test experience with. From an initial side-by-side comparison.

I'll need to read the code that differs more closely (such as the
xn_bm_lock that Simon commented on) and will run a sanity test. Even
just a successful boot will have exercised much of this code.

One comment for patch [4/6] only.