[ovs-dev] [PATCH v4 3/7] dpif-netdev: Register packet processing cores to KA framework.

[Bodireddy, Bhanuprakash]

>"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.

Regards,
Bhanuprakash.