| Message ID | 20221022014403.3881893-1-Jason@zx2c4.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | convert tree to get_random_u32_{below,above,between}() | expand |
On Fri, 21 Oct 2022 21:43:58 -0400 Jason A. Donenfeld wrote: > Since get_random_u32_below() sits in my random.git tree, these patches > too will flow through that same tree. How big is it? Can you provide a stable branch to pull in the new helpers and then everyone will be able to apply the patches to their subsystem?
On Fri, Oct 21, 2022 at 08:55:22PM -0700, Jakub Kicinski wrote: > On Fri, 21 Oct 2022 21:43:58 -0400 Jason A. Donenfeld wrote: > > Since get_random_u32_below() sits in my random.git tree, these patches > > too will flow through that same tree. > > How big is it? Can you provide a stable branch to pull in the new > helpers and then everyone will be able to apply the patches to their > subsystem? It's a patch. But what you suggest sounds crazy to me. Supply some branch and have every tree merge that branch separately, in duplicate, and then get all of the conversion patches through every tree, and then somehow coordinate the removal of the deprecated function after all of that, and then baby sit the grand orchestration of all this over the course of two and half months, watch it fail because of some unmaintained corner that's affected, and then try to herd it through for another two and a half months after that? Holy crap. That's torture. Were this an actually technically interesting patchset that required some really detailed expert review, maybe that'd have some iota of merit. But this is a really boring refactoring, mostly automated with Coccinelle. If having to baby sit one hundred separate patches over the course of months, handling confusion of walking maintainers through the exercise of merging some weird duplicate branch into their trees before, and so forth, is required to get this kind of grunt work done, I'm just going to wind up losing all motivation for this kind of thing, and naturally, as a matter of human nature, stop doing it. The result will be that we have garbage pile up over time that operates on the principle of "least hassle to deal with for the time being" rather than "love of the code and a desire for long term maintainability and quality". The former is sometimes how things go. The latter is what I'm striving for. So what you suggest sounds really dreadful to me. Sorry. Instead, this series follows the same template as the last one, and the last one was much more nuanced and invasive and went fine. In the very worst case, it'll require me to be on the ball with what's happening with -next, which is something I've done before and can do again. Jason
On Sat, 22 Oct 2022 00:23:00 -0400 Jason A. Donenfeld wrote: > > How big is it? Can you provide a stable branch to pull in the new > > helpers and then everyone will be able to apply the patches to their > > subsystem? > > It's a patch. But what you suggest sounds crazy to me. Supply some > branch and have every tree merge that branch separately, in duplicate, > and then get all of the conversion patches through every tree, and then > somehow coordinate the removal of the deprecated function after all of > that, and then baby sit the grand orchestration of all this over the > course of two and half months, watch it fail because of some > unmaintained corner that's affected, and then try to herd it through for > another two and a half months after that? Holy crap. That's torture. I clean up some random networking API every couple of releases. Unfortunately other subsystems use networking APIs too, so I have to do what you describe as "torture". It's not that hard in my experience but perhaps I'm incredibly gifted. Or resilient to pain. > Were this an actually technically interesting patchset that required > some really detailed expert review, maybe that'd have some iota of > merit. But this is a really boring refactoring, mostly automated with > Coccinelle. If having to baby sit one hundred separate patches over the > course of months, handling confusion of walking maintainers through the > exercise of merging some weird duplicate branch into their trees before, > and so forth, is required to get this kind of grunt work done, I'm just > going to wind up losing all motivation for this kind of thing, and > naturally, as a matter of human nature, stop doing it. The result will > be that we have garbage pile up over time that operates on the principle > of "least hassle to deal with for the time being" rather than "love of > the code and a desire for long term maintainability and quality". The > former is sometimes how things go. The latter is what I'm striving for. > > So what you suggest sounds really dreadful to me. Sorry. > > Instead, this series follows the same template as the last one, and the > last one was much more nuanced and invasive and went fine. In the very > worst case, it'll require me to be on the ball with what's happening > with -next, which is something I've done before and can do again. Not sure what you mean by "being on the ball with what's happening with -next" surely it's Steven who'll be fixing the conflicts and paying with his time? Or carrying extra patches because neither you will be able to convert the new cases in your tree nor in the origin tree since it won't have your new helpers. To me putting the new helpers first, on a clean branch off Linus's tree so in case of emergency it can be pulled into a random^W arbitrary tree is just good hygiene. But whatever. I mean - hopefully there aren't any conflicts in the ~50 networking files you touch. I just wish that people didn't pipe up with the tree wide changes right after the merge window. Feels like the worst possible timing.
On Fri, Oct 21, 2022 at 10:32:42PM -0700, Jakub Kicinski wrote: > But whatever. I mean - hopefully there aren't any conflicts in the ~50 > networking files you touch. I just wish that people didn't pipe up with > the tree wide changes right after the merge window. Feels like the > worst possible timing. Oh, if the timing is what makes this especially worrisome, I have no qualms about rebasing much later, and reposting this series then. I'll do that. Jason
On Sat, 22 Oct 2022 07:47:06 +0200 Jason A. Donenfeld wrote: > On Fri, Oct 21, 2022 at 10:32:42PM -0700, Jakub Kicinski wrote: > > But whatever. I mean - hopefully there aren't any conflicts in the ~50 > > networking files you touch. I just wish that people didn't pipe up with > > the tree wide changes right after the merge window. Feels like the > > worst possible timing. > > Oh, if the timing is what makes this especially worrisome, I have > no qualms about rebasing much later, and reposting this series then. > I'll do that. Cool, thanks! I promise to not be grumpy if you repost around rc6 :)
On Fri, Oct 21, 2022 at 09:43:58PM -0400, Jason A. Donenfeld wrote: > Hey everyone, > > Here's the second and final tranche of tree-wide conversions to get > random integer handling a bit tamer. It's predominantly another > Coccinelle-based patchset. > > First we s/prandom_u32_max/get_random_u32_below/, since the former is > just a deprecated alias for the latter. Then in the next commit we can > remove prandom_u32_max all together. I'm quite happy about finally being > able to do that. It means that prandom.h is now only for deterministic and > repeatable randomness, not non-deterministic/cryptographic randomness. > That line is no longer blurred. > > Then, in order to clean up a bunch of inefficient patterns, we introduce > two trivial static inline helper functions built on top of > get_random_u32_below: get_random_u32_above and get_random_u32_between. > These are pretty straight forward to use and understand. Then the final > two patches convert some gnarly open-coded number juggling to use these > helpers. > > I've used Coccinelle for all the treewide patches, so hopefully review > is rather uneventful. I didn't accept all of the changes that Coccinelle > proposed, though, as these tend to be somewhat context-specific. I erred > on the side of just going with the most obvious cases, at least this > time through. And then we can address more complicated cases through > actual maintainer trees. > > Since get_random_u32_below() sits in my random.git tree, these patches > too will flow through that same tree. > > Regards, > Jason Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
On Fri, Oct 21, 2022 at 11:03:22PM -0700, Jakub Kicinski wrote: > On Sat, 22 Oct 2022 07:47:06 +0200 Jason A. Donenfeld wrote: > > On Fri, Oct 21, 2022 at 10:32:42PM -0700, Jakub Kicinski wrote: > > > But whatever. I mean - hopefully there aren't any conflicts in the ~50 > > > networking files you touch. I just wish that people didn't pipe up with > > > the tree wide changes right after the merge window. Feels like the > > > worst possible timing. > > > > Oh, if the timing is what makes this especially worrisome, I have > > no qualms about rebasing much later, and reposting this series then. > > I'll do that. > > Cool, thanks! I promise to not be grumpy if you repost around rc6 :) One way of making things less painful for the stable branch and for the upstream branch is to *add* new helpers instead of playing replacement games like s/prandom_u32_max/get_random_u32_below/. This is what causes the patch conflict problems. One advantage of at least adding the new functions to the stable branches, even if we don't do the wholesale replacement, is that it makes it much less likely that a backported patch, which uses the new API, won't fail to compile --- and of course, the major headache case is one where it's not noticed at first because it didn't get picked up in people's test compiles until after the Linux x.y.z release has been pushed out. Whether it's worth doing the wholesale replacement is a different question, but separating "add the new functions with one or two use cases so the functions are actulaly _used_" from the "convert the world to use the new functions" from the "remove the old functions", might life easier. - Ted
On Fri, Oct 21, 2022 at 09:43:58PM -0400, Jason A. Donenfeld wrote: > Hey everyone, > > Here's the second and final tranche of tree-wide conversions to get > random integer handling a bit tamer. It's predominantly another > Coccinelle-based patchset. > > First we s/prandom_u32_max/get_random_u32_below/, since the former is > just a deprecated alias for the latter. Then in the next commit we can > remove prandom_u32_max all together. I'm quite happy about finally being > able to do that. It means that prandom.h is now only for deterministic and > repeatable randomness, not non-deterministic/cryptographic randomness. > That line is no longer blurred. > > Then, in order to clean up a bunch of inefficient patterns, we introduce > two trivial static inline helper functions built on top of > get_random_u32_below: get_random_u32_above and get_random_u32_between. > These are pretty straight forward to use and understand. Then the final > two patches convert some gnarly open-coded number juggling to use these > helpers. > > I've used Coccinelle for all the treewide patches, so hopefully review > is rather uneventful. I didn't accept all of the changes that Coccinelle > proposed, though, as these tend to be somewhat context-specific. I erred > on the side of just going with the most obvious cases, at least this > time through. And then we can address more complicated cases through > actual maintainer trees. > > Since get_random_u32_below() sits in my random.git tree, these patches > too will flow through that same tree. > For drivers/infiniband Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> Jason
On Sun, Oct 23, 2022 at 05:07:13PM -0400, Theodore Ts'o wrote: > On Fri, Oct 21, 2022 at 11:03:22PM -0700, Jakub Kicinski wrote: > > On Sat, 22 Oct 2022 07:47:06 +0200 Jason A. Donenfeld wrote: > > > On Fri, Oct 21, 2022 at 10:32:42PM -0700, Jakub Kicinski wrote: > > > > But whatever. I mean - hopefully there aren't any conflicts in the ~50 > > > > networking files you touch. I just wish that people didn't pipe up with > > > > the tree wide changes right after the merge window. Feels like the > > > > worst possible timing. > > > > > > Oh, if the timing is what makes this especially worrisome, I have > > > no qualms about rebasing much later, and reposting this series then. > > > I'll do that. > > > > Cool, thanks! I promise to not be grumpy if you repost around rc6 :) > > One way of making things less painful for the stable branch and for > the upstream branch is to *add* new helpers instead of playing > replacement games like s/prandom_u32_max/get_random_u32_below/. This > is what causes the patch conflict problems. > > One advantage of at least adding the new functions to the stable > branches, even if we don't do the wholesale replacement, is that it That's a good idea. I'll also save the removal commit, anyhow, for a separate thing at the end of 6.2 rc1, so that -next doesn't have issues either. That's how things wound up going down for the first tranche of these, and that worked well. Jason
Hey everyone, Here's the second and final tranche of tree-wide conversions to get random integer handling a bit tamer. It's predominantly another Coccinelle-based patchset. First we s/prandom_u32_max/get_random_u32_below/, since the former is just a deprecated alias for the latter. Then in the next commit we can remove prandom_u32_max all together. I'm quite happy about finally being able to do that. It means that prandom.h is now only for deterministic and repeatable randomness, not non-deterministic/cryptographic randomness. That line is no longer blurred. Then, in order to clean up a bunch of inefficient patterns, we introduce two trivial static inline helper functions built on top of get_random_u32_below: get_random_u32_above and get_random_u32_between. These are pretty straight forward to use and understand. Then the final two patches convert some gnarly open-coded number juggling to use these helpers. I've used Coccinelle for all the treewide patches, so hopefully review is rather uneventful. I didn't accept all of the changes that Coccinelle proposed, though, as these tend to be somewhat context-specific. I erred on the side of just going with the most obvious cases, at least this time through. And then we can address more complicated cases through actual maintainer trees. Since get_random_u32_below() sits in my random.git tree, these patches too will flow through that same tree. Regards, Jason Cc: Kees Cook <keescook@chromium.org> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Jakub Kicinski <kuba@kernel.org> Cc: Russell King <linux@armlinux.org.uk> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de> Cc: Heiko Carstens <hca@linux.ibm.com> Cc: Herbert Xu <herbert@gondor.apana.org.au> Cc: Christoph Böhmwalder <christoph.boehmwalder@linbit.com> Cc: Jani Nikula <jani.nikula@linux.intel.com> Cc: Jason Gunthorpe <jgg@nvidia.com> Cc: Sakari Ailus <sakari.ailus@linux.intel.com> Cc: Martin K. Petersen <martin.petersen@oracle.com> Cc: Theodore Ts'o <tytso@mit.edu> Cc: Andreas Dilger <adilger.kernel@dilger.ca> Cc: Jaegeuk Kim <jaegeuk@kernel.org> Cc: Richard Weinberger <richard@nod.at> Cc: Darrick J. Wong <djwong@kernel.org> Cc: SeongJae Park <sj@kernel.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Michael Ellerman <mpe@ellerman.id.au> Cc: Helge Deller <deller@gmx.de> Cc: netdev@vger.kernel.org Cc: linux-crypto@vger.kernel.org Cc: linux-block@vger.kernel.org Cc: linux-fsdevel@vger.kernel.org Cc: linux-media@vger.kernel.org Cc: linux-arm-kernel@lists.infradead.org Cc: loongarch@lists.linux.dev Cc: linux-mips@vger.kernel.org Cc: linuxppc-dev@lists.ozlabs.org Cc: linux-mmc@vger.kernel.org Cc: linux-parisc@vger.kernel.org Jason A. Donenfeld (5): treewide: use get_random_u32_below() instead of deprecated function prandom: remove prandom_u32_max() random: add helpers for random numbers with given floor or range treewide: use get_random_u32_{above,below}() instead of manual loop treewide: use get_random_u32_between() when possible arch/arm/kernel/process.c | 2 +- arch/arm64/kernel/process.c | 2 +- arch/loongarch/kernel/process.c | 2 +- arch/loongarch/kernel/vdso.c | 2 +- arch/mips/kernel/process.c | 2 +- arch/mips/kernel/vdso.c | 2 +- arch/parisc/kernel/vdso.c | 2 +- arch/powerpc/crypto/crc-vpmsum_test.c | 4 +- arch/powerpc/kernel/process.c | 2 +- arch/s390/kernel/process.c | 2 +- arch/s390/kernel/vdso.c | 2 +- arch/sparc/vdso/vma.c | 2 +- arch/um/kernel/process.c | 2 +- arch/x86/entry/vdso/vma.c | 2 +- arch/x86/kernel/module.c | 2 +- arch/x86/kernel/process.c | 2 +- arch/x86/mm/pat/cpa-test.c | 4 +- crypto/rsa-pkcs1pad.c | 2 +- crypto/testmgr.c | 86 +++++++++---------- drivers/block/drbd/drbd_receiver.c | 4 +- drivers/bus/mhi/host/internal.h | 2 +- drivers/dma-buf/st-dma-fence-chain.c | 6 +- .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 2 +- .../drm/i915/gt/intel_execlists_submission.c | 2 +- drivers/gpu/drm/i915/intel_memory_region.c | 4 +- drivers/infiniband/core/cma.c | 2 +- drivers/infiniband/hw/cxgb4/id_table.c | 4 +- drivers/infiniband/hw/hns/hns_roce_ah.c | 5 +- drivers/infiniband/ulp/rtrs/rtrs-clt.c | 2 +- drivers/md/bcache/request.c | 2 +- drivers/media/common/v4l2-tpg/v4l2-tpg-core.c | 8 +- .../media/test-drivers/vidtv/vidtv_demod.c | 8 +- .../test-drivers/vivid/vivid-kthread-cap.c | 2 +- .../test-drivers/vivid/vivid-kthread-out.c | 2 +- .../media/test-drivers/vivid/vivid-radio-rx.c | 4 +- .../media/test-drivers/vivid/vivid-sdr-cap.c | 2 +- .../test-drivers/vivid/vivid-touch-cap.c | 2 +- drivers/mmc/core/core.c | 4 +- drivers/mmc/host/dw_mmc.c | 2 +- drivers/mtd/nand/raw/nandsim.c | 4 +- drivers/mtd/tests/mtd_nandecctest.c | 10 +-- drivers/mtd/tests/stresstest.c | 8 +- drivers/mtd/ubi/debug.c | 2 +- drivers/mtd/ubi/debug.h | 6 +- drivers/net/ethernet/broadcom/cnic.c | 2 +- .../chelsio/inline_crypto/chtls/chtls_io.c | 4 +- drivers/net/phy/at803x.c | 2 +- drivers/net/team/team_mode_random.c | 2 +- drivers/net/wireguard/selftest/allowedips.c | 20 ++--- drivers/net/wireguard/timers.c | 4 +- .../broadcom/brcm80211/brcmfmac/p2p.c | 2 +- .../net/wireless/intel/iwlwifi/mvm/mac-ctxt.c | 2 +- drivers/pci/p2pdma.c | 2 +- drivers/s390/scsi/zfcp_fc.c | 2 +- drivers/scsi/fcoe/fcoe_ctlr.c | 4 +- drivers/scsi/qedi/qedi_main.c | 2 +- drivers/scsi/scsi_debug.c | 6 +- fs/ceph/inode.c | 2 +- fs/ceph/mdsmap.c | 2 +- fs/ext2/ialloc.c | 2 +- fs/ext4/ialloc.c | 2 +- fs/ext4/mmp.c | 8 +- fs/ext4/super.c | 5 +- fs/f2fs/gc.c | 2 +- fs/f2fs/segment.c | 8 +- fs/ubifs/debug.c | 8 +- fs/ubifs/lpt_commit.c | 14 +-- fs/ubifs/tnc_commit.c | 2 +- fs/xfs/libxfs/xfs_alloc.c | 2 +- fs/xfs/libxfs/xfs_ialloc.c | 2 +- fs/xfs/xfs_error.c | 2 +- include/linux/damon.h | 2 +- include/linux/nodemask.h | 2 +- include/linux/prandom.h | 6 -- include/linux/random.h | 24 ++++++ kernel/bpf/core.c | 4 +- kernel/kcsan/selftest.c | 4 +- kernel/locking/test-ww_mutex.c | 4 +- kernel/time/clocksource.c | 2 +- lib/fault-inject.c | 2 +- lib/find_bit_benchmark.c | 4 +- lib/kobject.c | 2 +- lib/reed_solomon/test_rslib.c | 6 +- lib/sbitmap.c | 4 +- lib/test-string_helpers.c | 2 +- lib/test_fprobe.c | 5 +- lib/test_hexdump.c | 10 +-- lib/test_kprobes.c | 5 +- lib/test_list_sort.c | 2 +- lib/test_printf.c | 2 +- lib/test_rhashtable.c | 4 +- lib/test_vmalloc.c | 8 +- mm/kasan/kasan_test.c | 6 +- mm/kfence/core.c | 4 +- mm/kfence/kfence_test.c | 4 +- mm/slub.c | 2 +- mm/swapfile.c | 5 +- net/802/garp.c | 2 +- net/802/mrp.c | 2 +- net/batman-adv/bat_iv_ogm.c | 4 +- net/batman-adv/bat_v_elp.c | 2 +- net/batman-adv/bat_v_ogm.c | 4 +- net/batman-adv/network-coding.c | 2 +- net/bluetooth/mgmt.c | 5 +- net/can/j1939/socket.c | 2 +- net/can/j1939/transport.c | 2 +- net/ceph/mon_client.c | 2 +- net/ceph/osd_client.c | 2 +- net/core/neighbour.c | 4 +- net/core/pktgen.c | 37 ++++---- net/core/stream.c | 2 +- net/ipv4/icmp.c | 2 +- net/ipv4/igmp.c | 6 +- net/ipv4/inet_connection_sock.c | 2 +- net/ipv4/inet_hashtables.c | 2 +- net/ipv4/route.c | 4 +- net/ipv4/tcp_bbr.c | 2 +- net/ipv4/tcp_input.c | 3 +- net/ipv6/addrconf.c | 8 +- net/ipv6/mcast.c | 10 +-- net/ipv6/output_core.c | 8 +- net/ipv6/route.c | 2 +- net/netfilter/ipvs/ip_vs_twos.c | 4 +- net/netfilter/nf_conntrack_core.c | 4 +- net/netfilter/nf_nat_helper.c | 2 +- net/netlink/af_netlink.c | 2 +- net/packet/af_packet.c | 4 +- net/sched/act_gact.c | 2 +- net/sched/act_sample.c | 2 +- net/sched/sch_choke.c | 2 +- net/sched/sch_netem.c | 4 +- net/sctp/socket.c | 2 +- net/sctp/transport.c | 2 +- net/sunrpc/cache.c | 2 +- net/sunrpc/xprtsock.c | 2 +- net/tipc/socket.c | 2 +- net/vmw_vsock/af_vsock.c | 3 +- net/xfrm/xfrm_state.c | 2 +- 138 files changed, 309 insertions(+), 318 deletions(-)