Re: [ovs-dev] [PATCH v1 3/5] treewide: use get_random_u32() when possible
On Wed, 2022-10-12 at 21:29 +, David Laight wrote: > From: Joe Perches > > Sent: 12 October 2022 20:17 > > > > On Wed, 2022-10-05 at 23:48 +0200, Jason A. Donenfeld wrote: > > > The prandom_u32() function has been a deprecated inline wrapper around > > > get_random_u32() for several releases now, and compiles down to the > > > exact same code. Replace the deprecated wrapper with a direct call to > > > the real function. > > [] > > > diff --git a/drivers/infiniband/hw/cxgb4/cm.c > > > b/drivers/infiniband/hw/cxgb4/cm.c > > [] > > > @@ -734,7 +734,7 @@ static int send_connect(struct c4iw_ep *ep) > > > &ep->com.remote_addr; > > > int ret; > > > enum chip_type adapter_type = ep->com.dev->rdev.lldi.adapter_type; > > > - u32 isn = (prandom_u32() & ~7UL) - 1; > > > + u32 isn = (get_random_u32() & ~7UL) - 1; > > > > trivia: > > > > There are somewhat odd size mismatches here. > > > > I had to think a tiny bit if random() returned a value from 0 to 7 > > and was promoted to a 64 bit value then truncated to 32 bit. > > > > Perhaps these would be clearer as ~7U and not ~7UL > > That makes no difference - the compiler will generate the same code. True, more or less. It's more a question for the reader. > The real question is WTF is the code doing? True. > The '& ~7u' clears the bottom 3 bits. > The '- 1' then sets the bottom 3 bits and decrements the > (random) high bits. Right. > So is the same as get_random_u32() | 7. True, it's effectively the same as the upper 29 bits are random anyway and the bottom 3 bits are always set. > But I bet the coder had something else in mind. Likely. And it was also likely copy/pasted a few times. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v1 3/5] treewide: use get_random_u32() when possible
From: Joe Perches > Sent: 12 October 2022 20:17 > > On Wed, 2022-10-05 at 23:48 +0200, Jason A. Donenfeld wrote: > > The prandom_u32() function has been a deprecated inline wrapper around > > get_random_u32() for several releases now, and compiles down to the > > exact same code. Replace the deprecated wrapper with a direct call to > > the real function. > [] > > diff --git a/drivers/infiniband/hw/cxgb4/cm.c > > b/drivers/infiniband/hw/cxgb4/cm.c > [] > > @@ -734,7 +734,7 @@ static int send_connect(struct c4iw_ep *ep) > >&ep->com.remote_addr; > > int ret; > > enum chip_type adapter_type = ep->com.dev->rdev.lldi.adapter_type; > > - u32 isn = (prandom_u32() & ~7UL) - 1; > > + u32 isn = (get_random_u32() & ~7UL) - 1; > > trivia: > > There are somewhat odd size mismatches here. > > I had to think a tiny bit if random() returned a value from 0 to 7 > and was promoted to a 64 bit value then truncated to 32 bit. > > Perhaps these would be clearer as ~7U and not ~7UL That makes no difference - the compiler will generate the same code. The real question is WTF is the code doing? The '& ~7u' clears the bottom 3 bits. The '- 1' then sets the bottom 3 bits and decrements the (random) high bits. So is the same as get_random_u32() | 7. But I bet the coder had something else in mind. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales) ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v1 3/5] treewide: use get_random_u32() when possible
On Wed, 2022-10-05 at 23:48 +0200, Jason A. Donenfeld wrote: > The prandom_u32() function has been a deprecated inline wrapper around > get_random_u32() for several releases now, and compiles down to the > exact same code. Replace the deprecated wrapper with a direct call to > the real function. [] > diff --git a/drivers/infiniband/hw/cxgb4/cm.c > b/drivers/infiniband/hw/cxgb4/cm.c [] > @@ -734,7 +734,7 @@ static int send_connect(struct c4iw_ep *ep) > &ep->com.remote_addr; > int ret; > enum chip_type adapter_type = ep->com.dev->rdev.lldi.adapter_type; > - u32 isn = (prandom_u32() & ~7UL) - 1; > + u32 isn = (get_random_u32() & ~7UL) - 1; trivia: There are somewhat odd size mismatches here. I had to think a tiny bit if random() returned a value from 0 to 7 and was promoted to a 64 bit value then truncated to 32 bit. Perhaps these would be clearer as ~7U and not ~7UL > struct net_device *netdev; > u64 params; > > @@ -2469,7 +2469,7 @@ static int accept_cr(struct c4iw_ep *ep, struct sk_buff > *skb, > } > > if (!is_t4(adapter_type)) { > - u32 isn = (prandom_u32() & ~7UL) - 1; > + u32 isn = (get_random_u32() & ~7UL) - 1; etc... drivers/infiniband/hw/cxgb4/cm.c: u32 isn = (prandom_u32() & ~7UL) - 1; drivers/infiniband/hw/cxgb4/cm.c: u32 isn = (prandom_u32() & ~7UL) - 1; drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c:rpl5->iss = cpu_to_be32((prandom_u32() & ~7UL) - 1); drivers/scsi/cxgbi/cxgb4i/cxgb4i.c: u32 isn = (prandom_u32() & ~7UL) - 1; drivers/scsi/cxgbi/cxgb4i/cxgb4i.c: u32 isn = (prandom_u32() & ~7UL) - 1; drivers/target/iscsi/cxgbit/cxgbit_cm.c:rpl5->iss = cpu_to_be32((prandom_u32() & ~7UL) - 1); ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v1 3/5] treewide: use get_random_u32() when possible
On Thu, Oct 06, 2022 at 07:05:48AM -0600, Jason A. Donenfeld wrote: > > > diff --git a/drivers/infiniband/ulp/ipoib/ipoib_cm.c > > > b/drivers/infiniband/ulp/ipoib/ipoib_cm.c > > > index fd9d7f2c4d64..a605cf66b83e 100644 > > > --- a/drivers/infiniband/ulp/ipoib/ipoib_cm.c > > > +++ b/drivers/infiniband/ulp/ipoib/ipoib_cm.c > > > @@ -465,7 +465,7 @@ static int ipoib_cm_req_handler(struct ib_cm_id > > > *cm_id, > > > goto err_qp; > > > } > > > > > > - psn = prandom_u32() & 0xff; > > > + psn = get_random_u32() & 0xff; > > > > prandom_max(0xff + 1) > > That'd work, but again it's not more clear. Authors here are going for > a 24-bit number, and masking seems like a clear way to express that. vs just asking directly for a 24 bit number? Jason ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v1 3/5] treewide: use get_random_u32() when possible
On Wed, Oct 05, 2022 at 11:48:42PM +0200, Jason A. Donenfeld wrote: > index 14392c942f49..499a425a3379 100644 > --- a/drivers/infiniband/hw/cxgb4/cm.c > +++ b/drivers/infiniband/hw/cxgb4/cm.c > @@ -734,7 +734,7 @@ static int send_connect(struct c4iw_ep *ep) > &ep->com.remote_addr; > int ret; > enum chip_type adapter_type = ep->com.dev->rdev.lldi.adapter_type; > - u32 isn = (prandom_u32() & ~7UL) - 1; > + u32 isn = (get_random_u32() & ~7UL) - 1; Maybe this wants to be written as (prandom_max(U32_MAX >> 7) << 7) | 7 ? > diff --git a/drivers/infiniband/ulp/ipoib/ipoib_cm.c > b/drivers/infiniband/ulp/ipoib/ipoib_cm.c > index fd9d7f2c4d64..a605cf66b83e 100644 > --- a/drivers/infiniband/ulp/ipoib/ipoib_cm.c > +++ b/drivers/infiniband/ulp/ipoib/ipoib_cm.c > @@ -465,7 +465,7 @@ static int ipoib_cm_req_handler(struct ib_cm_id *cm_id, > goto err_qp; > } > > - psn = prandom_u32() & 0xff; > + psn = get_random_u32() & 0xff; prandom_max(0xff + 1) ? Jason ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v1 3/5] treewide: use get_random_u32() when possible
On Wed 05-10-22 23:48:42, Jason A. Donenfeld wrote: > The prandom_u32() function has been a deprecated inline wrapper around > get_random_u32() for several releases now, and compiles down to the > exact same code. Replace the deprecated wrapper with a direct call to > the real function. > > Signed-off-by: Jason A. Donenfeld ... > diff --git a/fs/ext2/ialloc.c b/fs/ext2/ialloc.c > index 998dd2ac8008..e439a872c398 100644 > --- a/fs/ext2/ialloc.c > +++ b/fs/ext2/ialloc.c > @@ -277,7 +277,7 @@ static int find_group_orlov(struct super_block *sb, > struct inode *parent) > int best_ndir = inodes_per_group; > int best_group = -1; > > - group = prandom_u32(); > + group = get_random_u32(); > parent_group = (unsigned)group % ngroups; > for (i = 0; i < ngroups; i++) { > group = (parent_group + i) % ngroups; The code here is effectively doing the parent_group = prandom_u32_max(ngroups); > diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c > index f73e5eb43eae..954ec9736a8d 100644 > --- a/fs/ext4/ialloc.c > +++ b/fs/ext4/ialloc.c > @@ -465,7 +465,7 @@ static int find_group_orlov(struct super_block *sb, > struct inode *parent, > ext4fs_dirhash(parent, qstr->name, qstr->len, &hinfo); > grp = hinfo.hash; > } else > - grp = prandom_u32(); > + grp = get_random_u32(); Similarly here we can use prandom_u32_max(ngroups) like: if (qstr) { ... parent_group = hinfo.hash % ngroups; } else parent_group = prandom_u32_max(ngroups); > diff --git a/fs/ext4/mmp.c b/fs/ext4/mmp.c > index 9af68a7ecdcf..588cb09c5291 100644 > --- a/fs/ext4/mmp.c > +++ b/fs/ext4/mmp.c > @@ -265,7 +265,7 @@ static unsigned int mmp_new_seq(void) > u32 new_seq; > > do { > - new_seq = prandom_u32(); > + new_seq = get_random_u32(); > } while (new_seq > EXT4_MMP_SEQ_MAX); OK, here we again effectively implement prandom_u32_max(EXT4_MMP_SEQ_MAX + 1). Just presumably we didn't want to use modulo here because EXT4_MMP_SEQ_MAX is rather big and so the resulting 'new_seq' would be seriously non-uniform. Honza -- Jan Kara SUSE Labs, CR ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v1 3/5] treewide: use get_random_u32() when possible
On Thu, Oct 06, 2022 at 07:05:48AM -0600, Jason A. Donenfeld wrote: > On Thu, Oct 6, 2022 at 6:47 AM Jason Gunthorpe wrote: > > On Wed, Oct 05, 2022 at 11:48:42PM +0200, Jason A. Donenfeld wrote: ... > > > - u32 isn = (prandom_u32() & ~7UL) - 1; > > > + u32 isn = (get_random_u32() & ~7UL) - 1; > > > > Maybe this wants to be written as > > > > (prandom_max(U32_MAX >> 7) << 7) | 7 > > ? > > Holy smokes. Yea I guess maybe? It doesn't exactly gain anything or > make the code clearer though, and is a little bit more magical than > I'd like on a first pass. Shouldn't the two first 7s to be 3s? ... > > > - psn = prandom_u32() & 0xff; > > > + psn = get_random_u32() & 0xff; > > > > prandom_max(0xff + 1) > > That'd work, but again it's not more clear. Authors here are going for > a 24-bit number, and masking seems like a clear way to express that. We have some 24-bit APIs (and 48-bit) already in kernel, why not to have get_random_u24() ? -- With Best Regards, Andy Shevchenko ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v1 3/5] treewide: use get_random_u32() when possible
On Thu, Oct 6, 2022 at 6:47 AM Jason Gunthorpe wrote: > > On Wed, Oct 05, 2022 at 11:48:42PM +0200, Jason A. Donenfeld wrote: > > > index 14392c942f49..499a425a3379 100644 > > --- a/drivers/infiniband/hw/cxgb4/cm.c > > +++ b/drivers/infiniband/hw/cxgb4/cm.c > > @@ -734,7 +734,7 @@ static int send_connect(struct c4iw_ep *ep) > > &ep->com.remote_addr; > > int ret; > > enum chip_type adapter_type = ep->com.dev->rdev.lldi.adapter_type; > > - u32 isn = (prandom_u32() & ~7UL) - 1; > > + u32 isn = (get_random_u32() & ~7UL) - 1; > > Maybe this wants to be written as > > (prandom_max(U32_MAX >> 7) << 7) | 7 > > ? Holy smokes. Yea I guess maybe? It doesn't exactly gain anything or make the code clearer though, and is a little bit more magical than I'd like on a first pass. > > > diff --git a/drivers/infiniband/ulp/ipoib/ipoib_cm.c > > b/drivers/infiniband/ulp/ipoib/ipoib_cm.c > > index fd9d7f2c4d64..a605cf66b83e 100644 > > --- a/drivers/infiniband/ulp/ipoib/ipoib_cm.c > > +++ b/drivers/infiniband/ulp/ipoib/ipoib_cm.c > > @@ -465,7 +465,7 @@ static int ipoib_cm_req_handler(struct ib_cm_id *cm_id, > > goto err_qp; > > } > > > > - psn = prandom_u32() & 0xff; > > + psn = get_random_u32() & 0xff; > > prandom_max(0xff + 1) That'd work, but again it's not more clear. Authors here are going for a 24-bit number, and masking seems like a clear way to express that. Jason ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v1 3/5] treewide: use get_random_u32() when possible
The prandom_u32() function has been a deprecated inline wrapper around get_random_u32() for several releases now, and compiles down to the exact same code. Replace the deprecated wrapper with a direct call to the real function. Signed-off-by: Jason A. Donenfeld --- Documentation/networking/filter.rst| 2 +- drivers/infiniband/hw/cxgb4/cm.c | 4 ++-- drivers/infiniband/hw/hfi1/tid_rdma.c | 2 +- drivers/infiniband/hw/mlx4/mad.c | 2 +- drivers/infiniband/ulp/ipoib/ipoib_cm.c| 2 +- drivers/md/raid5-cache.c | 2 +- drivers/mtd/nand/raw/nandsim.c | 2 +- drivers/net/bonding/bond_main.c| 2 +- drivers/net/ethernet/broadcom/cnic.c | 2 +- .../chelsio/inline_crypto/chtls/chtls_cm.c | 2 +- drivers/net/ethernet/rocker/rocker_main.c | 6 +++--- .../net/wireless/marvell/mwifiex/cfg80211.c| 4 ++-- .../net/wireless/microchip/wilc1000/cfg80211.c | 2 +- .../net/wireless/quantenna/qtnfmac/cfg80211.c | 2 +- drivers/nvme/common/auth.c | 2 +- drivers/scsi/cxgbi/cxgb4i/cxgb4i.c | 4 ++-- drivers/target/iscsi/cxgbit/cxgbit_cm.c| 2 +- drivers/thunderbolt/xdomain.c | 2 +- drivers/video/fbdev/uvesafb.c | 2 +- fs/exfat/inode.c | 2 +- fs/ext2/ialloc.c | 2 +- fs/ext4/ialloc.c | 4 ++-- fs/ext4/ioctl.c| 4 ++-- fs/ext4/mmp.c | 2 +- fs/f2fs/namei.c| 2 +- fs/fat/inode.c | 2 +- fs/nfsd/nfs4state.c| 4 ++-- fs/ubifs/journal.c | 2 +- fs/xfs/libxfs/xfs_ialloc.c | 2 +- fs/xfs/xfs_icache.c| 2 +- fs/xfs/xfs_log.c | 2 +- include/net/netfilter/nf_queue.h | 2 +- include/net/red.h | 2 +- include/net/sock.h | 2 +- kernel/kcsan/selftest.c| 2 +- lib/random32.c | 2 +- lib/reed_solomon/test_rslib.c | 6 +++--- lib/test_fprobe.c | 2 +- lib/test_kprobes.c | 2 +- lib/test_rhashtable.c | 6 +++--- mm/shmem.c | 2 +- net/802/garp.c | 2 +- net/802/mrp.c | 2 +- net/core/pktgen.c | 4 ++-- net/ipv4/tcp_cdg.c | 2 +- net/ipv4/udp.c | 2 +- net/ipv6/ip6_flowlabel.c | 2 +- net/ipv6/output_core.c | 2 +- net/netfilter/ipvs/ip_vs_conn.c| 2 +- net/netfilter/xt_statistic.c | 2 +- net/openvswitch/actions.c | 2 +- net/rds/bind.c | 2 +- net/sched/sch_cake.c | 2 +- net/sched/sch_netem.c | 18 +- net/sunrpc/auth_gss/gss_krb5_wrap.c| 4 ++-- net/sunrpc/xprt.c | 2 +- net/unix/af_unix.c | 2 +- 57 files changed, 79 insertions(+), 79 deletions(-) diff --git a/Documentation/networking/filter.rst b/Documentation/networking/filter.rst index 43cdc4d34745..f69da5074860 100644 --- a/Documentation/networking/filter.rst +++ b/Documentation/networking/filter.rst @@ -305,7 +305,7 @@ Possible BPF extensions are shown in the following table: vlan_tci skb_vlan_tag_get(skb) vlan_availskb_vlan_tag_present(skb) vlan_tpid skb->vlan_proto - rand prandom_u32() + rand get_random_u32() === = These extensions can also be prefixed with '#'. diff --git a/drivers/infiniband/hw/cxgb4/cm.c b/drivers/infiniband/hw/cxgb4/cm.c index 14392c942f49..499a425a3379 100644 --- a/drivers/infiniband/hw/cxgb4/cm.c +++ b/drivers/infiniband/hw/cxgb4/cm.c @@ -734,7 +734,7 @@ static int send_connect(struct c4iw_ep *ep) &ep->com.remote_addr; int ret; enum chip_type adapter_type = ep->com.dev->rdev.lldi.adapter_type; - u32 isn = (prandom_u32() & ~7UL) - 1; + u32 isn = (get_random_u32() & ~7UL) - 1; struct net_device *netdev; u64 params; @@ -2469,7 +2469,7 @@ static int accept_cr(struct c4iw_ep *ep, struct sk_buff *skb, } if (!is_t4(adapter_type)) { -