[v4,00/10,iwl-next] idpf: refactor virtchnl messages

Message

Alan Brady Feb. 6, 2024, 3:37 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 fairly
significant more lines than I added which is a win in my book.

Additionally, this converts some variables to use auto-variables where
appropriate. This makes the alloc paths much cleaner and less prone to
memory leaks. We also fix a few virtchnl related bugs while we're here.

Alan Brady (10):
  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
  idpf: prevent deinit uninitialized virtchnl core
  idpf: fix minor controlq issues
  idpf: remove dealloc vector msg err in idpf_intr_rel

 drivers/net/ethernet/intel/idpf/idpf.h        |  194 +-
 .../net/ethernet/intel/idpf/idpf_controlq.c   |    7 +-
 .../ethernet/intel/idpf/idpf_controlq_api.h   |    5 +
 drivers/net/ethernet/intel/idpf/idpf_lib.c    |   38 +-
 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   | 2175 ++++++++---------
 7 files changed, 1096 insertions(+), 1328 deletions(-)

Comments

Alexander Lobakin Feb. 6, 2024, 5:02 p.m. UTC | #1

From: Alan Brady <[e-mail]>
Date: Mon, 5 Feb 2024 19:37:54 -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.

This works better than v3.
For the basic scenarios:

Tested-by: Alexander Lobakin <[e-mail]>

Thanks,
Olek

Jakub Kicinski Feb. 6, 2024, 6:57 p.m. UTC | #2

On Mon,  5 Feb 2024 19:37:54 -0800 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.

Coccinelle points out some potential places to use min()

testing/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c:1956:17-18: WARNING opportunity for min()
testing/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c:1271:17-18: WARNING opportunity for min()
testing/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c:1319:17-18: WARNING opportunity for min()
testing/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c:2640:17-18: WARNING opportunity for min()
testing/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c:1295:17-18: WARNING opportunity for min()
testing/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c:2157:17-18: WARNING opportunity for min()
testing/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c:3582:17-18: WARNING opportunity for min()

Alan Brady Feb. 6, 2024, 7:18 p.m. UTC | #3

On 2/6/2024 10:57 AM, Jakub Kicinski wrote:
> On Mon,  5 Feb 2024 19:37:54 -0800 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.
> 
> Coccinelle points out some potential places to use min()
> 
> testing/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c:1956:17-18: WARNING opportunity for min()
> testing/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c:1271:17-18: WARNING opportunity for min()
> testing/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c:1319:17-18: WARNING opportunity for min()
> testing/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c:2640:17-18: WARNING opportunity for min()
> testing/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c:1295:17-18: WARNING opportunity for min()
> testing/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c:2157:17-18: WARNING opportunity for min()
> testing/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c:3582:17-18: WARNING opportunity for min()

We did run coccinelle check and see the min suggestions. It's triggering 
on these statements I added:

return reply_sz < 0 ? reply_sz : 0;

A min here would change it to:

return min(reply_sz, 0);

I didn't really like that because it's misleading as though we're 
returning the size of the reply and might accidentally encourage someone 
to change it to a max. Here reply_sz will be negative if an error was 
returned from message sending. But this function we only want to return 
0 or negative. By being explicit in what we want to do, it seems clearer 
to me what the intention is but I could be wrong.

We can definitely change it however if that's preferred here.

Jakub Kicinski Feb. 6, 2024, 8:03 p.m. UTC | #4

On Tue, 6 Feb 2024 11:18:48 -0800 Alan Brady wrote:
> We did run coccinelle check and see the min suggestions. It's triggering 
> on these statements I added:
> 
> return reply_sz < 0 ? reply_sz : 0;
> 
> A min here would change it to:
> 
> return min(reply_sz, 0);
> 
> I didn't really like that because it's misleading as though we're 
> returning the size of the reply and might accidentally encourage someone 
> to change it to a max. Here reply_sz will be negative if an error was 
> returned from message sending. But this function we only want to return 
> 0 or negative. By being explicit in what we want to do, it seems clearer 
> to me what the intention is but I could be wrong.
> 
> We can definitely change it however if that's preferred here.

