[PATCH] xen/events: fix build

[PATCH] xen/events: fix build

[Juergen Gross]

Commit 5f2df45ead7c1195 ("xen/evtchn: rework per event channel lock")
introduced a build failure for NDEBUG builds.

Fixes: 5f2df45ead7c1195 ("xen/evtchn: rework per event channel lock")
Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/common/event_channel.c | 2 ++
 xen/include/xen/sched.h    | 2 --
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index eacd96b92f..da85d536f4 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -61,7 +61,9 @@ static inline void evtchn_write_lock(struct evtchn *evtchn)
 {
     write_lock(&evtchn->lock);
 
+#ifndef NDEBUG
     evtchn->old_state = evtchn->state;
+#endif
 }
 
 static inline void evtchn_write_unlock(struct evtchn *evtchn)
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 7251b3ae3e..95f96e7a33 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -114,9 +114,7 @@ struct evtchn
         u16 virq;      /* state == ECS_VIRQ */
     } u;
     u8 priority;
-#ifndef NDEBUG
     u8 old_state;      /* State when taking lock in write mode. */
-#endif
     u8 last_priority;
     u16 last_vcpu_id;
 #ifdef CONFIG_XSM
-- 
2.26.2

Re: [PATCH] xen/events: fix build

[Jan Beulich]

On 11.11.2020 06:49, Juergen Gross wrote:
> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -61,7 +61,9 @@ static inline void evtchn_write_lock(struct evtchn *evtchn)
>  {
>      write_lock(&evtchn->lock);
>  
> +#ifndef NDEBUG
>      evtchn->old_state = evtchn->state;
> +#endif
>  }
>  
>  static inline void evtchn_write_unlock(struct evtchn *evtchn)
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index 7251b3ae3e..95f96e7a33 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -114,9 +114,7 @@ struct evtchn
>          u16 virq;      /* state == ECS_VIRQ */
>      } u;
>      u8 priority;
> -#ifndef NDEBUG
>      u8 old_state;      /* State when taking lock in write mode. */
> -#endif
>      u8 last_priority;
>      u16 last_vcpu_id;
>  #ifdef CONFIG_XSM

Did you mean just either of the two changes (and then the former),
not both? If so
Reviewed-by: Jan Beulich <jbeulich@suse.com>
and I'll be happy to drop the other half for committing.

Jan

Re: [PATCH] xen/events: fix build

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 1291 bytes --]

On 11.11.20 08:20, Jan Beulich wrote:
> On 11.11.2020 06:49, Juergen Gross wrote:
>> --- a/xen/common/event_channel.c
>> +++ b/xen/common/event_channel.c
>> @@ -61,7 +61,9 @@ static inline void evtchn_write_lock(struct evtchn *evtchn)
>>   {
>>       write_lock(&evtchn->lock);
>>   
>> +#ifndef NDEBUG
>>       evtchn->old_state = evtchn->state;
>> +#endif
>>   }
>>   
>>   static inline void evtchn_write_unlock(struct evtchn *evtchn)
>> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
>> index 7251b3ae3e..95f96e7a33 100644
>> --- a/xen/include/xen/sched.h
>> +++ b/xen/include/xen/sched.h
>> @@ -114,9 +114,7 @@ struct evtchn
>>           u16 virq;      /* state == ECS_VIRQ */
>>       } u;
>>       u8 priority;
>> -#ifndef NDEBUG
>>       u8 old_state;      /* State when taking lock in write mode. */
>> -#endif
>>       u8 last_priority;
>>       u16 last_vcpu_id;
>>   #ifdef CONFIG_XSM
> 
> Did you mean just either of the two changes (and then the former),
> not both? If so
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> and I'll be happy to drop the other half for committing.

The header fix is required for NDEBUG builds, while the other one is
removing a write with no accompanied read for NDEBUG builds.


Juergen


[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH] xen/events: fix build

[Jan Beulich]

