replace BUG_ON to WARN_ON

Commit Message

Hyunmin Lee Jan. 27, 2023, 11:58 a.m. UTC

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 <[e-mail]>
Co-Developed-by: Jeungwoo Yoo <[e-mail]>
Co-Developed-by: Sangyun Kim <[e-mail]>
Signed-off-by: Hyunmin Lee <[e-mail]>
Signed-off-by: Gwan-gyeong Mun <[e-mail]>
Signed-off-by: Jeungwoo Yoo <[e-mail]>
Signed-off-by: Sangyun Kim <[e-mail]>
Cc: Hyeonggon Yoo <[e-mail]>
---
 mm/vmalloc.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

Comments

Hyeonggon Yoo Jan. 27, 2023, 3:03 p.m. UTC | #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 <[e-mail]>
> Co-Developed-by: Jeungwoo Yoo <[e-mail]>
> Co-Developed-by: Sangyun Kim <[e-mail]>
> Signed-off-by: Hyunmin Lee <[e-mail]>
> Signed-off-by: Gwan-gyeong Mun <[e-mail]>
> Signed-off-by: Jeungwoo Yoo <[e-mail]>
> Signed-off-by: Sangyun Kim <[e-mail]>
> Cc: Hyeonggon Yoo <[e-mail]>

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

Mike Rapoport Jan. 30, 2023, 10:14 a.m. UTC | #2

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 <[e-mail]>
> Co-Developed-by: Jeungwoo Yoo <[e-mail]>
> Co-Developed-by: Sangyun Kim <[e-mail]>
> Signed-off-by: Hyunmin Lee <[e-mail]>
> Signed-off-by: Gwan-gyeong Mun <[e-mail]>
> Signed-off-by: Jeungwoo Yoo <[e-mail]>
> Signed-off-by: Sangyun Kim <[e-mail]>
> Cc: Hyeonggon Yoo <[e-mail]>
> ---
>  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
> 
>

Hyunmin Lee Jan. 31, 2023, 10:56 a.m. UTC | #3

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 <[e-mail]>
> > Co-Developed-by: Jeungwoo Yoo <[e-mail]>
> > Co-Developed-by: Sangyun Kim <[e-mail]>
> > Signed-off-by: Hyunmin Lee <[e-mail]>
> > Signed-off-by: Gwan-gyeong Mun <[e-mail]>
> > Signed-off-by: Jeungwoo Yoo <[e-mail]>
> > Signed-off-by: Sangyun Kim <[e-mail]>
> > Cc: Hyeonggon Yoo <[e-mail]>
> > ---
> >  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

Mike Rapoport Jan. 31, 2023, 1:47 p.m. UTC | #4

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

Hyunmin Lee Feb. 1, 2023, 10:03 a.m. UTC | #5

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

Patch

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);