| Message ID | 20231206144009.29154-1-jgross@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tools/libs/evtchn: drop assert()s in stubdom | expand |
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,
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;
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(-)