Hi, On 25/01/2016 04:00, "Ilya Maximets" <i.maxim...@samsung.com> wrote:
>Thank you. >Comments inline. > >On 23.01.2016 04:21, Daniele Di Proietto wrote: >> Hi Ilya, >> >> Thank you very much for the patch. >> >> I definitely like that the queue assignment is performed by the >> main thread: not only is less bug-prone, but the logic will be more >> easily customizable. >> >> I absolutely welcome the changes to do_add_port and do_del_port to >> keep the queues to the currently assigned threads. >> >> I think we can avoid pausing and resuming the threads each time and, >> instead, leave the current reloading logic unaltered. Here's a way: >> >> * pmd_thread_main() would be identical to master. pmd_load_queues(), >> instead, would return a poll_list by copying the struct rxq_poll >> from 'pmd->poll_list'. >> * do_add_port() and do_del_port() would still write on the pmd >> specific lists while the threads are running. After updating >> a list for a pmd thread, they would call dp_netdev_reload_pmd__(). >> >> This behaviour should still fix the bugs, but it requires less >> sychronization. What do you think? > >Originally there was 3 or 4 ideas of how to implement synchronization >between main and pmd thread. This was one of them. Yes, I agree that >this requires less synchronization. It seemed to me that it is more >difficult to implement. But, apparently, I was wrong. > >> I don't think this should >> create any problems to the following patch, right? > >Yes. Just a little fix. > >> >> I've prepared an incremental on top of this patch to illustrate the >> idea, but other ideas/implementations/fixes are welcome. > >Thanks for this. > >1. >There is a race in your implementation. While loading >queues of newly started pmd threads main thread can modify >poll_list. So, pmd_load_queues must be called with >poll_mutex locked. You're right, thanks for adding this. > >2. >There is a copying of references to rxq_poll in pmd_load_queues(). >I think, ref/unref of corresponding ports needed. Even though not strictly necessary (the main thread waits on the condition variable before unreferencing the port), I agree it's better to have it. > >3. >Having poll_mutex and condition variable in pmd_thread_main >simultaneously is an over-synchronization. As soon as poll_mutex >is mandatory here and we actually don't need to wait for a pmd >thread at some particular point, condition variable may be >removed with dp_netdev_pmd_reload_done() and cond_mutex. >Must be no problems here if port_ref/unref done. You're right, it's not necessary to have both the condition variable and 'poll_mutex' to avoid race conditions between PMD threads. The condition variable is still useful to stop the main thread until the PMDs have actually performed the required operations. We want to return from do_del_port() only when the port has actually been deleted, i.e. when the main thread has dropped the last reference, so I'd prefer we keep it. Now, even though the condition variable might provide enough synchronization on its own, I'd still keep the 'poll_mutex'. What do you think? Daniele _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
