Replace unnacessary BUG_ON to WARN_ON. These BUG_ONs verify aruguments of a function. Thus, the WARN_ONs return an EINVAL error when their condition is true.
Co-Developed-by: Gwan-gyeong Mun <[email protected]>
Co-Developed-by: Jeungwoo Yoo <[email protected]>
Co-Developed-by: Sangyun Kim <[email protected]>
Signed-off-by: Hyunmin Lee <[email protected]>
Signed-off-by: Gwan-gyeong Mun <[email protected]>
Signed-off-by: Jeungwoo Yoo <[email protected]>
Signed-off-by: Sangyun Kim <[email protected]>
Cc: Hyeonggon Yoo <[email protected]>
---
mm/vmalloc.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index 74afa2208558..9f9dba3132c5 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -1587,9 +1587,14 @@ static struct vmap_area *alloc_vmap_area(unsigned long size,
int purged = 0;
int ret;
- BUG_ON(!size);
- BUG_ON(offset_in_page(size));
- BUG_ON(!is_power_of_2(align));
+ if (WARN_ON(!size))
+ return ERR_PTR(-EINVAL);
+
+ if (WARN_ON(offset_in_page(size)))
+ return ERR_PTR(-EINVAL);
+
+ if (WARN_ON(!is_power_of_2(align)))
+ return ERR_PTR(-EINVAL);
if (unlikely(!vmap_initialized))
return ERR_PTR(-EBUSY);
--
2.25.1
Hello Hyunmin.
the subject line could be "mm/vmalloc: replace BUG_ON() with WARN_ON()".
On Fri, Jan 27, 2023 at 08:58:44PM +0900, Hyunmin Lee wrote:
> Replace unnacessary BUG_ON to WARN_ON. These BUG_ONs verify aruguments of a function. Thus, the WARN_ONs return an EINVAL error when their condition is true.
>
> Co-Developed-by: Gwan-gyeong Mun <[email protected]>
> Co-Developed-by: Jeungwoo Yoo <[email protected]>
> Co-Developed-by: Sangyun Kim <[email protected]>
> Signed-off-by: Hyunmin Lee <[email protected]>
> Signed-off-by: Gwan-gyeong Mun <[email protected]>
> Signed-off-by: Jeungwoo Yoo <[email protected]>
> Signed-off-by: Sangyun Kim <[email protected]>
> Cc: Hyeonggon Yoo <[email protected]>
could be rephrased a little, like:
"As per the coding standards, in the event of an abnormal condition that
should not occur under normal circumstances, the kernel should attempt
recovery and proceed with execution, rather than halting the machine.
Specifically, in the alloc_vmap_area() function, use WARN_ON()
and fail the request instead of using BUG_ON() to halt the machine."
> ---
> mm/vmalloc.c | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index 74afa2208558..9f9dba3132c5 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -1587,9 +1587,14 @@ static struct vmap_area *alloc_vmap_area(unsigned long size,
> int purged = 0;
> int ret;
>
> - BUG_ON(!size);
> - BUG_ON(offset_in_page(size));
> - BUG_ON(!is_power_of_2(align));
> + if (WARN_ON(!size))
> + return ERR_PTR(-EINVAL);
> +
> + if (WARN_ON(offset_in_page(size)))
> + return ERR_PTR(-EINVAL);
> +
> + if (WARN_ON(!is_power_of_2(align)))
> + return ERR_PTR(-EINVAL);
>
> if (unlikely(!vmap_initialized))
> return ERR_PTR(-EBUSY);
> --
> 2.25.1
The code change itself looks fine to me.
Even if BUG*() -> WARN*() conversion may not be a high priority task,
I see no reason to reject such changes.
--
Thanks,
Hyeonggon
Hi,
On Fri, Jan 27, 2023 at 08:58:44PM +0900, Hyunmin Lee wrote:
> Replace unnacessary BUG_ON to WARN_ON. These BUG_ONs verify aruguments of a function. Thus, the WARN_ONs return an EINVAL error when their condition is true.
Some users enable panic_on_warn, so for them WARN_ON will still crash a
machine.
I think a simple if() will be sufficient.
> Co-Developed-by: Gwan-gyeong Mun <[email protected]>
> Co-Developed-by: Jeungwoo Yoo <[email protected]>
> Co-Developed-by: Sangyun Kim <[email protected]>
> Signed-off-by: Hyunmin Lee <[email protected]>
> Signed-off-by: Gwan-gyeong Mun <[email protected]>
> Signed-off-by: Jeungwoo Yoo <[email protected]>
> Signed-off-by: Sangyun Kim <[email protected]>
> Cc: Hyeonggon Yoo <[email protected]>
> ---
> mm/vmalloc.c | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index 74afa2208558..9f9dba3132c5 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -1587,9 +1587,14 @@ static struct vmap_area *alloc_vmap_area(unsigned long size,
> int purged = 0;
> int ret;
>
> - BUG_ON(!size);
> - BUG_ON(offset_in_page(size));
> - BUG_ON(!is_power_of_2(align));
> + if (WARN_ON(!size))
> + return ERR_PTR(-EINVAL);
> +
> + if (WARN_ON(offset_in_page(size)))
> + return ERR_PTR(-EINVAL);
> +
> + if (WARN_ON(!is_power_of_2(align)))
> + return ERR_PTR(-EINVAL);
>
> if (unlikely(!vmap_initialized))
> return ERR_PTR(-EBUSY);
> --
> 2.25.1
>
>
--
Sincerely yours,
Mike.
On Mon, Jan 30, 2023 at 12:14:04PM +0200, Mike Rapoport wrote:
> Hi,
>
> On Fri, Jan 27, 2023 at 08:58:44PM +0900, Hyunmin Lee wrote:
> > Replace unnacessary BUG_ON to WARN_ON. These BUG_ONs verify aruguments of a function. Thus, the WARN_ONs return an EINVAL error when their condition is true.
>
> Some users enable panic_on_warn, so for them WARN_ON will still crash a
> machine.
>
> I think a simple if() will be sufficient.
>
> > Co-Developed-by: Gwan-gyeong Mun <[email protected]>
> > Co-Developed-by: Jeungwoo Yoo <[email protected]>
> > Co-Developed-by: Sangyun Kim <[email protected]>
> > Signed-off-by: Hyunmin Lee <[email protected]>
> > Signed-off-by: Gwan-gyeong Mun <[email protected]>
> > Signed-off-by: Jeungwoo Yoo <[email protected]>
> > Signed-off-by: Sangyun Kim <[email protected]>
> > Cc: Hyeonggon Yoo <[email protected]>
> > ---
> > mm/vmalloc.c | 11 ++++++++---
> > 1 file changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> > index 74afa2208558..9f9dba3132c5 100644
> > --- a/mm/vmalloc.c
> > +++ b/mm/vmalloc.c
> > @@ -1587,9 +1587,14 @@ static struct vmap_area *alloc_vmap_area(unsigned long size,
> > int purged = 0;
> > int ret;
> >
> > - BUG_ON(!size);
> > - BUG_ON(offset_in_page(size));
> > - BUG_ON(!is_power_of_2(align));
> > + if (WARN_ON(!size))
> > + return ERR_PTR(-EINVAL);
> > +
> > + if (WARN_ON(offset_in_page(size)))
> > + return ERR_PTR(-EINVAL);
> > +
> > + if (WARN_ON(!is_power_of_2(align)))
> > + return ERR_PTR(-EINVAL);
> >
> > if (unlikely(!vmap_initialized))
> > return ERR_PTR(-EBUSY);
> > --
> > 2.25.1
> >
> >
>
> --
> Sincerely yours,
> Mike.
Hi Mike,
Thank you for your advice.
Would you please give feedback about the below opinion?
- Printing warning messages is helpful to inform what happened in the system to the users.
- When a simple if() is used instead of WARN_ON, the if() should print a warning message.
- The condition of the simple if() should also have unlikely() for optimization of system performance.
- WARN_ON is a macro doing like thoes easily. It has a notifying function and unlikely optimization.
- Eventhough WARN_ON will still crash like BUG_ON by some users who enable panic_on_warn, it is their intention. They should accept the crash by WARN_ON.
- Therefore, using WARN_ON looks like natural and efficient.
Best,
Hyunmin
On Tue, Jan 31, 2023 at 07:56:29PM +0900, Hyunmin Lee wrote:
> On Mon, Jan 30, 2023 at 12:14:04PM +0200, Mike Rapoport wrote:
> > Hi,
> >
> > On Fri, Jan 27, 2023 at 08:58:44PM +0900, Hyunmin Lee wrote:
> > > Replace unnacessary BUG_ON to WARN_ON. These BUG_ONs verify aruguments of a function. Thus, the WARN_ONs return an EINVAL error when their condition is true.
> >
> > Some users enable panic_on_warn, so for them WARN_ON will still crash a
> > machine.
> >
> > I think a simple if() will be sufficient.
> >
> Hi Mike,
>
> Thank you for your advice.
> Would you please give feedback about the below opinion?
> - Printing warning messages is helpful to inform what happened in the system to the users.
> - When a simple if() is used instead of WARN_ON, the if() should print a warning message.
> - The condition of the simple if() should also have unlikely() for optimization of system performance.
> - WARN_ON is a macro doing like thoes easily. It has a notifying function and unlikely optimization.
> - Eventhough WARN_ON will still crash like BUG_ON by some users who enable panic_on_warn, it is their intention. They should accept the crash by WARN_ON.
> - Therefore, using WARN_ON looks like natural and efficient.
As this is a validation of the function parameters, there is no need in
warning messages and if(unlikely()) will do. There is really no point in
WARN_ON() for something that's totally recoverable and very unlikely to
happen.
> Best,
> Hyunmin
--
Sincerely yours,
Mike.
On Tue, Jan 31, 2023 at 03:47:05PM +0200, Mike Rapoport wrote:
> On Tue, Jan 31, 2023 at 07:56:29PM +0900, Hyunmin Lee wrote:
> > On Mon, Jan 30, 2023 at 12:14:04PM +0200, Mike Rapoport wrote:
> > > Hi,
> > >
> > > On Fri, Jan 27, 2023 at 08:58:44PM +0900, Hyunmin Lee wrote:
> > > > Replace unnacessary BUG_ON to WARN_ON. These BUG_ONs verify aruguments of a function. Thus, the WARN_ONs return an EINVAL error when their condition is true.
> > >
> > > Some users enable panic_on_warn, so for them WARN_ON will still crash a
> > > machine.
> > >
> > > I think a simple if() will be sufficient.
> > >
> > Hi Mike,
> >
> > Thank you for your advice.
> > Would you please give feedback about the below opinion?
> > - Printing warning messages is helpful to inform what happened in the system to the users.
> > - When a simple if() is used instead of WARN_ON, the if() should print a warning message.
> > - The condition of the simple if() should also have unlikely() for optimization of system performance.
> > - WARN_ON is a macro doing like thoes easily. It has a notifying function and unlikely optimization.
> > - Eventhough WARN_ON will still crash like BUG_ON by some users who enable panic_on_warn, it is their intention. They should accept the crash by WARN_ON.
> > - Therefore, using WARN_ON looks like natural and efficient.
>
> As this is a validation of the function parameters, there is no need in
> warning messages and if(unlikely()) will do. There is really no point in
> WARN_ON() for something that's totally recoverable and very unlikely to
> happen.
>
> > Best,
> > Hyunmin
>
> --
> Sincerely yours,
> Mike.
Hi Mike,
According to your guidance, I will send a v3 patch.
Thanks a lot.
Best,
Min