Re: [PATCH] tools/libs/evtchn: drop assert()s in stubdom

[Julien Grall <julien.grall@xxxxxxxxx>]

Le mer. 6 déc. 2023 à 21:03, Jason Andryuk <jandryuk@xxxxxxxxx> a écrit :
>
> On Wed, Dec 6, 2023 at 11:44 AM Juergen Gross <jgross@xxxxxxxx> wrote:
> >
> > On 06.12.23 17:38, Jason Andryuk wrote:
> > > On Wed, Dec 6, 2023 at 9:40 AM Juergen Gross <jgross@xxxxxxxx> 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@xxxxxxxx>
> > >> ---
> > >>   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