[for-4.9 PATCH] xen: credit: change an ASSERT on nr_runnable so that it makes sense.

[for-4.9 PATCH] xen: credit: change an ASSERT on nr_runnable so that it makes sense.

[Dario Faggioli]

Since the counter is unsigned, it's pointless/bogous to check
for if to be above zero.

Check that it is at least one before it's decremented, instead.

Spotted by Coverity.

Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
---
Cc: George Dunlap <george.dunlap@citrix.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Julien Grall <julien.grall@arm.com>
---
Julien,

This is very low risk, and I'd call it a bugfix in the sense that it quiesces
coverity.

Dario
---
 xen/common/sched_credit.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
index 93658dc..efdf6bf 100644
--- a/xen/common/sched_credit.c
+++ b/xen/common/sched_credit.c
@@ -275,8 +275,8 @@ static inline void
 dec_nr_runnable(unsigned int cpu)
 {
     ASSERT(spin_is_locked(per_cpu(schedule_data, cpu).schedule_lock));
+    ASSERT(CSCHED_PCPU(cpu)->nr_runnable >= 1);
     CSCHED_PCPU(cpu)->nr_runnable--;
-    ASSERT(CSCHED_PCPU(cpu)->nr_runnable >= 0);
 }
 
 static inline void


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [for-4.9 PATCH] xen: credit: change an ASSERT on nr_runnable so that it makes sense.

[Julien Grall]

Hi Dario,

On 13/04/17 08:49, Dario Faggioli wrote:
> Since the counter is unsigned, it's pointless/bogous to check
> for if to be above zero.
>
> Check that it is at least one before it's decremented, instead.
>
> Spotted by Coverity.

Do you have the Coverity-ID? :)

>
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
> ---
> Cc: George Dunlap <george.dunlap@citrix.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Julien Grall <julien.grall@arm.com>
> ---
> Julien,
>
> This is very low risk, and I'd call it a bugfix in the sense that it quiesces
> coverity.

Release-Acked-by: Julien Grall <julien.grall@arm.com>

>
> Dario
> ---
>  xen/common/sched_credit.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
> index 93658dc..efdf6bf 100644
> --- a/xen/common/sched_credit.c
> +++ b/xen/common/sched_credit.c
> @@ -275,8 +275,8 @@ static inline void
>  dec_nr_runnable(unsigned int cpu)
>  {
>      ASSERT(spin_is_locked(per_cpu(schedule_data, cpu).schedule_lock));
> +    ASSERT(CSCHED_PCPU(cpu)->nr_runnable >= 1);
>      CSCHED_PCPU(cpu)->nr_runnable--;
> -    ASSERT(CSCHED_PCPU(cpu)->nr_runnable >= 0);
>  }
>
>  static inline void
>

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [for-4.9 PATCH] xen: credit: change an ASSERT on nr_runnable so that it makes sense.

[Wei Liu]

On Thu, Apr 13, 2017 at 10:42:56AM +0100, Julien Grall wrote:
> Hi Dario,
> 
> On 13/04/17 08:49, Dario Faggioli wrote:
> > Since the counter is unsigned, it's pointless/bogous to check
> > for if to be above zero.
> > 
> > Check that it is at least one before it's decremented, instead.
> > 
> > Spotted by Coverity.
> 
> Do you have the Coverity-ID? :)
> 

Not really. It is XenServer's internal instance.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [for-4.9 PATCH] xen: credit: change an ASSERT on nr_runnable so that it makes sense.

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1120 bytes --]

On Thu, 2017-04-13 at 10:42 +0100, Julien Grall wrote:
> Hi Dario,
> 
> On 13/04/17 08:49, Dario Faggioli wrote:
> > Since the counter is unsigned, it's pointless/bogous to check
> > for if to be above zero.
> > 
> > Check that it is at least one before it's decremented, instead.
> > 
> > Spotted by Coverity.
> 
> Do you have the Coverity-ID? :)
> 
This comes from the Citrix internal instance, so the ID wouldn't make
any sense.

I don't know if the XenProject instance has also caught it. I guess
it's likely, but I don't have access, so I can't check.

Adding the above line is what Andrew suggested doing (saying he does it
 himself) when things like this happens, to better reflect the reality.

Let me know if I should do anything different (of feel free to add or
change anything related to this upon commit).

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [for-4.9 PATCH] xen: credit: change an ASSERT on nr_runnable so that it makes sense.

[Andrew Cooper]

On 13/04/17 11:00, Dario Faggioli wrote:
> On Thu, 2017-04-13 at 10:42 +0100, Julien Grall wrote:
>> Hi Dario,
>>
>> On 13/04/17 08:49, Dario Faggioli wrote:
>>> Since the counter is unsigned, it's pointless/bogous to check
>>> for if to be above zero.
>>>
>>> Check that it is at least one before it's decremented, instead.
>>>
>>> Spotted by Coverity.
>> Do you have the Coverity-ID? :)
>>
> This comes from the Citrix internal instance, so the ID wouldn't make
> any sense.
>
> I don't know if the XenProject instance has also caught it. I guess
> it's likely, but I don't have access, so I can't check.

OSSTest is currently failing at the Coverity jobs, probably because of
firewall issues uploading the analysis results.

I expect it would have noticed, had an upload succeeded recently.

On the subject of access, we really should open it up now.  The fact
that anyone can clone Xen and run Coverity themselves means there is no
point in keeping the main one private.

>
> Adding the above line is what Andrew suggested doing (saying he does it
>  himself) when things like this happens, to better reflect the reality.

It is the least bad way I've found of expressing the difference.

>
> Let me know if I should do anything different (of feel free to add or
> change anything related to this upon commit).
>
> Regards,
> Dario


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [for-4.9 PATCH] xen: credit: change an ASSERT on nr_runnable so that it makes sense.

[George Dunlap]

On 13/04/17 08:49, Dario Faggioli wrote:
> Since the counter is unsigned, it's pointless/bogous to check
> for if to be above zero.
> 
> Check that it is at least one before it's decremented, instead.
> 
> Spotted by Coverity.
> 
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>

Reviewed-by: George Dunlap <george.dunlap@citrix.com>

And queued

> ---
> Cc: George Dunlap <george.dunlap@citrix.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Julien Grall <julien.grall@arm.com>
> ---
> Julien,
> 
> This is very low risk, and I'd call it a bugfix in the sense that it quiesces
> coverity.
> 
> Dario
> ---
>  xen/common/sched_credit.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
> index 93658dc..efdf6bf 100644
> --- a/xen/common/sched_credit.c
> +++ b/xen/common/sched_credit.c
> @@ -275,8 +275,8 @@ static inline void
>  dec_nr_runnable(unsigned int cpu)
>  {
>      ASSERT(spin_is_locked(per_cpu(schedule_data, cpu).schedule_lock));
> +    ASSERT(CSCHED_PCPU(cpu)->nr_runnable >= 1);
>      CSCHED_PCPU(cpu)->nr_runnable--;
> -    ASSERT(CSCHED_PCPU(cpu)->nr_runnable >= 0);
>  }
>  
>  static inline void
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel