code question?

code question?

[Jerone Young]

Doing some janitorial (you cleaning the flooded toilets and such) work
today. I have come across this line of code that really I'm not sure
what the intent was..in xen/include/sched.h

#define hypercall_preempt_check() (unlikely(            \
        softirq_pending(smp_processor_id()) |           \
        (!!current->vcpu_info->evtchn_upcall_pending &  \
          !current->vcpu_info->evtchn_upcall_mask)      \
    ))

the part where we have !!current->vcpu_info_evtchen_upcall pending
should this be..should the "!! just be "!"?

And if so shouldn't this just be changed to 

#define hypercall_preempt_check() (unlikely( \
	softirq_pending(smp_processor_id()) |           \
	(!(current->vcpu_info->evtchn_upcall_pending & \
	  current->vcpu_info->evtchn_upcall_mask)) \
	))
	

In a lot of the code in Xen we are using the "!" operator with bitwise
operations..this is one of those examples.



-- 
Jerone Young
IBM Linux Technology Center
jyoung5@us.ibm.com
512-838-1157 (T/L: 678-1157)

Re: code question?

[Anthony Liguori]

Jerone Young wrote:

>Doing some janitorial (you cleaning the flooded toilets and such) work
>today. I have come across this line of code that really I'm not sure
>what the intent was..in xen/include/sched.h
>
>#define hypercall_preempt_check() (unlikely(            \
>        softirq_pending(smp_processor_id()) |           \
>        (!!current->vcpu_info->evtchn_upcall_pending &  \
>          !current->vcpu_info->evtchn_upcall_mask)      \
>    ))
>
>the part where we have !!current->vcpu_info_evtchen_upcall pending
>should this be..should the "!! just be "!"?
>  
>
The usual use of !!a is to do (a != 0) ? 1 : 0.

I'm not sure if there's a more readable way to do it.

Regards,

Anthony Liguori

>And if so shouldn't this just be changed to 
>
>#define hypercall_preempt_check() (unlikely( \
>	softirq_pending(smp_processor_id()) |           \
>	(!(current->vcpu_info->evtchn_upcall_pending & \
>	  current->vcpu_info->evtchn_upcall_mask)) \
>	))
>	
>
>In a lot of the code in Xen we are using the "!" operator with bitwise
>operations..this is one of those examples.
>
>
>
>  
>

Re: code question?

[Keir Fraser]

On 11 Aug 2005, at 16:55, Jerone Young wrote:

> the part where we have !!current->vcpu_info_evtchen_upcall pending
> should this be..should the "!! just be "!"?

The code is being a bit defensive, and dealing with the case that 
evtchn_upcall_pending may be non-zero, but the least significant bit 
isn't set. That is never actually the case (Xen never sets any other 
bit than the lsb) so the code could be changed, but not in the way you 
suggest. The correct change would be simply to remove the !!.

> In a lot of the code in Xen we are using the "!" operator with bitwise
> operations..this is one of those examples.

Forming compound predicates for bitwise operators can be faster than 
using the logical operators because they are non 'short circuiting'. 
This means you end up with fewer branches in the generated code and end 
up with nice straight-line code that should execute fast.

  -- Keir

Re: code question?

[David Hopwood]

Jerone Young wrote:
> Doing some janitorial (you cleaning the flooded toilets and such) work
> today. I have come across this line of code that really I'm not sure
> what the intent was..in xen/include/sched.h
> 
> #define hypercall_preempt_check() (unlikely(            \
>         softirq_pending(smp_processor_id()) |           \
>         (!!current->vcpu_info->evtchn_upcall_pending &  \
>           !current->vcpu_info->evtchn_upcall_mask)      \
>     ))
> 
> the part where we have !!current->vcpu_info_evtchen_upcall pending
> should this be..should the "!! just be "!"?
> 
> And if so shouldn't this just be changed to 
> 
> #define hypercall_preempt_check() (unlikely( \
> 	softirq_pending(smp_processor_id()) |           \
> 	(!(current->vcpu_info->evtchn_upcall_pending & \
> 	  current->vcpu_info->evtchn_upcall_mask)) \
> 	))

Seems more likely to have been intended as:

   #define hypercall_preempt_check() (unlikely(           \
           softirq_pending(smp_processor_id()) ||         \
           (current->vcpu_info->evtchn_upcall_pending &&  \
             !current->vcpu_info->evtchn_upcall_mask)     \
       ))

