[ovs-dev] [PATCH v4 3/7] dpif-netdev: Register packet processing cores to KA framework.
Aaron Conole
aconole at redhat.com
Wed Sep 13 16:56:20 UTC 2017
"Bodireddy, Bhanuprakash" <bhanuprakash.bodireddy at intel.com> writes:
>>"Bodireddy, Bhanuprakash" <bhanuprakash.bodireddy at intel.com> writes:
>>
>>>>Bhanuprakash Bodireddy <bhanuprakash.bodireddy at 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.bodireddy at 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.
More information about the dev
mailing list