"Bodireddy, Bhanuprakash" <bhanuprakash.bodire...@intel.com> writes:
>>"Bodireddy, Bhanuprakash" <bhanuprakash.bodire...@intel.com> writes: >> >>>>Bhanuprakash Bodireddy <bhanuprakash.bodire...@intel.com> writes: >>>> >>>>> This commit registers the packet processing PMD cores to keepalive >>>>> framework. Only PMDs that have rxqs mapped will be registered and >>>>> actively monitored by KA framework. >>>>> >>>>> This commit spawns a keepalive thread that will dispatch heartbeats >>>>> to PMD cores. The pmd threads respond to heartbeats by marking >>>>> themselves alive. As long as PMD responds to heartbeats it is considered >>'healthy'. >>>>> >>>>> Signed-off-by: Bhanuprakash Bodireddy >>>>> <bhanuprakash.bodire...@intel.com> >>>>> --- >>>> >>>>I'm really confused with this patch. I've stopped reviewing the series. >>>> >>>>It seems like there's a mix of 'track by core id' and 'track by thread id'. >>>> >>>>I don't think it's possible to do anything by core id. We can never >>>>know what else has been scheduled on those cores, and we cannot be >>>>sure that a taskset or other scheduler provisioning call will move the >>threads. >>> >>> [BHANU] I have already answered this in other thread. >>> one can't correlate threads with cores and we shouldn't be tracking by >>> cores. However with PMD threads there is 1:1 mapping of PMD and the >>> core-id and it's safe to temporarily write PMD liveness info in to an >>> array indexed by core id before updating this in to HMAP. >> >>The core-id as a concept here is deceptive. An external entity (such as >>taskset) can rebalance the PMDs. External entities can be scheduled on the >>cores. I think it's dangerous to have anything called core-id in this series >>or >>feature, because people will naturally infer things which aren't true. >>Additionally, it will lead to things like "well, we know that core x,y,z are >>running at A%, so we can schedule things thusly..." >> >>Makes sense? >> > > The concerns above makes sense and you have a valid point. > Unfortunately the logic that you see w.r.t PMD, core_id mapping is something > that was > implemented in rte_keepalive library and I inherited it. As the 1:1 > mapping of a thread(PMD) > to core is deceptive and makes little sense, I reworked on a different > approach with no impact > on datapath performance. I was testing this last few days to check for > perf impacts and other > possible issues. > > Previous design: > -------------------- > As part of heartbeat mechanism (dispath_heartbeats()), in keepalive_info > structure > we had arrays indexed by core-ids used by PMDs and Keepalive thread for > heart-beating. > The arrays were used to keep the logic simple and lock-free. > > Also Keepalive thread was updating the status periodically in to > 'process_list' map using callback function. > > New design: > ------------------- > we already have a 'process_list' map where all the PMD threads are > added by main(vswitchd) > thread. In this new approach, I take a copy of the 'process_list', > let's call it 'cached_process_list' > and use this cached map for heartbeating between Keepalive and PMD > threads. No locks are > needed on the 'cached_process_list' there by not impacting the datapath > performance. > > Also whenever there is datapath reconfiguration(triggered by > pmd-cpu-mask), the 'process_list' map > will be updated and also the cached_process_list will be reloaded from > the main map there by having > the maps in sync. This is handled as part of > ka_register_datapath_threads(). I have been testing this > and seems to be working fine. > > This way we can completely avoid all references to core_id in this > series. Let me know if you have > any comments on this new approach. Sounds good to me. Looking forward to the code :-) > Regards, > Bhanuprakash. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
