| Message ID | 20250225-converge-secs-to-jiffies-part-two-v3-0-a43967e36c88@linux.microsoft.com |
|---|---|
| Headers | show |
| Series | Converge on using secs_to_jiffies() part two | expand |
On Tue, 25 Feb 2025 20:17:14 +0000, Easwar Hariharan wrote: > This is the second series (part 1*) that converts users of msecs_to_jiffies() that > either use the multiply pattern of either of: > - msecs_to_jiffies(N*1000) or > - msecs_to_jiffies(N*MSEC_PER_SEC) > > where N is a constant or an expression, to avoid the multiplication. > > [...] Applied, thanks! [06/16] rbd: convert timeouts to secs_to_jiffies() commit: c02eea7eeaebd7270cb8ff09049cc7e0fc9bc8da Best regards,
On 2/26/25 5:24 AM, Easwar Hariharan wrote: > On 2/25/2025 12:17 PM, Easwar Hariharan wrote: >> Commit b35108a51cf7 ("jiffies: Define secs_to_jiffies()") introduced >> secs_to_jiffies(). As the value here is a multiple of 1000, use >> secs_to_jiffies() instead of msecs_to_jiffies() to avoid the multiplication >> >> This is converted using scripts/coccinelle/misc/secs_to_jiffies.cocci with >> the following Coccinelle rules: >> >> @depends on patch@ >> expression E; >> @@ >> >> -msecs_to_jiffies >> +secs_to_jiffies >> (E >> - * \( 1000 \| MSEC_PER_SEC \) >> ) >> >> Signed-off-by: Easwar Hariharan <eahariha@linux.microsoft.com> >> --- > > This was meant to carry Damien's ack once the subject line was fixed[1], but I fixed > the subject line and missed adding the ack in. Damien, would you like to ack again? Looks like Andrew already applied the patch, which is fine. But nevertheless: Acked-by: Damien Le Moal <dlemoal@kernel.org>
Le 26/02/2025 à 08:28, Daniel Vacek a écrit : > On Tue, 25 Feb 2025 at 22:10, Christophe JAILLET > <christophe.jaillet-39ZsbGIQGT5GWvitb5QawA@public.gmane.org> wrote: >> >> Le 25/02/2025 à 21:17, Easwar Hariharan a écrit : >>> Commit b35108a51cf7 ("jiffies: Define secs_to_jiffies()") introduced >>> secs_to_jiffies(). As the value here is a multiple of 1000, use >>> secs_to_jiffies() instead of msecs_to_jiffies() to avoid the multiplication >>> >>> This is converted using scripts/coccinelle/misc/secs_to_jiffies.cocci with >>> the following Coccinelle rules: >>> >>> @depends on patch@ expression E; @@ >>> >>> -msecs_to_jiffies(E * 1000) >>> +secs_to_jiffies(E) >>> >>> @depends on patch@ expression E; @@ >>> >>> -msecs_to_jiffies(E * MSEC_PER_SEC) >>> +secs_to_jiffies(E) >>> >>> While here, remove the no-longer necessary check for range since there's >>> no multiplication involved. >> >> I'm not sure this is correct. >> Now you multiply by HZ and things can still overflow. > > This does not deal with any additional multiplications. If there is an > overflow, it was already there before to begin with, IMO. > >> Hoping I got casting right: > > Maybe not exactly? See below... > >> #define MSEC_PER_SEC 1000L >> #define HZ 100 >> >> >> #define secs_to_jiffies(_secs) (unsigned long)((_secs) * HZ) >> >> static inline unsigned long _msecs_to_jiffies(const unsigned int m) >> { >> return (m + (MSEC_PER_SEC / HZ) - 1) / (MSEC_PER_SEC / HZ); >> } >> >> int main() { >> >> int n = INT_MAX - 5; >> >> printf("res = %ld\n", secs_to_jiffies(n)); >> printf("res = %ld\n", _msecs_to_jiffies(1000 * n)); > > I think the format should actually be %lu giving the below results: > > res = 18446744073709551016 > res = 429496130 > > Which is still wrong nonetheless. But here, *both* results are wrong > as the expected output should be 214748364200 which you'll get with > the correct helper/macro. > > But note another thing, the 1000 * (INT_MAX - 5) already overflows > even before calling _msecs_to_jiffies(). See? Agreed and intentional in my test C code. That is the point. The "if (result.uint_32 > INT_MAX / 1000)" in the original code was handling such values. > > Now, you'll get that mentioned correct result with: > > #define secs_to_jiffies(_secs) ((unsigned long)(_secs) * HZ) Not looked in details, but I think I would second on you on this, in this specific example. Not sure if it would handle all possible uses of secs_to_jiffies(). But it is not how secs_to_jiffies() is defined up to now. See [1]. [1]: https://elixir.bootlin.com/linux/v6.14-rc4/source/include/linux/jiffies.h#L540 > > Still, why unsigned? What if you wanted to convert -5 seconds to jiffies? See commit bb2784d9ab495 which added the cast. > >> return 0; >> } >> >> >> gives : >> >> res = -600 >> res = 429496130 >> >> with msec, the previous code would catch the overflow, now it overflows >> silently. > > What compiler options are you using? I'm not getting any warnings. I mean, with: if (result.uint_32 > INT_MAX / 1000) goto out_of_range; the overflow would be handled *at runtime*. Without such a check, an unexpected value could be stored in opt->lock_timeout. I think that a test is needed and with secs_to_jiffies(), I tentatively proposed: if (result.uint_32 > INT_MAX / HZ) goto out_of_range; CJ > >> untested, but maybe: >> if (result.uint_32 > INT_MAX / HZ) >> goto out_of_range; >> >> ? >> >> CJ >> ...
On Wed, 26 Feb 2025 at 09:10, Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote: > > Le 26/02/2025 à 08:28, Daniel Vacek a écrit : > > On Tue, 25 Feb 2025 at 22:10, Christophe JAILLET > > <christophe.jaillet-39ZsbGIQGT5GWvitb5QawA@public.gmane.org> wrote: > >> > >> Le 25/02/2025 à 21:17, Easwar Hariharan a écrit : > >>> Commit b35108a51cf7 ("jiffies: Define secs_to_jiffies()") introduced > >>> secs_to_jiffies(). As the value here is a multiple of 1000, use > >>> secs_to_jiffies() instead of msecs_to_jiffies() to avoid the multiplication > >>> > >>> This is converted using scripts/coccinelle/misc/secs_to_jiffies.cocci with > >>> the following Coccinelle rules: > >>> > >>> @depends on patch@ expression E; @@ > >>> > >>> -msecs_to_jiffies(E * 1000) > >>> +secs_to_jiffies(E) > >>> > >>> @depends on patch@ expression E; @@ > >>> > >>> -msecs_to_jiffies(E * MSEC_PER_SEC) > >>> +secs_to_jiffies(E) > >>> > >>> While here, remove the no-longer necessary check for range since there's > >>> no multiplication involved. > >> > >> I'm not sure this is correct. > >> Now you multiply by HZ and things can still overflow. > > > > This does not deal with any additional multiplications. If there is an > > overflow, it was already there before to begin with, IMO. > > > >> Hoping I got casting right: > > > > Maybe not exactly? See below... > > > >> #define MSEC_PER_SEC 1000L > >> #define HZ 100 > >> > >> > >> #define secs_to_jiffies(_secs) (unsigned long)((_secs) * HZ) > >> > >> static inline unsigned long _msecs_to_jiffies(const unsigned int m) > >> { > >> return (m + (MSEC_PER_SEC / HZ) - 1) / (MSEC_PER_SEC / HZ); > >> } > >> > >> int main() { > >> > >> int n = INT_MAX - 5; > >> > >> printf("res = %ld\n", secs_to_jiffies(n)); > >> printf("res = %ld\n", _msecs_to_jiffies(1000 * n)); > > > > I think the format should actually be %lu giving the below results: > > > > res = 18446744073709551016 > > res = 429496130 > > > > Which is still wrong nonetheless. But here, *both* results are wrong > > as the expected output should be 214748364200 which you'll get with > > the correct helper/macro. > > > > But note another thing, the 1000 * (INT_MAX - 5) already overflows > > even before calling _msecs_to_jiffies(). See? > > Agreed and intentional in my test C code. > > That is the point. > > The "if (result.uint_32 > INT_MAX / 1000)" in the original code was > handling such values. I see. But that was rather an unrelated side-effect. Still you're right, it needs to be handled carefully not to remove additional guarantees which were implied unintentionally. At least in places where these were provided in the first place. > > > > Now, you'll get that mentioned correct result with: > > > > #define secs_to_jiffies(_secs) ((unsigned long)(_secs) * HZ) > > Not looked in details, but I think I would second on you on this, in > this specific example. Not sure if it would handle all possible uses of > secs_to_jiffies(). Yeah, I was referring only in context of the example you presented, not for the rest of the kernel. Sorry about the confusion. > But it is not how secs_to_jiffies() is defined up to now. See [1]. > > [1]: > https://elixir.bootlin.com/linux/v6.14-rc4/source/include/linux/jiffies.h#L540 > > > > > Still, why unsigned? What if you wanted to convert -5 seconds to jiffies? > > See commit bb2784d9ab495 which added the cast. Hmmm, fishy. Maybe a function would be better than a macro? > > > >> return 0; > >> } > >> > >> > >> gives : > >> > >> res = -600 > >> res = 429496130 > >> > >> with msec, the previous code would catch the overflow, now it overflows > >> silently. > > > > What compiler options are you using? I'm not getting any warnings. > > I mean, with: > if (result.uint_32 > INT_MAX / 1000) > goto out_of_range; > the overflow would be handled *at runtime*. Got it. But that may still fail if you configure HZ to 5000 or anything above 1000. Not that anyone should go this way but... > Without such a check, an unexpected value could be stored in > opt->lock_timeout. > > I think that a test is needed and with secs_to_jiffies(), I tentatively > proposed: > if (result.uint_32 > INT_MAX / HZ) > goto out_of_range; Right, that should correctly handle any HZ value. Looks good to me. > CJ > > > > >> untested, but maybe: > >> if (result.uint_32 > INT_MAX / HZ) > >> goto out_of_range; > >> > >> ? > >> > >> CJ > >> > > ...
On Tue, Feb 25, 2025 at 08:17:14PM +0000, Easwar Hariharan wrote: > This is the second series (part 1*) that converts users of msecs_to_jiffies() that > either use the multiply pattern of either of: > - msecs_to_jiffies(N*1000) or > - msecs_to_jiffies(N*MSEC_PER_SEC) > > where N is a constant or an expression, to avoid the multiplication. Please don't combine patches for multiple subsystems into a single series if there's no dependencies between them, it just creates confusion about how things get merged, problems for tooling and makes everything more noisy. It's best to split things up per subsystem in that case.
On Tue, 25 Feb 2025 20:17:14 +0000, Easwar Hariharan wrote: > This is the second series (part 1*) that converts users of msecs_to_jiffies() that > either use the multiply pattern of either of: > - msecs_to_jiffies(N*1000) or > - msecs_to_jiffies(N*MSEC_PER_SEC) > > where N is a constant or an expression, to avoid the multiplication. > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next Thanks! [12/16] spi: spi-fsl-lpspi: convert timeouts to secs_to_jiffies() commit: 32fcd1b9c397ccca7fde2fcbcf4fc7e0ec8f34aa [13/16] spi: spi-imx: convert timeouts to secs_to_jiffies() commit: 1d2e01d53a8ebfffb49e8cc656f8c85239121b26 All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
On Wed, 26 Feb 2025 11:29:53 +0000 Mark Brown <broonie@kernel.org> wrote: > On Tue, Feb 25, 2025 at 08:17:14PM +0000, Easwar Hariharan wrote: > > This is the second series (part 1*) that converts users of msecs_to_jiffies() that > > either use the multiply pattern of either of: > > - msecs_to_jiffies(N*1000) or > > - msecs_to_jiffies(N*MSEC_PER_SEC) > > > > where N is a constant or an expression, to avoid the multiplication. > > Please don't combine patches for multiple subsystems into a single > series if there's no dependencies between them, it just creates > confusion about how things get merged, problems for tooling and makes > everything more noisy. It's best to split things up per subsystem in > that case. I asked for this. I'll merge everything, spend a few weeks gathering up maintainer acks. Anything which a subsystem maintainer merges will be reported by Stephen and I'll drop that particular patch. This way, nothing gets lost. I take this approach often and it works. If these were sent as a bunch of individual patches then it would be up to the sender to keep track of what has been merged and what hasn't. That person will be resending some stragglers many times. Until they give up and some patches get permanently lost. Scale all that across many senders and the whole process becomes costly and unreliable. Whereas centralizing it on akpm is more efficient, more reliable, more scalable, lower latency and less frustrating for senders.
On Wed, Feb 26, 2025 at 12:38:51PM -0800, Andrew Morton wrote: > On Wed, 26 Feb 2025 11:29:53 +0000 Mark Brown <broonie@kernel.org> wrote: > > Please don't combine patches for multiple subsystems into a single > > series if there's no dependencies between them, it just creates > > confusion about how things get merged, problems for tooling and makes > > everything more noisy. It's best to split things up per subsystem in > > that case. > I asked for this. I'll merge everything, spend a few weeks gathering > up maintainer acks. Anything which a subsystem maintainer merges will > be reported by Stephen and I'll drop that particular patch. > This way, nothing gets lost. I take this approach often and it works. I've only started seeing these in the past few weeks, but we do have a bunch of people routinely doing cross tree stuff who split things up and it seems to work OK there. > If these were sent as a bunch of individual patches then it would be up > to the sender to keep track of what has been merged and what hasn't. > That person will be resending some stragglers many times. Until they > give up and some patches get permanently lost. Surely the sender can just CC you on each individual thing just as well? Ensuring things get picked up is great, but it's not clear to me that copying everyone on a cross tree series is helping with that. > Scale all that across many senders and the whole process becomes costly > and unreliable. Whereas centralizing it on akpm is more efficient, > more reliable, more scalable, lower latency and less frustrating for > senders. Whereas copying everyone means all the maintainers see something that looks terribly complicated in their inboxes and have to figure out if there are actually any dependencies in the series and how it's supposed to be handed, and then every reply goes to a huge CC list. That's not good for either getting people to look at things or general noise avoidance, especially for people who are expecting to get cross tree serieses which do have dependencies that need to be managed. There's also some bad failure modes as soon as anyone has any sort of comment on the series, suddenly everyone's got a coding style debate or whatever in their inboxes they can pile into.
On Wed, Feb 26, 2025 at 12:38:51PM -0800, Andrew Morton wrote: > On Wed, 26 Feb 2025 11:29:53 +0000 Mark Brown <broonie@kernel.org> wrote: > > > On Tue, Feb 25, 2025 at 08:17:14PM +0000, Easwar Hariharan wrote: > > > This is the second series (part 1*) that converts users of msecs_to_jiffies() that > > > either use the multiply pattern of either of: > > > - msecs_to_jiffies(N*1000) or > > > - msecs_to_jiffies(N*MSEC_PER_SEC) > > > > > > where N is a constant or an expression, to avoid the multiplication. > > > > Please don't combine patches for multiple subsystems into a single > > series if there's no dependencies between them, it just creates > > confusion about how things get merged, problems for tooling and makes > > everything more noisy. It's best to split things up per subsystem in > > that case. > > I asked for this. I'll merge everything, spend a few weeks gathering > up maintainer acks. Anything which a subsystem maintainer merges will > be reported by Stephen and I'll drop that particular patch. I'm removing this from my queue then and let it go through your tree. Cheers, Carlos > > This way, nothing gets lost. I take this approach often and it works. > > If these were sent as a bunch of individual patches then it would be up > to the sender to keep track of what has been merged and what hasn't. > That person will be resending some stragglers many times. Until they > give up and some patches get permanently lost. > > Scale all that across many senders and the whole process becomes costly > and unreliable. Whereas centralizing it on akpm is more efficient, > more reliable, more scalable, lower latency and less frustrating for > senders. >
On Tue, 25 Feb 2025 20:17:14 +0000, Easwar Hariharan wrote: > This is the second series (part 1*) that converts users of msecs_to_jiffies() that > either use the multiply pattern of either of: > - msecs_to_jiffies(N*1000) or > - msecs_to_jiffies(N*MSEC_PER_SEC) > > where N is a constant or an expression, to avoid the multiplication. > > [...] Applied to 6.15/scsi-queue, thanks! [02/16] scsi: lpfc: convert timeouts to secs_to_jiffies() https://git.kernel.org/mkp/scsi/c/a131f20804d6
Hi Andrew! > Really, an acked-by would have been much easier all around, but > whatever. Hard to keep track of which of these kernel-wide series go through one tree and which ones don't. I generally err on the side of picking up things which may conflict with driver updates in my tree. Judging by the commit message, I did notice the missing parenthesis. Thanks!
This is the second series (part 1*) that converts users of msecs_to_jiffies() that either use the multiply pattern of either of: - msecs_to_jiffies(N*1000) or - msecs_to_jiffies(N*MSEC_PER_SEC) where N is a constant or an expression, to avoid the multiplication. The conversion is made with Coccinelle with the secs_to_jiffies() script in scripts/coccinelle/misc. Attention is paid to what the best change can be rather than restricting to what the tool provides. Andrew has kindly agreed to take the series through mm.git modulo the patches maintainers want to pick through their own trees. This series is based on next-20250225 Signed-off-by: Easwar Hariharan <eahariha@linux.microsoft.com> * https://lore.kernel.org/all/20241210-converge-secs-to-jiffies-v3-0-ddfefd7e9f2a@linux.microsoft.com/ --- Changes in v3: - Change commit message prefix from libata: zpodd to ata: libata-zpodd: in patch 8 (Damien) - Split up overly long line in patch 9 (Christoph) - Fixup unnecessary line break in patch 14 (Ilpo) - Combine v1 and v2 - Fix some additional hunks in patch 2 (scsi: lpfc) which the more concise script missed - msecs_to_jiffies -> msecs_to_jiffies() in commit messages throughout - Bug in secs_to_jiffies() uncovered by LKP merged in 6.14-rc2: bb2784d9ab4958 ("jiffies: Cast to unsigned long in secs_to_jiffies() conversion") - Link to v2: https://lore.kernel.org/r/20250203-converge-secs-to-jiffies-part-two-v2-0-d7058a01fd0e@linux.microsoft.com Changes in v2: - Remove unneeded range checks in rbd and libceph. While there, convert some timeouts that should have been fixed in part 1. (Ilya) - Fixup secs_to_jiffies.cocci to be a bit more verbose - Link to v1: https://lore.kernel.org/r/20250128-converge-secs-to-jiffies-part-two-v1-0-9a6ecf0b2308@linux.microsoft.com --- Easwar Hariharan (16): coccinelle: misc: secs_to_jiffies: Patch expressions too scsi: lpfc: convert timeouts to secs_to_jiffies() accel/habanalabs: convert timeouts to secs_to_jiffies() ALSA: ac97: convert timeouts to secs_to_jiffies() btrfs: convert timeouts to secs_to_jiffies() rbd: convert timeouts to secs_to_jiffies() libceph: convert timeouts to secs_to_jiffies() ata: libata-zpodd: convert timeouts to secs_to_jiffies() xfs: convert timeouts to secs_to_jiffies() power: supply: da9030: convert timeouts to secs_to_jiffies() nvme: convert timeouts to secs_to_jiffies() spi: spi-fsl-lpspi: convert timeouts to secs_to_jiffies() spi: spi-imx: convert timeouts to secs_to_jiffies() platform/x86/amd/pmf: convert timeouts to secs_to_jiffies() platform/x86: thinkpad_acpi: convert timeouts to secs_to_jiffies() RDMA/bnxt_re: convert timeouts to secs_to_jiffies() .../accel/habanalabs/common/command_submission.c | 2 +- drivers/accel/habanalabs/common/debugfs.c | 2 +- drivers/accel/habanalabs/common/device.c | 2 +- drivers/accel/habanalabs/common/habanalabs_drv.c | 2 +- drivers/ata/libata-zpodd.c | 3 +- drivers/block/rbd.c | 8 ++--- drivers/infiniband/hw/bnxt_re/qplib_rcfw.c | 2 +- drivers/nvme/host/core.c | 6 ++-- drivers/platform/x86/amd/pmf/acpi.c | 2 +- drivers/platform/x86/thinkpad_acpi.c | 2 +- drivers/power/supply/da9030_battery.c | 3 +- drivers/scsi/lpfc/lpfc.h | 3 +- drivers/scsi/lpfc/lpfc_els.c | 11 +++--- drivers/scsi/lpfc/lpfc_hbadisc.c | 2 +- drivers/scsi/lpfc/lpfc_init.c | 10 +++--- drivers/scsi/lpfc/lpfc_scsi.c | 12 +++---- drivers/scsi/lpfc/lpfc_sli.c | 41 +++++++++------------- drivers/scsi/lpfc/lpfc_vport.c | 2 +- drivers/spi/spi-fsl-lpspi.c | 2 +- drivers/spi/spi-imx.c | 2 +- fs/btrfs/disk-io.c | 6 ++-- fs/xfs/xfs_icache.c | 2 +- fs/xfs/xfs_sysfs.c | 8 ++--- include/linux/ceph/libceph.h | 12 +++---- net/ceph/ceph_common.c | 18 ++++------ net/ceph/osd_client.c | 3 +- scripts/coccinelle/misc/secs_to_jiffies.cocci | 10 ++++++ sound/pci/ac97/ac97_codec.c | 3 +- 28 files changed, 82 insertions(+), 99 deletions(-) --- base-commit: 0226d0ce98a477937ed295fb7df4cc30b46fc304 change-id: 20241217-converge-secs-to-jiffies-part-two-f44017aa6f67 Best regards,