| Message ID | 1447169760-15258-4-git-send-email-ivan.khoronzhuk@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On 18 November 2015 at 18:09, Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote: > 3patch > > On 10.11.15 17:36, Ivan Khoronzhuk wrote: >> >> By default all control threads on CPU0, and odp_cpumask_default_control By default, all control threads execute on CPU 0 and odp_cpumask_default_control() return this CPU. For reasons of performance, control and worker CPU's shouldn't overlap but for some scenarios it can be desirable to utilize all CPU's for worker threads. Thus we leave the decision of CPU allocation to the user but report when a CPU is used for both control and worker threads. >> returns it. It shouldn't overlap with worker cpumask, but for some >> cases it's correct to use potential processing capability, so better >> leave this choice to application and only draw attention to it when >> cpumask for worker thread is read. >> >> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> Apart from the language, the patch is OK. But I would prefer an update with rephrased description and ODP_DBG message. >> --- >> platform/linux-generic/odp_cpumask_task.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/platform/linux-generic/odp_cpumask_task.c >> b/platform/linux-generic/odp_cpumask_task.c >> index 535891c..b4b4f23 100644 >> --- a/platform/linux-generic/odp_cpumask_task.c >> +++ b/platform/linux-generic/odp_cpumask_task.c >> @@ -40,6 +40,10 @@ int odp_cpumask_default_worker(odp_cpumask_t *mask, int >> num) >> } >> } >> >> + if (odp_cpumask_isset(mask, 0)) >> + ODP_DBG("\n\tCPU0 will be used for control and worker >> thread.\n" >> + "\tIt can have impact on worker thread\n"); I would phrase it like this: "CPU 0 will be used for both control and worker threads, this will likely have a performance impact on the worker thread\n" >> + >> return cpu; >> } >> >> > > -- > Regards, > Ivan Khoronzhuk
On 18.11.15 20:25, Ola Liljedahl wrote: > On 18 November 2015 at 18:09, Ivan Khoronzhuk > <ivan.khoronzhuk@linaro.org> wrote: >> 3patch >> >> On 10.11.15 17:36, Ivan Khoronzhuk wrote: >>> >>> By default all control threads on CPU0, and odp_cpumask_default_control > By default, all control threads execute on CPU 0 and > odp_cpumask_default_control() > return this CPU. For reasons of performance, control and worker CPU's shouldn't > overlap but for some scenarios it can be desirable to utilize all > CPU's for worker threads. > Thus we leave the decision of CPU allocation to the user but report > when a CPU is > used for both control and worker threads. > >>> returns it. It shouldn't overlap with worker cpumask, but for some >>> cases it's correct to use potential processing capability, so better >>> leave this choice to application and only draw attention to it when >>> cpumask for worker thread is read. >>> >>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> > Apart from the language, the patch is OK. But I would prefer an update > with rephrased description and ODP_DBG message. Ok. next version will correct it as proposed. > >>> --- >>> platform/linux-generic/odp_cpumask_task.c | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/platform/linux-generic/odp_cpumask_task.c >>> b/platform/linux-generic/odp_cpumask_task.c >>> index 535891c..b4b4f23 100644 >>> --- a/platform/linux-generic/odp_cpumask_task.c >>> +++ b/platform/linux-generic/odp_cpumask_task.c >>> @@ -40,6 +40,10 @@ int odp_cpumask_default_worker(odp_cpumask_t *mask, int >>> num) >>> } >>> } >>> >>> + if (odp_cpumask_isset(mask, 0)) >>> + ODP_DBG("\n\tCPU0 will be used for control and worker >>> thread.\n" >>> + "\tIt can have impact on worker thread\n"); > I would phrase it like this: > "CPU 0 will be used for both control and worker threads, this will > likely have a performance impact on the worker thread\n" > Ok. >>> + >>> return cpu; >>> } >>> >>> >> >> -- >> Regards, >> Ivan Khoronzhuk
sent v3 with proposed corrections. On 18.11.15 20:34, Ivan Khoronzhuk wrote: > > > On 18.11.15 20:25, Ola Liljedahl wrote: >> On 18 November 2015 at 18:09, Ivan Khoronzhuk >> <ivan.khoronzhuk@linaro.org> wrote: >>> 3patch >>> >>> On 10.11.15 17:36, Ivan Khoronzhuk wrote: >>>> >>>> By default all control threads on CPU0, and odp_cpumask_default_control >> By default, all control threads execute on CPU 0 and >> odp_cpumask_default_control() >> return this CPU. For reasons of performance, control and worker CPU's shouldn't >> overlap but for some scenarios it can be desirable to utilize all >> CPU's for worker threads. >> Thus we leave the decision of CPU allocation to the user but report >> when a CPU is >> used for both control and worker threads. >> >>>> returns it. It shouldn't overlap with worker cpumask, but for some >>>> cases it's correct to use potential processing capability, so better >>>> leave this choice to application and only draw attention to it when >>>> cpumask for worker thread is read. >>>> >>>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> >> Apart from the language, the patch is OK. But I would prefer an update >> with rephrased description and ODP_DBG message. > > Ok. next version will correct it as proposed. > >> >>>> --- >>>> platform/linux-generic/odp_cpumask_task.c | 4 ++++ >>>> 1 file changed, 4 insertions(+) >>>> >>>> diff --git a/platform/linux-generic/odp_cpumask_task.c >>>> b/platform/linux-generic/odp_cpumask_task.c >>>> index 535891c..b4b4f23 100644 >>>> --- a/platform/linux-generic/odp_cpumask_task.c >>>> +++ b/platform/linux-generic/odp_cpumask_task.c >>>> @@ -40,6 +40,10 @@ int odp_cpumask_default_worker(odp_cpumask_t *mask, int >>>> num) >>>> } >>>> } >>>> >>>> + if (odp_cpumask_isset(mask, 0)) >>>> + ODP_DBG("\n\tCPU0 will be used for control and worker >>>> thread.\n" >>>> + "\tIt can have impact on worker thread\n"); >> I would phrase it like this: >> "CPU 0 will be used for both control and worker threads, this will >> likely have a performance impact on the worker thread\n" >> > Ok. > > >>>> + >>>> return cpu; >>>> } >>>> >>>> >>> >>> -- >>> Regards, >>> Ivan Khoronzhuk >
diff --git a/platform/linux-generic/odp_cpumask_task.c b/platform/linux-generic/odp_cpumask_task.c index 535891c..b4b4f23 100644 --- a/platform/linux-generic/odp_cpumask_task.c +++ b/platform/linux-generic/odp_cpumask_task.c @@ -40,6 +40,10 @@ int odp_cpumask_default_worker(odp_cpumask_t *mask, int num) } } + if (odp_cpumask_isset(mask, 0)) + ODP_DBG("\n\tCPU0 will be used for control and worker thread.\n" + "\tIt can have impact on worker thread\n"); + return cpu; }
By default all control threads on CPU0, and odp_cpumask_default_control returns it. It shouldn't overlap with worker cpumask, but for some cases it's correct to use potential processing capability, so better leave this choice to application and only draw attention to it when cpumask for worker thread is read. Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> --- platform/linux-generic/odp_cpumask_task.c | 4 ++++ 1 file changed, 4 insertions(+)