mbox series

[v2,0/7,iwl-next] idpf: refactor virtchnl messages

Message ID 20240126054747.960172-1-alan.brady@intel.com (mailing list archive)
Headers show
Series idpf: refactor virtchnl messages | expand

Message

Alan Brady Jan. 26, 2024, 5:47 a.m. UTC
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 (7):
  idpf: implement virtchnl transaction manager
  idpf: refactor vport virtchnl messages
  idpf: refactor queue related virtchnl messages
  idpf: refactor remaining virtchnl messages
  idpf: add async_handler for MAC filter 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   | 1982 ++++++++---------
 6 files changed, 1043 insertions(+), 1170 deletions(-)

Comments

Alexander Lobakin Jan. 29, 2024, 1:23 p.m. UTC | #1
From: Alan Brady <alan.brady@intel.com>
Date: Thu, 25 Jan 2024 21:47:40 -0800

> 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.

[...]

There are a fistful of functions in this series and IDPF's virtchnl code
in general that allocate a memory chunk via kzalloc() family and then
free it at the end of the function, i.e. the lifetime of those buffers
are the lifetime of the function.
Since recently, we have auto-variables in the kernel, so that the pieces
I described could be converted to:

	struct x *ptr __free(kfree) = NULL;

	ptr = kzalloc(sizeof(*x), GPF_KERNEL);

	// some code

	return 0; // kfree() is not needed anymore

err:
	return err; // here as well

That would allow to simplify the code and reduce its size.
I'd like you to convert such functions to use auto-variables.

Thanks,
Olek
Alan Brady Jan. 29, 2024, 3:12 p.m. UTC | #2
> -----Original Message-----
> From: Lobakin, Aleksander <aleksander.lobakin@intel.com>
> Sent: Monday, January 29, 2024 5:24 AM
> To: Brady, Alan <alan.brady@intel.com>
> Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org;
> willemdebruijn.kernel@gmail.com; Bagnucki, Igor
> <igor.bagnucki@intel.com>; Kitszel, Przemyslaw
> <przemyslaw.kitszel@intel.com>
> Subject: Re: [Intel-wired-lan] [PATCH v2 0/7 iwl-next] idpf: refactor virtchnl
> messages
> 
> From: Alan Brady <alan.brady@intel.com>
> Date: Thu, 25 Jan 2024 21:47:40 -0800
> 
> > 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.
> 
> [...]
> 
> There are a fistful of functions in this series and IDPF's virtchnl code in general
> that allocate a memory chunk via kzalloc() family and then free it at the end of
> the function, i.e. the lifetime of those buffers are the lifetime of the function.
> Since recently, we have auto-variables in the kernel, so that the pieces I
> described could be converted to:
> 
> 	struct x *ptr __free(kfree) = NULL;
> 
> 	ptr = kzalloc(sizeof(*x), GPF_KERNEL);
> 
> 	// some code
> 
> 	return 0; // kfree() is not needed anymore
> 
> err:
> 	return err; // here as well
> 
> That would allow to simplify the code and reduce its size.
> I'd like you to convert such functions to use auto-variables.

Certainly, should be straightforward and make the code much better, sounds good to me. Just to clarify I'm only going to mess with the virtchnl functions I've otherwise altered in this patch series to maintain appropriate scope, yes?

-Alan

> 
> Thanks,
> Olek
Alexander Lobakin Jan. 29, 2024, 3:43 p.m. UTC | #3
From: Brady, Alan <alan.brady@intel.com>
Date: Mon, 29 Jan 2024 16:12:06 +0100

>> -----Original Message-----
>> From: Lobakin, Aleksander <aleksander.lobakin@intel.com>
>> Sent: Monday, January 29, 2024 5:24 AM
>> To: Brady, Alan <alan.brady@intel.com>
>> Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org;
>> willemdebruijn.kernel@gmail.com; Bagnucki, Igor
>> <igor.bagnucki@intel.com>; Kitszel, Przemyslaw
>> <przemyslaw.kitszel@intel.com>
>> Subject: Re: [Intel-wired-lan] [PATCH v2 0/7 iwl-next] idpf: refactor virtchnl
>> messages
>>
>> From: Alan Brady <alan.brady@intel.com>
>> Date: Thu, 25 Jan 2024 21:47:40 -0800
>>
>>> 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.
>>
>> [...]
>>
>> There are a fistful of functions in this series and IDPF's virtchnl code in general
>> that allocate a memory chunk via kzalloc() family and then free it at the end of
>> the function, i.e. the lifetime of those buffers are the lifetime of the function.
>> Since recently, we have auto-variables in the kernel, so that the pieces I
>> described could be converted to:
>>
>> 	struct x *ptr __free(kfree) = NULL;
>>
>> 	ptr = kzalloc(sizeof(*x), GPF_KERNEL);
>>
>> 	// some code
>>
>> 	return 0; // kfree() is not needed anymore
>>
>> err:
>> 	return err; // here as well
>>
>> That would allow to simplify the code and reduce its size.
>> I'd like you to convert such functions to use auto-variables.
> 
> Certainly, should be straightforward and make the code much better, sounds good to me. Just to clarify I'm only going to mess with the virtchnl functions I've otherwise altered in this patch series to maintain appropriate scope, yes?

Yes, only virtchnl functions. New functions that you introduce 100%, the
rest only if you touch them.

> 
> -Alan
> 
>>
>> Thanks,
>> Olek

Thanks,
Olek
Alan Brady Jan. 31, 2024, 5:52 p.m. UTC | #4
On 1/29/2024 7:43 AM, Alexander Lobakin wrote:
> From: Brady, Alan <alan.brady@intel.com>
> Date: Mon, 29 Jan 2024 16:12:06 +0100
>
>>> -----Original Message-----
>>> From: Lobakin, Aleksander <aleksander.lobakin@intel.com>
>>> Sent: Monday, January 29, 2024 5:24 AM
>>> To: Brady, Alan <alan.brady@intel.com>
>>> Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org;
>>> willemdebruijn.kernel@gmail.com; Bagnucki, Igor
>>> <igor.bagnucki@intel.com>; Kitszel, Przemyslaw
>>> <przemyslaw.kitszel@intel.com>
>>> Subject: Re: [Intel-wired-lan] [PATCH v2 0/7 iwl-next] idpf: refactor virtchnl
>>> messages
>>>
>>> From: Alan Brady <alan.brady@intel.com>
>>> Date: Thu, 25 Jan 2024 21:47:40 -0800
>>>
>>>> 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.
>>> [...]
>>>
>>> There are a fistful of functions in this series and IDPF's virtchnl code in general
>>> that allocate a memory chunk via kzalloc() family and then free it at the end of
>>> the function, i.e. the lifetime of those buffers are the lifetime of the function.
>>> Since recently, we have auto-variables in the kernel, so that the pieces I
>>> described could be converted to:
>>>
>>> 	struct x *ptr __free(kfree) = NULL;
>>>
>>> 	ptr = kzalloc(sizeof(*x), GPF_KERNEL);
>>>
>>> 	// some code
>>>
>>> 	return 0; // kfree() is not needed anymore
>>>
>>> err:
>>> 	return err; // here as well
>>>
>>> That would allow to simplify the code and reduce its size.
>>> I'd like you to convert such functions to use auto-variables.
>> Certainly, should be straightforward and make the code much better, sounds good to me. Just to clarify I'm only going to mess with the virtchnl functions I've otherwise altered in this patch series to maintain appropriate scope, yes?
> Yes, only virtchnl functions. New functions that you introduce 100%, the
> rest only if you touch them.


I've gone ahead and submitted a v3 with the suggested changes. However, 
I would also like to follow this up with another series that addresses 
the entire idpf driver where we can not only use the automatic variables 
for kfree, but also make the various locks we have much better. I do end 
up touching some of the locks I introduced here, but I need to add a 
guard to include/linux/spinlock.h to convert all of it. I think it still 
makes sense to not include those changes in this series to avoid 
polluting it if possible, but there will be a dependency. Also thank you 
for pointing this out, I think it's a massive improvement to the driver 
code.