linuxkpi: Introduce a properly typed jiffies

[markj]

Summary

Now that we have a long-sized tick counter, we can migrate to using
properly typed timeout parameters in various bits of the LinuxKPI.

• Introduce a "jiffies" symbol in subr_ticks.S, declared only in the LinuxKPI as an unsigned long.
• Remove all references to "ticks" from the LinuxKPI.
• Convert interfaces to match Linux's type signatures where it makes sense.

[emaste]

sys/compat/linuxkpi/common/include/linux/jiffies.h

39–40

Not sure what this comment is getting at

40

We might want to switch the order, making jififes_64 real -- AFAICT this is the case in Linux and jiffies is set to alias it (or jiffies_64 + 4) by linker script.

[manu]

sys/compat/linuxkpi/common/include/linux/jiffies.h

39–40

The comment should probably says that it's defined in subr_ticks.S

[markj]

sys/compat/linuxkpi/common/include/linux/jiffies.h

39–40

The jiffies symbol is defined in the kernel, not in linuxkpi.ko.

40

But we're not implementing jiffies_64 here, just jiffies. On 64-bit platforms they're equivalent, but I didn't implement a 64-bit tick counter for 32-bit platforms: it's more complex, 32-bit platforms are slowly going away, and jiffies_64 is not widely used.

[markj]

Make the comment more useful.

[markj]

Any testing of the patch, whether it's DRM, wifi, or the infiniband stack, would be appreciated.

[manu]

In D48523#1107219, @markj wrote:

Any testing of the patch, whether it's DRM, wifi, or the infiniband stack, would be appreciated.

Ah, I was going to ask you if you tested drm with this :)

[markj]

In D48523#1107240, @manu wrote:

In D48523#1107219, @markj wrote:

Any testing of the patch, whether it's DRM, wifi, or the infiniband stack, would be appreciated.

Ah, I was going to ask you if you tested drm with this :)

Not yet. I plan to, just haven't gotten around to it yet. And, I'm only set up to test i915kms at the moment. I can also test iwlwifi, but not other wifi drivers.

[bz]

I'll try to do both the testing and a review in the next 72 hours if that is ok.

[markj]

In D48523#1107340, @bz wrote:

I'll try to do both the testing and a review in the next 72 hours if that is ok.

Thank you, no rush at all.

[kib]

sys/compat/linuxkpi/common/include/linux/jiffies.h

43

What is this constant about?

sys/compat/linuxkpi/common/include/linux/wait.h

151–153

Why these casts (old/int new/long) are needed at all?

169

Are linux jiffies unsigned?

Anyway, I suspect the cast there is nop.

[emaste]

sys/compat/linuxkpi/common/include/linux/jiffies.h

40

Yeah, I realized that after leaving the comment. This probably warrants an XXX comment that this is broken on 32-bit platforms.

sys/compat/linuxkpi/common/include/linux/wait.h

169

Linux jiffies are indeed unsigned

[markj]

sys/compat/linuxkpi/common/include/linux/jiffies.h

43

It's defined in Linux as well, seems to be used as a maximum value when converting jiffies to other units.

sys/compat/linuxkpi/common/include/linux/wait.h

151–153

I'm not sure if it's needed or not, but it causes negative values to be treated as in the past.

[markj]

Fix MAX_JIFFY_OFFSET, remove an uneeded cast, add an XXX comment for jiffies_64.

[ddaniel_nvidia.com]

When testing the new patch, I encountered the same issue where the mce0 device disappeared,
accompanied by the following error in the dmesg log.