Hm, okay, that does sound like making it worse.
I'll disable the minmax coccicheck for now, it seems noisy.

Jacob Keller Feb. 6, 2024, 10:50 p.m. UTC | #5

> -----Original Message-----
> From: Jakub Kicinski <[e-mail]>
> Sent: Tuesday, February 6, 2024 12:03 PM
> To: Brady, Alan <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; Kitszel, Przemyslaw
> <[e-mail]>; Bagnucki, Igor <[e-mail]>;
> Lobakin, Aleksander <[e-mail]>
> Subject: Re: [PATCH v4 00/10 iwl-next] idpf: refactor virtchnl messages
> 
> On Tue, 6 Feb 2024 11:18:48 -0800 Alan Brady wrote:
> > We did run coccinelle check and see the min suggestions. It's triggering
> > on these statements I added:
> >
> > return reply_sz < 0 ? reply_sz : 0;
> >
> > A min here would change it to:
> >
> > return min(reply_sz, 0);
> >
> > I didn't really like that because it's misleading as though we're
> > returning the size of the reply and might accidentally encourage someone
> > to change it to a max. Here reply_sz will be negative if an error was
> > returned from message sending. But this function we only want to return
> > 0 or negative. By being explicit in what we want to do, it seems clearer
> > to me what the intention is but I could be wrong.
> >
> > We can definitely change it however if that's preferred here.
> 
> Hm, okay, that does sound like making it worse.
> I'll disable the minmax coccicheck for now, it seems noisy.

Maybe you could make the coccicheck only complain if the value is non-zero or not const? Maybe that's a bit too complicated... Hm

Jakub Kicinski Feb. 6, 2024, 11:17 p.m. UTC | #6

On Tue, 6 Feb 2024 22:50:25 +0000 Keller, Jacob E wrote:
> > Hm, okay, that does sound like making it worse.
> > I'll disable the minmax coccicheck for now, it seems noisy.  
> 
> Maybe you could make the coccicheck only complain if the value is
> non-zero or not const? Maybe that's a bit too complicated... Hm

Non-zero could work. It may be worthwhile to look at the warnings cocci
currently generates and figure out what's common among the cases where
warning doesn't make sense.

Alexander Lobakin Feb. 14, 2024, 2:49 p.m. UTC | #7

From: Alexander Lobakin <[e-mail]>
Date: Tue, 6 Feb 2024 18:02:33 +0100

> From: Alan Brady <[e-mail]>
> Date: Mon, 5 Feb 2024 19:37:54 -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.
> 
> This works better than v3.
> For the basic scenarios:
> 
> Tested-by: Alexander Lobakin <[e-mail]>

Sorry I'm reverting my tag.
After the series, the CP segfaults on each rmmod idpf:

root@mev-imc:/usr/bin/cplane# cp_pim_mdd_handler: MDD interrupt detected
cp_pim_mdd_event_handler: reg = 40
Jan  1 00:27:57 mev-imc local0.warn LanCP[190]: cp_pim_mdd_handler: MDD
interrupt detected
cp_pim_mdd_event_handler: reg = 1
Jan  1 00:28:54 mev-imc local0.warn LanCP[190]: [hma_create_vport/4257]
WARNING: RSS is configured on 1st contiguous num of queuJan  1 00:28:54
mev-imc local0.warn LanCP[190]: [hma_create_vport/4257] WARNING: RSS is
configured on 1st contiguous num of queuJan  1 00:28:55 mev-imc
local0.warn LanCP[190]: [hma_create_vport/4257] WARNING: RSS is
configured on 1st contiguous num of queues= 16 start Qid= 34
Jan  1 00:28:55 mev-imc local0.warn LanCP[190]: [hma_create_vport/4257]
WARNING: RSS is configured on 1st contiguous num of queu16 start Qid= 640
Jan  1 00:28:55 mev-imc local0.err LanCP[190]:
[cp_del_node_rxbuff_lst/4179] ERR: Resource list is empty, so nothing to
delete here
Jan  1 00:29:08 mev-imc local0.err LanCP[190]:
[cp_uninit_vsi_tc_q_region/222] ERR: Failed to init vsi LUT on vsi 1.
Jan  1 00::08 mev-imc local0.err LanCP[190]:
[cp_uninit_vsi_fxp_config/1101] ERR: cp_uninit_vsi_rss_config() failed
on vsi (1).
Jan  1 00:29:08 mev-imc local0.err LanCP[190]:
[cp_uninit_vsi_tc_q_region/222] ERR: Failed to init vsi LUT on vsi 6.
Jan  1 00:29:08 mev-imc local0.err LanCP[190]:
[cp_uninit_vsi_rss_config/340] ERR: faininit_vsi_rss_config() failed on
vsi (6).
Jan  1 00:29:08 mev-imc local0.err LanCP[190]:
[cp_uninit_vsi_tc_q_region/222] ERR: Failed to init vsi LUT on vsi 7.
Jan  1 00:29:08 mev-imc local0.err LanCP[190]:
[cp_uninit_vsi_rss_config/340] ERR: failed to remove vsi (7)'s queue
regions.
Jan  1 00:29:08 mev-imc local0.err LanCP[190]:
[cp_uninit_vsi_fxp_config/1101] ERR: cp_uninit_vo init vsi LUT on vsi 8.
Jan  1 00:29:08 mev-imc local0.err LanCP[190]:
[cp_uninit_vsi_rss_config/340] ERR: failed to remove vsi (8)'s queue
regions.
Jan  1 00:29:08 mev-imc local0.err LanCP[190]:
[cp_uninit_vsi_fxp_config/1101] ERR: cp_uninit_vsi_rss_config() failed
on vsi (8).
Jan  1 00:29:08 mev-imc local0.err LanCP[190]:
[cp_uninit_vsi_tc_q_region/222] ERR: Failed to init vsi LUT on vsi 1.
Jan  1 00::08 mev-imc local0.err LanCP[190]:
[cp_uninit_vsi_fxp_config/1101] ERR: cp_uninit_vsi_rss_config() failed
on vsi (1).

[1]+  Segmentation fault      ./imccp 0000:00:01.6 0 cp_init.cfg

Only restarting the CP helps -- restarting the imccp daemon makes it
immediately crash again.

This should be dropped from the next-queue until it's fixed.

Thanks,
Olek

Alan Brady Feb. 14, 2024, 5:06 p.m. UTC | #8

