On 26.01.2016 03:21, Daniele Di Proietto wrote: > 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?
Yes. I agree that without the condition variable will be a problem with the quick addition of deleted ports. I believe that there may be a simpler solution. But I do not see it now. Maybe I'll fix it in the future. For now, I'll keep the condition variable. Best regards, Ilya Maximets. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
