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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