On 2/14/2024 6:49 AM, Alexander Lobakin wrote:
> From: Alexander Lobakin <[e-mail]>
> Date: Tue, 6 Feb 2024 18:02:33 +0100
> 
>> From: Alan Brady <[e-mail]>
>> Date: Mon, 5 Feb 2024 19:37:54 -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.
>>
>> This works better than v3.
>> For the basic scenarios:
>>
>> Tested-by: Alexander Lobakin <[e-mail]>
> 
> Sorry I'm reverting my tag.
> After the series, the CP segfaults on each rmmod idpf:
> 
> root@mev-imc:/usr/bin/cplane# cp_pim_mdd_handler: MDD interrupt detected
> cp_pim_mdd_event_handler: reg = 40
> Jan  1 00:27:57 mev-imc local0.warn LanCP[190]: cp_pim_mdd_handler: MDD
> interrupt detected
> cp_pim_mdd_event_handler: reg = 1
> Jan  1 00:28:54 mev-imc local0.warn LanCP[190]: [hma_create_vport/4257]
> WARNING: RSS is configured on 1st contiguous num of queuJan  1 00:28:54
> mev-imc local0.warn LanCP[190]: [hma_create_vport/4257] WARNING: RSS is
> configured on 1st contiguous num of queuJan  1 00:28:55 mev-imc
> local0.warn LanCP[190]: [hma_create_vport/4257] WARNING: RSS is
> configured on 1st contiguous num of queues= 16 start Qid= 34
> Jan  1 00:28:55 mev-imc local0.warn LanCP[190]: [hma_create_vport/4257]
> WARNING: RSS is configured on 1st contiguous num of queu16 start Qid= 640
> Jan  1 00:28:55 mev-imc local0.err LanCP[190]:
> [cp_del_node_rxbuff_lst/4179] ERR: Resource list is empty, so nothing to
> delete here
> Jan  1 00:29:08 mev-imc local0.err LanCP[190]:
> [cp_uninit_vsi_tc_q_region/222] ERR: Failed to init vsi LUT on vsi 1.
> Jan  1 00::08 mev-imc local0.err LanCP[190]:
> [cp_uninit_vsi_fxp_config/1101] ERR: cp_uninit_vsi_rss_config() failed
> on vsi (1).
> Jan  1 00:29:08 mev-imc local0.err LanCP[190]:
> [cp_uninit_vsi_tc_q_region/222] ERR: Failed to init vsi LUT on vsi 6.
> Jan  1 00:29:08 mev-imc local0.err LanCP[190]:
> [cp_uninit_vsi_rss_config/340] ERR: faininit_vsi_rss_config() failed on
> vsi (6).
> Jan  1 00:29:08 mev-imc local0.err LanCP[190]:
> [cp_uninit_vsi_tc_q_region/222] ERR: Failed to init vsi LUT on vsi 7.
> Jan  1 00:29:08 mev-imc local0.err LanCP[190]:
> [cp_uninit_vsi_rss_config/340] ERR: failed to remove vsi (7)'s queue
> regions.
> Jan  1 00:29:08 mev-imc local0.err LanCP[190]:
> [cp_uninit_vsi_fxp_config/1101] ERR: cp_uninit_vo init vsi LUT on vsi 8.
> Jan  1 00:29:08 mev-imc local0.err LanCP[190]:
> [cp_uninit_vsi_rss_config/340] ERR: failed to remove vsi (8)'s queue
> regions.
> Jan  1 00:29:08 mev-imc local0.err LanCP[190]:
> [cp_uninit_vsi_fxp_config/1101] ERR: cp_uninit_vsi_rss_config() failed
> on vsi (8).
> Jan  1 00:29:08 mev-imc local0.err LanCP[190]:
> [cp_uninit_vsi_tc_q_region/222] ERR: Failed to init vsi LUT on vsi 1.
> Jan  1 00::08 mev-imc local0.err LanCP[190]:
> [cp_uninit_vsi_fxp_config/1101] ERR: cp_uninit_vsi_rss_config() failed
> on vsi (1).
> 
> [1]+  Segmentation fault      ./imccp 0000:00:01.6 0 cp_init.cfg
> 
> Only restarting the CP helps -- restarting the imccp daemon makes it
> immediately crash again.
> 
> This should be dropped from the next-queue until it's fixed.
> 
> Thanks,
> Olek


I definitely tested rmmod so I'm frankly not understanding how this can 
be possible. If you would like I'm happy to sync on how you're able to 
cause this to happen. Our internal validation verified 1000 load/unloads 
passed their testing.

Is there more you're doing other than just rmmod?

Alan

Alexander Lobakin Feb. 20, 2024, 1:47 p.m. UTC | #9

From: Alexander Lobakin <[e-mail]>
Date: Wed, 14 Feb 2024 15:49:51 +0100