Dmesg:
mce0: link state changed to DOWN
mce1: link state changed to DOWN
mlx5_core: INFO: (mlx5_core0): E-Switch: cleanup
mlx5_core0: WARN: wait_func:968:(pid 89646): DESTROY_EQ(0x302) timeout. Will cause a leak of a command resource
mlx5_core0: WARN: mlx5_destroy_unmap_eq:528:(pid 89646): failed to destroy a previously created eq: eqn 4
mlx5_core0: WARN: wait_func:968:(pid 89646): DEALLOC_UAR(0x803) timeout. Will cause a leak of a command resource
mlx5_core0: WARN: up_rel_func:87:(pid 89646): failed to free uar index 256
mlx5_core0: WARN: wait_func:968:(pid 89646): TEARDOWN_HCA(0x103) timeout. Will cause a leak of a command resource
mlx5_core0: ERR: mlx5_unload_one:1345:(pid 89646): tear_down_hca failed, skip cleanup
mlx5_core0: ERR: remove_one:1786:(pid 89646): mlx5_unload_one() failed, leaked 14360576 bytes
mlx5_core0: detached
pci3: <network, ethernet> at device 0.0 (no driver attached)
mlx5_core: INFO: (mlx5_core1): E-Switch: cleanup
mlx5_core1: WARN: wait_func:968:(pid 89646): DESTROY_EQ(0x302) timeout. Will cause a leak of a command resource
mlx5_core1: WARN: mlx5_destroy_unmap_eq:528:(pid 89646): failed to destroy a previously created eq: eqn 4
mlx5_core1: WARN: wait_func:968:(pid 89646): DEALLOC_UAR(0x803) timeout. Will cause a leak of a command resource
mlx5_core1: WARN: up_rel_func:87:(pid 89646): failed to free uar index 256
mlx5_core1: WARN: wait_func:968:(pid 89646): TEARDOWN_HCA(0x103) timeout. Will cause a leak of a command resource
mlx5_core1: ERR: mlx5_unload_one:1345:(pid 89646): tear_down_hca failed, skip cleanup
mlx5_core1: ERR: remove_one:1786:(pid 89646): mlx5_unload_one() failed, leaked 14413824 bytes
mlx5_core1: detached
pci3: <network, ethernet> at device 0.1 (no driver attached)
mlx5_core0: <mlx5_core> mem 0x880000000-0x881ffffff irq 22 at device 0.0 on pci3
mlx5: Mellanox Core driver 3.7.1 (November 2021)mlx5_core0: WARN: wait_func:968:(pid 89677): ENABLE_HCA(0x104) timeout. Will cause a leak of a command resource
mlx5_core0: ERR: mlx5_load_one:1124:(pid 89677): enable hca failed
mlx5_core0: ERR: init_one:1709:(pid 89677): mlx5_load_one failed -60
device_attach: mlx5_core0 attach returned 60
mlx5_core0: <mlx5_core> mem 0x882000000-0x883ffffff irq 23 at device 0.1 on pci3
mlx5_core0: WARN: wait_func:968:(pid 89677): ENABLE_HCA(0x104) timeout. Will cause a leak of a command resource
mlx5_core0: ERR: mlx5_load_one:1124:(pid 89677): enable hca failed
mlx5_core0: ERR: init_one:1709:(pid 89677): mlx5_load_one failed -60
device_attach: mlx5_core0 attach returned 60

repro:
for i in {1..40}; do

echo "Iteration $i"
kldunload mlx5en
kldunload mlx5ib
sleep 1
kldload mlx5en
kldload mlx5ib
sleep 1

done

[markj]

In D48523#1111249, @ddaniel_nvidia.com wrote:

When testing the new patch, I encountered the same issue where the mce0 device disappeared,
accompanied by the following error in the dmesg log.

Thank you for testing. It seems that mlx5 (and probably mlx4) assume that jiffies is an int in some places, e.g., wait_func(). I am willing to work on converting these to unsigned long where appropriate. Presumably this would ease maintenance of shared code, but I am not the maintainer of these drivers - how would you like to handle it?

BTW, I have access to a mlx5 device and can test it locally, but would need help with mlx4.

[kib]

In D48523#1111690, @markj wrote:

In D48523#1111249, @ddaniel_nvidia.com wrote:

When testing the new patch, I encountered the same issue where the mce0 device disappeared,
accompanied by the following error in the dmesg log.

Thank you for testing. It seems that mlx5 (and probably mlx4) assume that jiffies is an int in some places, e.g., wait_func(). I am willing to work on converting these to unsigned long where appropriate. Presumably this would ease maintenance of shared code, but I am not the maintainer of these drivers - how would you like to handle it?

