On Mon, Jan 22, 2024 at 01:11:20PM -0800, Alan Brady wrote:
> This starts refactoring how virtchnl messages are handled by adding a
> transaction manager (idpf_vc_xn_manager).
>
> There are two primary motivations here which are to enable handling of
> multiple messages at once and to make it more robust in general. As it
> is right now, the driver may only have one pending message at a time and
> there's no guarantee that the response we receive was actually intended
> for the message we sent prior.
>
> This works by utilizing a "cookie" field of the message descriptor. It
> is arbitrary what data we put in the cookie and the response is required
> to have the same cookie the original message was sent with. Then using a
> "transaction" abstraction that uses the completion API to pair responses
> to the message it belongs to.
>
> The cookie works such that the first half is the index to the
> transaction in our array, and the second half is a "salt" that gets
> incremented every message. This enables quick lookups into the array and
> also ensuring we have the correct message. The salt is necessary because
> after, for example, a message times out and we deem the response was
> lost for some reason, we could theoretically reuse the same index but
> using a different salt ensures that when we do actually get a response
> it's not the old message that timed out previously finally coming in.
> Since the number of transactions allocated is U8_MAX and the salt is 8
> bits, we can never have a conflict because we can't roll over the salt
> without using more transactions than we have available.
>
> This starts by only converting the VIRTCHNL2_OP_VERSION message to use
> this new transaction API. Follow up patches will convert all virtchnl
> messages to use the API.
>
> Reviewed-by: Igor Bagnucki <[email protected]>
> Reviewed-by: Przemek Kitszel <[email protected]>
> Signed-off-by: Alan Brady <[email protected]>
> Co-developed-by: Joshua Hay <[email protected]>
> Signed-off-by: Joshua Hay <[email protected]>
...
> +/**
> + * idpf_vc_xn_init - Initialize virtchnl transaction object
> + * @vcxn_mngr: pointer to vc transaction manager struct
> + */
> +static void idpf_vc_xn_init(struct idpf_vc_xn_manager *vcxn_mngr)
> +{
> + int i;
> +
> + spin_lock_init(&vcxn_mngr->xn_bm_lock);
> +
> + for (i = 0; i < ARRAY_SIZE(vcxn_mngr->ring); i++) {
> + struct idpf_vc_xn *xn = &vcxn_mngr->ring[i];
> +
> + xn->state = IDPF_VC_XN_IDLE;
> + xn->idx = i;
> + idpf_vc_xn_release_bufs(xn);
> + init_completion(&xn->completed);
> + }
Hi Alan and Joshua,
I'm slightly surprised to see that
it is safe to initialise xn_bm_lock above,
but it needs to be taken below.
> +
> + spin_lock_bh(&vcxn_mngr->xn_bm_lock);
> + bitmap_set(vcxn_mngr->free_xn_bm, 0, IDPF_VC_XN_RING_LEN);
> + spin_unlock_bh(&vcxn_mngr->xn_bm_lock);
> +}