Keir wrote:
 > Forming compound predicates for bitwise operators can be faster than using the
 > logical operators because they are non 'short circuiting'. This means you end
 > up with fewer branches in the generated code and end up with nice straight-line
 > code that should execute fast.

If that's true on a given platform, the compiler can optimize it. Note that
in this case "(current->vcpu_info->evtchn_upcall_pending &&
!current->vcpu_info->evtchn_upcall_mask)" has no side effects (unless current
or current->vcpu_info are not valid pointers, which is undefined behaviour so
still optimizable).

-- 
David Hopwood <david.nospam.hopwood@blueyonder.co.uk>

Re: code question?

[M.A. Williamson]

>Doing some janitorial (you cleaning the flooded toilets and such) work
>today. I have come across this line of code that really I'm not sure
>what the intent was..in xen/include/sched.h
>
>#define hypercall_preempt_check() (unlikely(            \
>        softirq_pending(smp_processor_id()) |           \
>        (!!current->vcpu_info->evtchn_upcall_pending &  \
>          !current->vcpu_info->evtchn_upcall_mask)      \
>    ))
>
>the part where we have !!current->vcpu_info_evtchen_upcall pending
>should this be..should the "!! just be "!"?

I think that:
!!a & !b === (a != 0 ) && ( b == 0 )

It's just more efficient to do it directly using bitwise ops than to use 
the full logical operators (can gcc really not optimise this sort of 
thing???).

Cheers,
Mark

>And if so shouldn't this just be changed to 
>
>#define hypercall_preempt_check() (unlikely( \
>	softirq_pending(smp_processor_id()) |           \
>	(!(current->vcpu_info->evtchn_upcall_pending & \
>	  current->vcpu_info->evtchn_upcall_mask)) \
>	))
>	
>
>In a lot of the code in Xen we are using the "!" operator with bitwise
>operations..this is one of those examples.
>
>
>
>

Re: code question?

[Keir Fraser]

On 11 Aug 2005, at 18:03, M.A. Williamson wrote:

> I think that:
> !!a & !b === (a != 0 ) && ( b == 0 )
>
> It's just more efficient to do it directly using bitwise ops than to 
> use the full logical operators (can gcc really not optimise this sort 
> of thing???).

It can't optimise out short-circuit semantics. Without predicates on 
instructions (like e.g. ARM has) it has to emit conditional branches.

  -- Keir

Re: code question?

[Tim Newsham]

On Thu, 11 Aug 2005, Keir Fraser wrote:
> Forming compound predicates for bitwise operators can be faster than using 
> the logical operators because they are non 'short circuiting'. This means you 
> end up with fewer branches in the generated code and end up with nice 
> straight-line code that should execute fast.

Is it worth it to perform these optimizations?  For example the code to 
get time information in xen/386 linux:

        do {
                 shadow_tv_version = s->wc_version;
                 rmb();
                 shadow_tv.tv_sec  = s->wc_sec;
                 shadow_tv.tv_nsec = s->wc_nsec;
                 rmb();
         }
         while ((s->wc_version & 1) | (shadow_tv_version ^ s->wc_version));

Granted most experienced programmers will recognize that the xor is used 
to test for inequality and the bitwise-or is used as a logical-or.  But 
how often does this piece of code even run?  Seems like a case of trading 
readability for speed when optimization isn't even important.

> -- Keir

Tim Newsham
http://www.lava.net/~newsham/

Re: code question?

[David Hopwood]

Keir Fraser wrote:
> On 11 Aug 2005, at 18:03, M.A. Williamson wrote:
> 
>> I think that:
>> !!a & !b === (a != 0 ) && ( b == 0 )
>>
>> It's just more efficient to do it directly using bitwise ops than to 
>> use the full logical operators (can gcc really not optimise this sort 
>> of thing???).
> 
> It can't optimise out short-circuit semantics.

Yes it can, if the second branch of the short-circuit is guaranteed to
terminate with no side effects when it has defined behaviour.

The function thread_jump in
<http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/cfgcleanup.c?rev=1.140.2.7>
attempts to prove that a basic block satisfies this property, although I'm
not sure whether gcc is capable of doing the specific optimization we're
talking about here. I know that some other C compilers are.

-- 
David Hopwood <david.nospam.hopwood@blueyonder.co.uk>