| Message ID | 20240824074033.2134514-8-lihongbo22@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Use max/min to simplify the code | expand |
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? 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, 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 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)
diff --git a/net/rxrpc/input.c b/net/rxrpc/input.c index 16d49a861dbb..455aa0189b28 100644 --- a/net/rxrpc/input.c +++ b/net/rxrpc/input.c @@ -868,8 +868,7 @@ static void rxrpc_input_ack(struct rxrpc_call *call, struct sk_buff *skb) prev_pkt = sp->ack.prev_ack; nr_acks = sp->ack.nr_acks; hard_ack = first_soft_ack - 1; - 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); trace_rxrpc_rx_ack(call, ack_serial, acked_serial, first_soft_ack, prev_pkt,
Let's use min() to simplify the code and fix the Coccinelle/coccicheck warning reported by minmax.cocci. Signed-off-by: Hongbo Li <lihongbo22@huawei.com> --- net/rxrpc/input.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)