| Message ID | 20240824074033.2134514-3-lihongbo22@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Use max/min to simplify the code | expand |
On Sat, 2024-08-24 at 15:40 +0800, Hongbo Li wrote: > The target if-else can be replaced with max(). > > Signed-off-by: Hongbo Li <lihongbo22@huawei.com> Looks simple enough to me. Thanks Hongbo! Reviewed-by: Allison Henderson <allison.henderson@oracle.com> > --- > net/rds/info.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/net/rds/info.c b/net/rds/info.c > index b6b46a8214a0..8558b0a466b4 100644 > --- a/net/rds/info.c > +++ b/net/rds/info.c > @@ -194,10 +194,7 @@ int rds_info_getsockopt(struct socket *sock, int > optname, char __user *optval, > } > ret = pin_user_pages_fast(start, nr_pages, FOLL_WRITE, > pages); > if (ret != nr_pages) { > - if (ret > 0) > - nr_pages = ret; > - else > - nr_pages = 0; > + nr_pages = max(ret, 0); > ret = -EAGAIN; /* XXX ? */ > goto out; > }
On Sat, Aug 24, 2024 at 03:40:27PM +0800, Hongbo Li wrote: > The target if-else can be replaced with max(). > > Signed-off-by: Hongbo Li <lihongbo22@huawei.com> > --- > net/rds/info.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/net/rds/info.c b/net/rds/info.c > index b6b46a8214a0..8558b0a466b4 100644 > --- a/net/rds/info.c > +++ b/net/rds/info.c > @@ -194,10 +194,7 @@ int rds_info_getsockopt(struct socket *sock, int optname, char __user *optval, > } > ret = pin_user_pages_fast(start, nr_pages, FOLL_WRITE, pages); > if (ret != nr_pages) { > - if (ret > 0) > - nr_pages = ret; > - else > - nr_pages = 0; > + nr_pages = max(ret, 0); Along the same lines as Johannes Berg's comment on a different patch [1] I think that there is a subtle but important difference, semantically, between max() and that the existing code does, for which the best description I can think of is setting a floor on the value. Other than Johannes's comment, and now mine here, I think you will find that, if you search the netdev ML, you will find this point being made consistently, at least over the past year. And yes, we understand that mathematically max() is doing the right thing. But that is not the point that is being made here. I suggest dropping this patch. And any others like it. [1] https://lore.kernel.org/all/d5f495b67fe6bf128e7a51b9fcfe11f70c9b66ae.camel@sipsolutions.net/ > ret = -EAGAIN; /* XXX ? */ > goto out; > } > -- > 2.34.1 > >
diff --git a/net/rds/info.c b/net/rds/info.c index b6b46a8214a0..8558b0a466b4 100644 --- a/net/rds/info.c +++ b/net/rds/info.c @@ -194,10 +194,7 @@ int rds_info_getsockopt(struct socket *sock, int optname, char __user *optval, } ret = pin_user_pages_fast(start, nr_pages, FOLL_WRITE, pages); if (ret != nr_pages) { - if (ret > 0) - nr_pages = ret; - else - nr_pages = 0; + nr_pages = max(ret, 0); ret = -EAGAIN; /* XXX ? */ goto out; }
The target if-else can be replaced with max(). Signed-off-by: Hongbo Li <lihongbo22@huawei.com> --- net/rds/info.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)