Re: [for-4.8 PATCH] xen: credit2: fix wrong assert in runq_tickle().

[Dario Faggioli <dario.faggioli@citrix.com>]

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

On Tue, 2016-11-22 at 04:21 -0700, Jan Beulich wrote:
> > > > On 22.11.16 at 11:43, <dario.faggioli@citrix.com> wrote:
> > Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
Thanks a lot for looking at the patch.

> with two remarks:
>
> [...]
> 
> If you moved this past the if() following this comment, the ipid ==
> -1
> case would already be taken care of, simplifying the code.
> 
True. Though about it, but was two minded.

I think the ASSERT is more 'descriptive' if it's kept as close as
possible to the for (and in fact, I now even regret having put a blank
line in between). But I see your point and agree that it's looking
awkward when you see it together with the following if.

So, yes, all in all, I think I like your idea better.

> And then, having looked back at the commit mentioned in the
> description, that one resulted in two constructs like (taking the
> code as it looks now)
> 
>             if ( cpumask_test_cpu(cpu, &rqd->idle) )
>             {
>                 __cpumask_clear_cpu(cpu, &rqd->idle);
>                 ...
> 
> Is there a reason this can't or shouldn't be
> 
>             if ( __cpumask_test_and_clear_cpu(cpu, &rqd->idle) )
>             {
>                 ...
> ?
> 
It can indeed. You can find it done as it is right now in other places,
in scheduling code, though, I guess for cache betterness, as Juergen is
saying. I remember not being sure which way to go, and eventually
leaning toward this one.

I'm still not sure what's best, but it'd be a cleanup/optimization, and
would IMO require, if done, more than just that (e.g., comments should
be improved). So I'd be inclined to consider this 4.9 material... But
thanks for bringing it up. :-)

So, I'm resending this patch with the ASSERT moved below the if, and
I'm keeping your Reviewed-by. Hope that's ok.

Thanks again and 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