> From: Alexander Lobakin <[e-mail]>
> Date: Tue, 6 Feb 2024 18:02:33 +0100
> 
>> From: Alan Brady <[e-mail]>
>> Date: Mon, 5 Feb 2024 19:37:54 -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.
>>
>> This works better than v3.
>> For the basic scenarios:
>>
>> Tested-by: Alexander Lobakin <[e-mail]>
> 
> Sorry I'm reverting my tag.
> After the series, the CP segfaults on each rmmod idpf:
> 
> root@mev-imc:/usr/bin/cplane# cp_pim_mdd_handler: MDD interrupt detected
> cp_pim_mdd_event_handler: reg = 40
> Jan  1 00:27:57 mev-imc local0.warn LanCP[190]: cp_pim_mdd_handler: MDD
> interrupt detected
> cp_pim_mdd_event_handler: reg = 1
> Jan  1 00:28:54 mev-imc local0.warn LanCP[190]: [hma_create_vport/4257]
> WARNING: RSS is configured on 1st contiguous num of queuJan  1 00:28:54
> mev-imc local0.warn LanCP[190]: [hma_create_vport/4257] WARNING: RSS is
> configured on 1st contiguous num of queuJan  1 00:28:55 mev-imc
> local0.warn LanCP[190]: [hma_create_vport/4257] WARNING: RSS is
> configured on 1st contiguous num of queues= 16 start Qid= 34
> Jan  1 00:28:55 mev-imc local0.warn LanCP[190]: [hma_create_vport/4257]
> WARNING: RSS is configured on 1st contiguous num of queu16 start Qid= 640
> Jan  1 00:28:55 mev-imc local0.err LanCP[190]:
> [cp_del_node_rxbuff_lst/4179] ERR: Resource list is empty, so nothing to
> delete here
> Jan  1 00:29:08 mev-imc local0.err LanCP[190]:
> [cp_uninit_vsi_tc_q_region/222] ERR: Failed to init vsi LUT on vsi 1.
> Jan  1 00::08 mev-imc local0.err LanCP[190]:
> [cp_uninit_vsi_fxp_config/1101] ERR: cp_uninit_vsi_rss_config() failed
> on vsi (1).
> Jan  1 00:29:08 mev-imc local0.err LanCP[190]:
> [cp_uninit_vsi_tc_q_region/222] ERR: Failed to init vsi LUT on vsi 6.
> Jan  1 00:29:08 mev-imc local0.err LanCP[190]:
> [cp_uninit_vsi_rss_config/340] ERR: faininit_vsi_rss_config() failed on
> vsi (6).
> Jan  1 00:29:08 mev-imc local0.err LanCP[190]:
> [cp_uninit_vsi_tc_q_region/222] ERR: Failed to init vsi LUT on vsi 7.
> Jan  1 00:29:08 mev-imc local0.err LanCP[190]:
> [cp_uninit_vsi_rss_config/340] ERR: failed to remove vsi (7)'s queue
> regions.
> Jan  1 00:29:08 mev-imc local0.err LanCP[190]:
> [cp_uninit_vsi_fxp_config/1101] ERR: cp_uninit_vo init vsi LUT on vsi 8.
> Jan  1 00:29:08 mev-imc local0.err LanCP[190]:
> [cp_uninit_vsi_rss_config/340] ERR: failed to remove vsi (8)'s queue
> regions.
> Jan  1 00:29:08 mev-imc local0.err LanCP[190]:
> [cp_uninit_vsi_fxp_config/1101] ERR: cp_uninit_vsi_rss_config() failed
> on vsi (8).
> Jan  1 00:29:08 mev-imc local0.err LanCP[190]:
> [cp_uninit_vsi_tc_q_region/222] ERR: Failed to init vsi LUT on vsi 1.
> Jan  1 00::08 mev-imc local0.err LanCP[190]:
> [cp_uninit_vsi_fxp_config/1101] ERR: cp_uninit_vsi_rss_config() failed
> on vsi (1).
> 
> [1]+  Segmentation fault      ./imccp 0000:00:01.6 0 cp_init.cfg
> 
> Only restarting the CP helps -- restarting the imccp daemon makes it
> immediately crash again.
> 
> This should be dropped from the next-queue until it's fixed.

The latest firmware works with this series -- the problem was there,
the series only revealed it.

Tested-by: Alexander Lobakin <[e-mail]>

Can be taken back to Tony's tree.

Thanks,
Olek