Indeed I tried the following but it was not enough at least.

From ce2d32a56f3f73df01b30e3a6142565cbd869def Mon Sep 17 00:00:00 2001
From: Konstantin Belousov <[e-mail]>
Date: Mon, 27 Jan 2025 14:12:05 +0200
Subject: [PATCH] mlx5: jiffies is unsigned long

---
 sys/dev/mlx5/mlx5_core/mlx5_cmd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sys/dev/mlx5/mlx5_core/mlx5_cmd.c b/sys/dev/mlx5/mlx5_core/mlx5_cmd.c
index 8ce30bc24e50..52c29c5511f2 100644
--- a/sys/dev/mlx5/mlx5_core/mlx5_cmd.c
+++ b/sys/dev/mlx5/mlx5_core/mlx5_cmd.c
@@ -247,7 +247,7 @@ static void poll_timeout(struct mlx5_cmd_work_ent *ent)
 {
 	struct mlx5_core_dev *dev = container_of(ent->cmd,
 						 struct mlx5_core_dev, cmd);
-	int poll_end = jiffies +
+	long poll_end = jiffies +
 				msecs_to_jiffies(MLX5_CMD_TIMEOUT_MSEC + 1000);
 	u8 own;
 
@@ -950,7 +950,7 @@ static const char *deliv_status_to_str(u8 status)
 
 static int wait_func(struct mlx5_core_dev *dev, struct mlx5_cmd_work_ent *ent)
 {
-	int timeout = msecs_to_jiffies(MLX5_CMD_TIMEOUT_MSEC);
+	unsigned long timeout = msecs_to_jiffies(MLX5_CMD_TIMEOUT_MSEC);
 	int err;
 
 	if (ent->polling) {
-- 
2.48.1

[markj]

In D48523#1111725, @kib wrote:

In D48523#1111690, @markj wrote:

In D48523#1111249, @ddaniel_nvidia.com wrote:

When testing the new patch, I encountered the same issue where the mce0 device disappeared,
accompanied by the following error in the dmesg log.

Thank you for testing. It seems that mlx5 (and probably mlx4) assume that jiffies is an int in some places, e.g., wait_func(). I am willing to work on converting these to unsigned long where appropriate. Presumably this would ease maintenance of shared code, but I am not the maintainer of these drivers - how would you like to handle it?

Indeed I tried the following but it was not enough at least.

I think pretty much every use of msecs_to_jiffies() in mlx5 needs to be fixed. Every reference to jiffies needs to be audited too, at least.

[markj]

Update round_jiffies() as well.

[kib]

In D48523#1111727, @markj wrote:

I think pretty much every use of msecs_to_jiffies() in mlx5 needs to be fixed. Every reference to jiffies needs to be audited too, at least.

Ok I will do the pass.

[kib]

In D48523#1111731, @kib wrote:

In D48523#1111727, @markj wrote:

I think pretty much every use of msecs_to_jiffies() in mlx5 needs to be fixed. Every reference to jiffies needs to be audited too, at least.

Ok I will do the pass.

This diff + D48878 passed internal NVidia verification for mlx5.

[markj]

In D48523#1114719, @kib wrote:

In D48523#1111731, @kib wrote:

In D48523#1111727, @markj wrote:

I think pretty much every use of msecs_to_jiffies() in mlx5 needs to be fixed. Every reference to jiffies needs to be audited too, at least.

Ok I will do the pass.

This diff + D48878 passed internal NVidia verification for mlx5.

Thank you. I looked at mlx4 for a while and didn't spot any issues - it's already using unsigned long for ticks values. However, I'm not able to test it.

I guess the two patches must be committed together to avoid creating a window where mlx5 is broken. Is it ok if I include your patch with mine? I am not ready to commit yet, I'm still testing the patch on my desktop.

[kib]

In D48523#1115379, @markj wrote:

In D48523#1114719, @kib wrote:

In D48523#1111731, @kib wrote:

In D48523#1111727, @markj wrote:

I think pretty much every use of msecs_to_jiffies() in mlx5 needs to be fixed. Every reference to jiffies needs to be audited too, at least.

Ok I will do the pass.

This diff + D48878 passed internal NVidia verification for mlx5.

Thank you. I looked at mlx4 for a while and didn't spot any issues - it's already using unsigned long for ticks values. However, I'm not able to test it.

I do not think that NVidia currently can test mlx4 either.

I guess the two patches must be committed together to avoid creating a window where mlx5 is broken. Is it ok if I include your patch with mine? I am not ready to commit yet, I'm still testing the patch on my desktop.

There are two patches in my repo, one for dev/mlx5, another for sys/ofed. I can commit them myself shortly after you push the linuxkpi change. Or take mine from https://kib.kiev.ua/git/gitweb.cgi?p=deviant3.git;a=shortlog;h=refs/heads/ipsec2

[bz]

sys/compat/linuxkpi/common/include/linux/jiffies.h

71–72

timespec_to_jiffies is gone on main now, can you rebase to avoid conflicts when applying this?

[markj]

Rebase.

[bz]

I've run with this for 14 days now (after the two initial panics I couldn't reproduce the panic anymore so I assume it wasn't anything related to this). It's i915 here. Also I have done some reltek testing but if you want more of that there are probably two people on PR 283903 who could be convinced to test it with rtw88 in their laptops.

[kib]

Is this going to land? We gather changes for mlx5 which might start affecting D48878, and ideally that should be committed before the whole work rotten.

[markj]

In D48523#1141365, @kib wrote:

Is this going to land? We gather changes for mlx5 which might start affecting D48878, and ideally that should be committed before the whole work rotten.

Sorry, please give me a day or two to re-test and commit.

[bz]

I've run it since February on my laptop and didn't notice side effects. The initial crashes were due to unrelated drm-kmod things.

[markj]

In D48523#1141365, @kib wrote:

Is this going to land? We gather changes for mlx5 which might start affecting D48878, and ideally that should be committed before the whole work rotten.

Is it ok to commit the mlx5 changes together in one commit? That is, I would push this: https://github.com/markjdb/freebsd/commit/8b0636b48ad9a92662afe13fe715bb4368ab752e

[kib]

In D48523#1141758, @markj wrote:

In D48523#1141365, @kib wrote:

Is this going to land? We gather changes for mlx5 which might start affecting D48878, and ideally that should be committed before the whole work rotten.

Is it ok to commit the mlx5 changes together in one commit? That is, I would push this: https://github.com/markjdb/freebsd/commit/8b0636b48ad9a92662afe13fe715bb4368ab752e

I would prefer the mlx5 changes to go as separate commits. It is fine if you push them in the same push transaction as this one, but please do not squash.
The commits are there:
https://kib.kiev.ua/git/gitweb.cgi?p=deviant3.git;a=commit;h=f3bba80921bc77fe43e81559b87613eee23963ba
https://kib.kiev.ua/git/gitweb.cgi?p=deviant3.git;a=commit;h=92cd1f7502adcbf1b89c053048bcf6a787a496f1

[markj]

In D48523#1141792, @kib wrote:

In D48523#1141758, @markj wrote:

In D48523#1141365, @kib wrote:

Is this going to land? We gather changes for mlx5 which might start affecting D48878, and ideally that should be committed before the whole work rotten.

Is it ok to commit the mlx5 changes together in one commit? That is, I would push this: https://github.com/markjdb/freebsd/commit/8b0636b48ad9a92662afe13fe715bb4368ab752e

I would prefer the mlx5 changes to go as separate commits. It is fine if you push them in the same push transaction as this one, but please do not squash.
The commits are there:
https://kib.kiev.ua/git/gitweb.cgi?p=deviant3.git;a=commit;h=f3bba80921bc77fe43e81559b87613eee23963ba
https://kib.kiev.ua/git/gitweb.cgi?p=deviant3.git;a=commit;h=92cd1f7502adcbf1b89c053048bcf6a787a496f1

This is not so good since it breaks bisection, but ok.