[ovs-dev] [PATCH v3] netdev-dpdk: Set pmd thread priority

[Bodireddy, Bhanuprakash]

Thanks Mark and Daniele, My comments inline.

>
>I agree with Mark's comments, other than that this looks good to me.
>If you agree with the comments would you mind sending an updates version?

I have sent out V4 with the changes suggested by Mark.

>>
>>    * In the absence of pmd-cpu-mask, one pmd thread shall be created
>>      and default scheduling policy and prority gets applied.
>
>Typo above - 'prority'
[OK]

>
>>
>>    * If pmd-cpu-mask is specified, one ore more pmd threads shall be
>
>Typo above - 'ore'
[OK]

>
>>-  A set bit in the mask means a pmd thread is created and pinned
>>-  to the corresponding CPU core. e.g. to run pmd threads on core 1 and 2
>>+  A set bit in the mask means a pmd thread is created, pinned to the
>>+  corresponding CPU core and the scheduling policy SCHED_RR with highest
>>+  priority of the scheduling policy applied to pmd thread.
>>+  e.g. to run pmd threads on core 1 and 2
>There's some repetition in the last paragraph - I'm reviewing this patch in
>isolation, so the text may make sense/be required in the full document.

[BHANU]  This isn't repetition, the changes are going in to 2 different sections and this would make sense when viewing the INSTALL.DPDK-ADVANCED.md.

>
>>
>>   `ovs-vsctl set Open_vSwitch . other_config:pmd-cpu-mask=6`
>>
>>@@ -246,6 +250,9 @@ needs to be affinitized accordingly.
>>
>>   NIC port0 <-> OVS <-> VM <-> OVS <-> NIC port 1
>>
>>+  Note: 'dpdk-lcore-mask' and 'pmd-cpu-mask' cpu mask settings should be
>>+  non-overlapping.
>
>Although it's mentioned in the commit message, it might be worth mentioning
>here the consequences of attempting to pin non-PMD processes to a pmd-
>cpu-mask core (i.e. CPU starvation)

[OK]

>
>>+
>>+void
>>+ovs_numa_thread_setpriority(int policy)
>>+{
>>+    if (dummy_numa) {
>>+        return;
>>+    }
>>+
>>+    struct sched_param threadparam;
>>+    int err;
>>+
>>+    memset(&threadparam, 0, sizeof(threadparam));
>>+    threadparam.sched_priority = sched_get_priority_max(policy);
>>+    err = pthread_setschedparam(pthread_self(), policy, &threadparam);
>>+    if (err) {
>>+        VLOG_ERR("Thread priority error %d",err);
>The convention in this file seems to be to use ovs_strerror when reporting
>errors; suggest that you stick with same.

[AGREE]

Regards,
Bhanu Prakash.