On 11.11.2020 08:27, Jürgen Groß wrote:
> On 11.11.20 08:20, Jan Beulich wrote:
>> On 11.11.2020 06:49, Juergen Gross wrote:
>>> --- a/xen/common/event_channel.c
>>> +++ b/xen/common/event_channel.c
>>> @@ -61,7 +61,9 @@ static inline void evtchn_write_lock(struct evtchn *evtchn)
>>>   {
>>>       write_lock(&evtchn->lock);
>>>   
>>> +#ifndef NDEBUG
>>>       evtchn->old_state = evtchn->state;
>>> +#endif
>>>   }
>>>   
>>>   static inline void evtchn_write_unlock(struct evtchn *evtchn)
>>> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
>>> index 7251b3ae3e..95f96e7a33 100644
>>> --- a/xen/include/xen/sched.h
>>> +++ b/xen/include/xen/sched.h
>>> @@ -114,9 +114,7 @@ struct evtchn
>>>           u16 virq;      /* state == ECS_VIRQ */
>>>       } u;
>>>       u8 priority;
>>> -#ifndef NDEBUG
>>>       u8 old_state;      /* State when taking lock in write mode. */
>>> -#endif
>>>       u8 last_priority;
>>>       u16 last_vcpu_id;
>>>   #ifdef CONFIG_XSM
>>
>> Did you mean just either of the two changes (and then the former),
>> not both? If so
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>> and I'll be happy to drop the other half for committing.
> 
> The header fix is required for NDEBUG builds, while the other one is
> removing a write with no accompanied read for NDEBUG builds.

Oh, that's because of our absurd ASSERT() implementation in the
NDEBUG case. I stand by my position that the field should not be
there in the first place for release builds. Even more so with
the original patch having got re-based ahead of what was patch 1
of the series, which I did not the least because I want that one
backported urgently, while I continue to be hesitant altogether
about the other one.

I guess the course of action is then to put #ifndef NDEBUG
around the ASSERT() itself, however strange this may look. Or
introduce an evtchn_old_state() wrapper, perhaps returning
ECS_RESERVED in the NDEBUG case. I guess it'll be quicker if I
take your patch and massage it before throwing in, than to have
you make a v2.

Janan

Re: [PATCH] xen/events: fix build

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 2254 bytes --]

On 11.11.20 08:37, Jan Beulich wrote:
> On 11.11.2020 08:27, Jürgen Groß wrote:
>> On 11.11.20 08:20, Jan Beulich wrote:
>>> On 11.11.2020 06:49, Juergen Gross wrote:
>>>> --- a/xen/common/event_channel.c
>>>> +++ b/xen/common/event_channel.c
>>>> @@ -61,7 +61,9 @@ static inline void evtchn_write_lock(struct evtchn *evtchn)
>>>>    {
>>>>        write_lock(&evtchn->lock);
>>>>    
>>>> +#ifndef NDEBUG
>>>>        evtchn->old_state = evtchn->state;
>>>> +#endif
>>>>    }
>>>>    
>>>>    static inline void evtchn_write_unlock(struct evtchn *evtchn)
>>>> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
>>>> index 7251b3ae3e..95f96e7a33 100644
>>>> --- a/xen/include/xen/sched.h
>>>> +++ b/xen/include/xen/sched.h
>>>> @@ -114,9 +114,7 @@ struct evtchn
>>>>            u16 virq;      /* state == ECS_VIRQ */
>>>>        } u;
>>>>        u8 priority;
>>>> -#ifndef NDEBUG
>>>>        u8 old_state;      /* State when taking lock in write mode. */
>>>> -#endif
>>>>        u8 last_priority;
>>>>        u16 last_vcpu_id;
>>>>    #ifdef CONFIG_XSM
>>>
>>> Did you mean just either of the two changes (and then the former),
>>> not both? If so
>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>> and I'll be happy to drop the other half for committing.
>>
>> The header fix is required for NDEBUG builds, while the other one is
>> removing a write with no accompanied read for NDEBUG builds.
> 
> Oh, that's because of our absurd ASSERT() implementation in the
> NDEBUG case. I stand by my position that the field should not be
> there in the first place for release builds. Even more so with
> the original patch having got re-based ahead of what was patch 1
> of the series, which I did not the least because I want that one
> backported urgently, while I continue to be hesitant altogether
> about the other one.
> 
> I guess the course of action is then to put #ifndef NDEBUG
> around the ASSERT() itself, however strange this may look. Or
> introduce an evtchn_old_state() wrapper, perhaps returning
> ECS_RESERVED in the NDEBUG case. I guess it'll be quicker if I
> take your patch and massage it before throwing in, than to have
> you make a v2.

Fine with me.


Juergen

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]