| Message ID | 20240824074033.2134514-1-lihongbo22@huawei.com |
|---|---|
| Headers | show |
| Series | Use max/min to simplify the code | expand |
On 2024/8/24 20:06, David Howells wrote: > Hongbo Li <lihongbo22@huawei.com> wrote: > >> - summary.ack_reason = (sp->ack.reason < RXRPC_ACK__INVALID ? >> - sp->ack.reason : RXRPC_ACK__INVALID); >> + summary.ack_reason = min(sp->ack.reason, RXRPC_ACK__INVALID); > > Can you use umin() rather than min(), please? > I see reason is u8, may I use min_t(u8, sp->ack.reason, RXRPC_ACK__INVALID); Thanks, Hongbo > Thanks, > David > >
On 2024/8/24 20:06, David Howells wrote: > Hongbo Li <lihongbo22@huawei.com> wrote: > >> - summary.ack_reason = (sp->ack.reason < RXRPC_ACK__INVALID ? >> - sp->ack.reason : RXRPC_ACK__INVALID); >> + summary.ack_reason = min(sp->ack.reason, RXRPC_ACK__INVALID); > > Can you use umin() rather than min(), please? > I see reason is u8, so may I use min_t(u8, sp->ack.reason, RXRPC_ACK__INVALID)? Thanks, Hongbo > Thanks, > David > >
On 24 Aug 2024, at 9:40, Hongbo Li wrote: > Let's use max() to simplify the code and fix the > Coccinelle/coccicheck warning reported by minmax.cocci. > > Signed-off-by: Hongbo Li <lihongbo22@huawei.com> The change looks good to me. Acked-by: Eelco Chaudron <echaudro@redhat.com> > --- > net/openvswitch/vport.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/openvswitch/vport.c b/net/openvswitch/vport.c > index 8732f6e51ae5..859208df65ea 100644 > --- a/net/openvswitch/vport.c > +++ b/net/openvswitch/vport.c > @@ -534,7 +534,7 @@ static int packet_length(const struct sk_buff *skb, > * account for 802.1ad. e.g. is_skb_forwardable(). > */ > > - return length > 0 ? length : 0; > + return max(length, 0); > } > > void ovs_vport_send(struct vport *vport, struct sk_buff *skb, u8 mac_proto) > -- > 2.34.1
Hongbo Li via dev <ovs-dev@openvswitch.org> writes: > Let's use max() to simplify the code and fix the > Coccinelle/coccicheck warning reported by minmax.cocci. > > Signed-off-by: Hongbo Li <lihongbo22@huawei.com> > --- Reviewed-by: Aaron Conole <aconole@redhat.com> > net/openvswitch/vport.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/openvswitch/vport.c b/net/openvswitch/vport.c > index 8732f6e51ae5..859208df65ea 100644 > --- a/net/openvswitch/vport.c > +++ b/net/openvswitch/vport.c > @@ -534,7 +534,7 @@ static int packet_length(const struct sk_buff *skb, > * account for 802.1ad. e.g. is_skb_forwardable(). > */ > > - return length > 0 ? length : 0; > + return max(length, 0); > } > > void ovs_vport_send(struct vport *vport, struct sk_buff *skb, u8 mac_proto)
Hongbo Li <lihongbo22@huawei.com> writes: > The following Coccinelle/coccicheck warning reported by > minmax.cocci: > WARNING opportunity for max() > Let's use max() to simplify the code and fix the warning. > > Signed-off-by: Hongbo Li <lihongbo22@huawei.com> > --- > net/mac80211/driver-ops.h | 2 +- > net/mac80211/mlme.c | 2 +- > net/mac80211/scan.c | 6 ++---- > net/mac80211/tdls.c | 2 +- > 4 files changed, 5 insertions(+), 7 deletions(-) mac80211 patches go to wireless-next, please submit this separately. And the title should begin with 'wifi: mac80211:'.
On Sat, 24 Aug 2024 15:40:25 +0800 Hongbo Li wrote: > Many Coccinelle/coccicheck warning reported by minmax.cocci > in net module, such as: > WARNING opportunity for max() > WARNING opportunity for min() > > Let's use max/min to simplify the code and fix these warnings. > These patch have passed compilation test. This set does not build.
On 2024/8/27 5:44, Jakub Kicinski wrote: > On Sat, 24 Aug 2024 15:40:25 +0800 Hongbo Li wrote: >> Many Coccinelle/coccicheck warning reported by minmax.cocci >> in net module, such as: >> WARNING opportunity for max() >> WARNING opportunity for min() >> >> Let's use max/min to simplify the code and fix these warnings. >> These patch have passed compilation test. > > This set does not build. > Do you mean some patches will go to other branches (such as mac80211)? Thanks, Hongbo >
Hongbo Li <lihongbo22@huawei.com> writes: > On 2024/8/27 5:44, Jakub Kicinski wrote: >> On Sat, 24 Aug 2024 15:40:25 +0800 Hongbo Li wrote: >>> Many Coccinelle/coccicheck warning reported by minmax.cocci >>> in net module, such as: >>> WARNING opportunity for max() >>> WARNING opportunity for min() >>> >>> Let's use max/min to simplify the code and fix these warnings. >>> These patch have passed compilation test. >> This set does not build. >> > Do you mean some patches will go to other branches (such as mac80211)? Jakub means that your patchset had compilation errors, see the red on patchwork: https://patchwork.kernel.org/project/netdevbpf/list/?series=882901&state=*&order=date
On Tue, 27 Aug 2024 07:45:02 +0300 Kalle Valo wrote: > > Do you mean some patches will go to other branches (such as mac80211)? > > Jakub means that your patchset had compilation errors, see the red on > patchwork: > > https://patchwork.kernel.org/project/netdevbpf/list/?series=882901&state=*&order=date FWIW I prefer not to point noobs to the patchwork checks, lest they think it's a public CI and they can fling broken code at the list :( But yes, in case "code doesn't build" needs a further explanation: net/core/pktgen.c: In function ‘pktgen_finalize_skb’: ./../include/linux/compiler_types.h:510:45: error: call to ‘__compiletime_assert_928’ declared with attribute error: min(datalen/frags, ((1UL) << 12)) signedness error 510 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__) | ^ ./../include/linux/compiler_types.h:491:25: note: in definition of macro ‘__compiletime_assert’ 491 | prefix ## suffix(); \ | ^~~~~~ ./../include/linux/compiler_types.h:510:9: note: in expansion of macro ‘_compiletime_assert’ 510 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__) | ^~~~~~~~~~~~~~~~~~~ ../include/linux/build_bug.h:39:37: note: in expansion of macro ‘compiletime_assert’ 39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg) | ^~~~~~~~~~~~~~~~~~ ../include/linux/minmax.h:100:9: note: in expansion of macro ‘BUILD_BUG_ON_MSG’ 100 | BUILD_BUG_ON_MSG(!__types_ok(x,y,ux,uy), \ | ^~~~~~~~~~~~~~~~ ../include/linux/minmax.h:105:9: note: in expansion of macro ‘__careful_cmp_once’ 105 | __careful_cmp_once(op, x, y, __UNIQUE_ID(x_), __UNIQUE_ID(y_)) | ^~~~~~~~~~~~~~~~~~ ../include/linux/minmax.h:129:25: note: in expansion of macro ‘__careful_cmp’ 129 | #define min(x, y) __careful_cmp(min, x, y) | ^~~~~~~~~~~~~ ../net/core/pktgen.c:2796:28: note: in expansion of macro ‘min’ 2796 | frag_len = min(datalen/frags, PAGE_SIZE); | ^~~ make[5]: *** [../scripts/Makefile.build:244: net/core/pktgen.o] Error 1
Jakub Kicinski <kuba@kernel.org> writes: > On Tue, 27 Aug 2024 07:45:02 +0300 Kalle Valo wrote: >> > Do you mean some patches will go to other branches (such as mac80211)? >> >> Jakub means that your patchset had compilation errors, see the red on >> patchwork: >> >> https://patchwork.kernel.org/project/netdevbpf/list/?series=882901&state=*&order=date > > FWIW I prefer not to point noobs to the patchwork checks, lest they > think it's a public CI and they can fling broken code at the list :( Good point, that's definitely what we do not want. I'll keep this in mind.
On Mon, Aug 26, 2024 at 10:50:03AM +0800, Hongbo Li wrote: > > > On 2024/8/24 20:06, David Howells wrote: > > Hongbo Li <lihongbo22@huawei.com> wrote: > > > > > - summary.ack_reason = (sp->ack.reason < RXRPC_ACK__INVALID ? > > > - sp->ack.reason : RXRPC_ACK__INVALID); > > > + summary.ack_reason = min(sp->ack.reason, RXRPC_ACK__INVALID); > > > > Can you use umin() rather than min(), please? > > > > I see reason is u8, so may I use min_t(u8, sp->ack.reason, > RXRPC_ACK__INVALID)? I believe that umin was added precisely to avoid such constructions. See: 80fcac55385c ("minmax: add umin(a, b) and umax(a, b)") https://git.kernel.org/torvalds/c/80fcac55385c
Hongbo Li <lihongbo22@huawei.com> wrote: > I see reason is u8, so may I use min_t(u8, sp->ack.reason, > RXRPC_ACK__INVALID)? No, please don't use min_t(<unsigned type>, ...) if umin() will do. IIRC, some arches can't do byte-level arithmetic. Thanks, David
From: David Howells <dhowells@redhat.com> > Sent: 28 August 2024 09:18 > > Hongbo Li <lihongbo22@huawei.com> wrote: > > > I see reason is u8, so may I use min_t(u8, sp->ack.reason, > > RXRPC_ACK__INVALID)? > > No, please don't use min_t(<unsigned type>, ...) if umin() will do. IIRC, > some arches can't do byte-level arithmetic. Not to mention all the places where the wrong type has been used and significant bits masked off before the comparison. Is there even a problem with min() here? It should be fine unless sp->ack.reason is a signed type. In which case things are probably horribly wrong if it is negative. Looking at the code I'm not even sure that min() is right. It really ought to be used for counters/sizes. This is a bit like the (broken) suggestion of replacing: return rval < 0 ? rval : 0; with return min(rval, 0); David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
From: Jakub Kicinski > Sent: 27 August 2024 15:04 > > On Tue, 27 Aug 2024 07:45:02 +0300 Kalle Valo wrote: > > > Do you mean some patches will go to other branches (such as mac80211)? > > > > Jakub means that your patchset had compilation errors, see the red on > > patchwork: > > > > https://patchwork.kernel.org/project/netdevbpf/list/?series=882901&state=*&order=date > > FWIW I prefer not to point noobs to the patchwork checks, lest they > think it's a public CI and they can fling broken code at the list :( > But yes, in case "code doesn't build" needs a further explanation: > > net/core/pktgen.c: In function ‘pktgen_finalize_skb’: > ./../include/linux/compiler_types.h:510:45: error: call to ‘__compiletime_assert_928’ declared with > attribute error: min(datalen/frags, ((1UL) << 12)) signedness error ... > ../net/core/pktgen.c:2796:28: note: in expansion of macro ‘min’ > 2796 | frag_len = min(datalen/frags, PAGE_SIZE); > | ^~~ I can't help feeling that a signed divide isn't intended here. Which rather implies that both datalen and frags are signed types. Whereas neither can be sensibly negative. Perhaps that is the real bug? David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Many Coccinelle/coccicheck warning reported by minmax.cocci in net module, such as: WARNING opportunity for max() WARNING opportunity for min() Let's use max/min to simplify the code and fix these warnings. These patch have passed compilation test. Hongbo Li (8): net/mac80211: use max to simplify the code net/rds: Use max() to simplify the code net/ipv4: Use min() to simplify the code net/core: Use min()/max() to simplify the code net/dccp: Use min()/max() to simplify the code net/openvswitch: Use max() to simplify the code net/rxrpc: Use min() to simplify the code net/ceph: Use min() to simplify the code net/ceph/osd_client.c | 2 +- net/core/pktgen.c | 6 ++---- net/core/sock.c | 2 +- net/dccp/ackvec.c | 2 +- net/dccp/dccp.h | 2 +- net/ipv4/ip_sockglue.c | 2 +- net/mac80211/driver-ops.h | 2 +- net/mac80211/mlme.c | 2 +- net/mac80211/scan.c | 6 ++---- net/mac80211/tdls.c | 2 +- net/openvswitch/vport.c | 2 +- net/rds/info.c | 5 +---- net/rxrpc/input.c | 3 +-- 13 files changed, 15 insertions(+), 23 deletions(-)