In tools/libs/evtchn/minios.c there are assert()s for the current
thread being the main thread when binding an event channel.

As Mini-OS is supporting multiple threads, there is no real reason
why the binding shouldn't be allowed to happen in any other thread.

Just drop the assert()s.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/libs/evtchn/minios.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/tools/libs/evtchn/minios.c b/tools/libs/evtchn/minios.c
index 28743cb055..e33ddec7e7 100644
--- a/tools/libs/evtchn/minios.c
+++ b/tools/libs/evtchn/minios.c
@@ -195,7 +195,6 @@ xenevtchn_port_or_error_t xenevtchn_bind_unbound_port(xenevtchn_handle *xce,
     int ret;
     evtchn_port_t port;
 
-    assert(get_current() == main_thread);
     port_info = port_alloc(xce);
     if ( port_info == NULL )
         return -1;
@@ -226,7 +225,6 @@ xenevtchn_port_or_error_t xenevtchn_bind_interdomain(xenevtchn_handle *xce,
     evtchn_port_t local_port;
     int ret;
 
-    assert(get_current() == main_thread);
     port_info = port_alloc(xce);
     if ( port_info == NULL )
         return -1;
@@ -279,7 +277,6 @@ xenevtchn_port_or_error_t xenevtchn_bind_virq(xenevtchn_handle *xce,
     struct port_info *port_info;
     evtchn_port_t port;
 
-    assert(get_current() == main_thread);
     port_info = port_alloc(xce);
     if ( port_info == NULL )
         return -1;
-- 
2.35.3

On Wed, Dec 6, 2023 at 9:40 AM Juergen Gross <jgross@suse.com> wrote:
>
> In tools/libs/evtchn/minios.c there are assert()s for the current
> thread being the main thread when binding an event channel.
>
> As Mini-OS is supporting multiple threads, there is no real reason
> why the binding shouldn't be allowed to happen in any other thread.
>
> Just drop the assert()s.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  tools/libs/evtchn/minios.c | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/tools/libs/evtchn/minios.c b/tools/libs/evtchn/minios.c
> index 28743cb055..e33ddec7e7 100644
> --- a/tools/libs/evtchn/minios.c
> +++ b/tools/libs/evtchn/minios.c
> @@ -195,7 +195,6 @@ xenevtchn_port_or_error_t xenevtchn_bind_unbound_port(xenevtchn_handle *xce,
>      int ret;
>      evtchn_port_t port;
>
> -    assert(get_current() == main_thread);
>      port_info = port_alloc(xce);

If multiple threads are allowed, does port_list need to gain a lock
protecting it?

Regards,
Jason

On 06.12.23 17:38, Jason Andryuk wrote:
> On Wed, Dec 6, 2023 at 9:40 AM Juergen Gross <jgross@suse.com> wrote:
>>
>> In tools/libs/evtchn/minios.c there are assert()s for the current
>> thread being the main thread when binding an event channel.
>>
>> As Mini-OS is supporting multiple threads, there is no real reason
>> why the binding shouldn't be allowed to happen in any other thread.
>>
>> Just drop the assert()s.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>   tools/libs/evtchn/minios.c | 3 ---
>>   1 file changed, 3 deletions(-)
>>
>> diff --git a/tools/libs/evtchn/minios.c b/tools/libs/evtchn/minios.c
>> index 28743cb055..e33ddec7e7 100644
>> --- a/tools/libs/evtchn/minios.c
>> +++ b/tools/libs/evtchn/minios.c
>> @@ -195,7 +195,6 @@ xenevtchn_port_or_error_t xenevtchn_bind_unbound_port(xenevtchn_handle *xce,
>>       int ret;
>>       evtchn_port_t port;
>>
>> -    assert(get_current() == main_thread);
>>       port_info = port_alloc(xce);
> 
> If multiple threads are allowed, does port_list need to gain a lock
> protecting it?

I thought of that, too.

The answer is: maybe

Any other list operation on the list isn't protected by an assert(), so
technically there is no real new aspect added in this regard.

I believe adding a lock would make sense, but it is orthogonal to this
patch.


Juergen

On Wed, Dec 6, 2023 at 11:44 AM Juergen Gross <jgross@suse.com> wrote:
>
> On 06.12.23 17:38, Jason Andryuk wrote:
> > On Wed, Dec 6, 2023 at 9:40 AM Juergen Gross <jgross@suse.com> wrote:
> >>
> >> In tools/libs/evtchn/minios.c there are assert()s for the current
> >> thread being the main thread when binding an event channel.
> >>
> >> As Mini-OS is supporting multiple threads, there is no real reason
> >> why the binding shouldn't be allowed to happen in any other thread.
> >>
> >> Just drop the assert()s.
> >>
> >> Signed-off-by: Juergen Gross <jgross@suse.com>
> >> ---
> >>   tools/libs/evtchn/minios.c | 3 ---
> >>   1 file changed, 3 deletions(-)
> >>
> >> diff --git a/tools/libs/evtchn/minios.c b/tools/libs/evtchn/minios.c
> >> index 28743cb055..e33ddec7e7 100644
> >> --- a/tools/libs/evtchn/minios.c
> >> +++ b/tools/libs/evtchn/minios.c
> >> @@ -195,7 +195,6 @@ xenevtchn_port_or_error_t xenevtchn_bind_unbound_port(xenevtchn_handle *xce,
> >>       int ret;
> >>       evtchn_port_t port;
> >>
> >> -    assert(get_current() == main_thread);
> >>       port_info = port_alloc(xce);
> >
> > If multiple threads are allowed, does port_list need to gain a lock
> > protecting it?
>
> I thought of that, too.
>
> The answer is: maybe
>
> Any other list operation on the list isn't protected by an assert(), so
> technically there is no real new aspect added in this regard.

Yes.

> I believe adding a lock would make sense, but it is orthogonal to this
> patch.

The assert() feels like it was an attempt to avoid introducing
locking, so I'm not sure it is really orthogonal.

I was kinda waiting to see if anyone else would lend an opinion.

Since the asserts haven't been tripping there doesn't seem to be an
issue with the code as-is, so:

Reviewed-by: Jason Andryuk <jandryuk@gmail.com>

Regards,
Jason

Le mer. 6 déc. 2023 à 21:03, Jason Andryuk <jandryuk@gmail.com> a écrit :
>
> On Wed, Dec 6, 2023 at 11:44 AM Juergen Gross <jgross@suse.com> wrote:
> >
> > On 06.12.23 17:38, Jason Andryuk wrote:
> > > On Wed, Dec 6, 2023 at 9:40 AM Juergen Gross <jgross@suse.com> wrote:
> > >>
> > >> In tools/libs/evtchn/minios.c there are assert()s for the current
> > >> thread being the main thread when binding an event channel.
> > >>
> > >> As Mini-OS is supporting multiple threads, there is no real reason
> > >> why the binding shouldn't be allowed to happen in any other thread.
> > >>
> > >> Just drop the assert()s.
> > >>
> > >> Signed-off-by: Juergen Gross <jgross@suse.com>
> > >> ---
> > >>   tools/libs/evtchn/minios.c | 3 ---
> > >>   1 file changed, 3 deletions(-)
> > >>
> > >> diff --git a/tools/libs/evtchn/minios.c b/tools/libs/evtchn/minios.c
> > >> index 28743cb055..e33ddec7e7 100644
> > >> --- a/tools/libs/evtchn/minios.c
> > >> +++ b/tools/libs/evtchn/minios.c
> > >> @@ -195,7 +195,6 @@ xenevtchn_port_or_error_t xenevtchn_bind_unbound_port(xenevtchn_handle *xce,
> > >>       int ret;
> > >>       evtchn_port_t port;
> > >>
> > >> -    assert(get_current() == main_thread);
> > >>       port_info = port_alloc(xce);
> > >
> > > If multiple threads are allowed, does port_list need to gain a lock
> > > protecting it?
> >
> > I thought of that, too.
> >
> > The answer is: maybe
> >
> > Any other list operation on the list isn't protected by an assert(), so
> > technically there is no real new aspect added in this regard.

I read this as "The others are not protected so let's remove all the
protections"
which sounds really wrong to me.
At least with the existing ASSERT()s there is a chance a user would hit them.

Without any, how would a user be able to know that they are mixing threads?
Where is it documented?

>
> Yes.
>
> > I believe adding a lock would make sense, but it is orthogonal to this
> > patch.
>
> The assert() feels like it was an attempt to avoid introducing
> locking, so I'm not sure it is really orthogonal.

+1. I agree this is not orthogonal.

>
> I was kinda waiting to see if anyone else would lend an opinion.
>
> Since the asserts haven't been tripping there doesn't seem to be an
> issue with the code as-is, so:

The goal of an ASSERTs() is really to never trip in normal circumstances.
So the fact nobody complained until now is a sign that they are working :).

The right course of action is to add more, not less. If there is a problem
with the existing ASSERT(), then the condition should either be updated
as we switch to proper locking.

Cheers,

-- 
Julien Grall