On Fri, Mar 17, 2023 at 07:51:25PM +0100, David Marchand wrote:
> On Fri, Mar 17, 2023 at 3:50 PM Tyler Retzlaff
> <roret...@linux.microsoft.com> wrote:
> > > > -struct thread_routine_ctx {
> > > > +struct thread_start_context {
> > > > rte_thread_func thread_func;
> > > > - void *routine_args;
> > > > + void *thread_args;
> > > > + const rte_thread_attr_t *thread_attr;
> > > > + pthread_mutex_t wrapper_mutex;
> > > > + pthread_cond_t wrapper_cond;
> > > > + int wrapper_ret;
> > > > + volatile int wrapper_done;
> > >
> > > One question.
> > >
> > > I see that wrapper_done is accessed under wrapper_mutex.
> > > Is volatile needed?
> >
> > I'm not entirely certain. i'm being cautious since i can conceive of the
> > load in the loop being optimized as a single load by the compiler. but
> > again i'm not sure, i always like to learn if someone knows better.
>
> After an interesting discussion with Dodji on C99 and side effects
> (5.1.2.3/2 and 5.1.2.3/3), I am a bit more convinced that we don't
> need this volatile.
Thanks for the references, based on the reading i agree we can drop the
volatile.
>
>
> >
> > >
> > > (nit: a boolean is probably enough too)
> >
> > I have no issue with it being a _Bool if you want to adjust it for that
> > i certainly don't object. ordinarily i would use _Bool but a lot of dpdk
> > code seems to prefer int so that's why i chose it. if we use the macro
> > bool then we should include stdbool.h directly into this translation
> > unit.
> >
> > >
> > > I was thinking of squashing below diff:
> >
> > Yeah, no objection. you can decide if you want to keep the volatile or
> > not and add the stdbool.h include.
> >
> > Thanks for reviewing, appreciate it.
>
> This is a fix but this v5 had an additional change in affinity setting
> (switching to rte_thread_set_affinity()).
> To be on the safe side wrt backport, I'll also revert to calling
> rte_thread_set_affinity_by_id as this is what was being used before.
> And this removes the need for patch 1.
Is it worth merging the const patch but not backporting? I'm not fussed
either way.
>
> Sending a v6 soon, so that it goes through the CI before rc3.
Yes, great.
Thanks David!
>
>
> --
> David Marchand
