[ovs-dev] [PATCH v6 1/7] dpif-netdev: Refactor PMD thread structure for further extension.
Eelco Chaudron
echaudro at redhat.com
Thu Dec 7 16:01:02 UTC 2017
On 07/12/17 14:28, Ilya Maximets wrote:
> Thanks for review, comments inline.
>
> On 07.12.2017 15:49, Eelco Chaudron wrote:
>> On 01/12/17 16:44, Ilya Maximets wrote:
>>> This is preparation for 'struct dp_netdev_pmd_thread' modification
>>> in upcoming commits. Needed to avoid reordering and regrouping while
>>> replacing old and adding new members.
>>>
>> Should this be part of the TX batching set? Anyway, I'm ok if it's not stalling the approval :)
> Unfortunately yes, because members reordered and regrouped just to include
> new members: pmd->ctx and pmd->n_output_batches. This could not be a standalone
> change because adding of different members will require different regrouping/
> reordering. I moved this change to a separate patch to not do this twice while
> adding each member in patches 2/7 and 6/7.
>
> Anyway, as I mentioned in cover letter, I still prefer reverting of the padding
> at all by this patch:
> https://mail.openvswitch.org/pipermail/ovs-dev/2017-November/341153.html
Agree, see reply to Jan's email.
>> See some comments below on the use of remaining padding...
>>> Signed-off-by: Bhanuprakash Bodireddy <bhanuprakash.bodireddy at intel.com>
>>> Co-authored-by: Bhanuprakash Bodireddy <bhanuprakash.bodireddy at intel.com>
>>> Signed-off-by: Ilya Maximets <i.maximets at samsung.com>
>>> ---
>>> lib/dpif-netdev.c | 65 ++++++++++++++++++++++++++++++++++---------------------
>>> 1 file changed, 40 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>>> index 0a62630..7a7c6ce 100644
>>> --- a/lib/dpif-netdev.c
>>> +++ b/lib/dpif-netdev.c
>>> @@ -549,29 +549,22 @@ struct tx_port {
>>> struct dp_netdev_pmd_thread {
>>> PADDED_MEMBERS_CACHELINE_MARKER(CACHE_LINE_SIZE, cacheline0,
>>> struct dp_netdev *dp;
>>> - struct cmap_node node; /* In 'dp->poll_threads'. */
>>> - pthread_cond_t cond; /* For synchronizing pmd thread
>>> - reload. */
>>> + struct cmap_node node; /* In 'dp->poll_threads'. */
>>> + pthread_cond_t cond; /* For synchronizing pmd thread reload. */
>>> );
>>> PADDED_MEMBERS_CACHELINE_MARKER(CACHE_LINE_SIZE, cacheline1,
>>> struct ovs_mutex cond_mutex; /* Mutex for condition variable. */
>>> pthread_t thread;
>>> - unsigned core_id; /* CPU core id of this pmd thread. */
>>> - int numa_id; /* numa node id of this pmd thread. */
>>> );
>>> /* Per thread exact-match cache. Note, the instance for cpu core
>>> * NON_PMD_CORE_ID can be accessed by multiple threads, and thusly
>>> * need to be protected by 'non_pmd_mutex'. Every other instance
>>> * will only be accessed by its own pmd thread. */
>>> - OVS_ALIGNED_VAR(CACHE_LINE_SIZE) struct emc_cache flow_cache;
>>> - struct ovs_refcount ref_cnt; /* Every reference must be refcount'ed. */
>>> -
>>> - /* Queue id used by this pmd thread to send packets on all netdevs if
>>> - * XPS disabled for this netdev. All static_tx_qid's are unique and less
>>> - * than 'cmap_count(dp->poll_threads)'. */
>>> - uint32_t static_tx_qid;
>>> + PADDED_MEMBERS(CACHE_LINE_SIZE,
>>> + OVS_ALIGNED_VAR(CACHE_LINE_SIZE) struct emc_cache flow_cache;
>>> + );
>>> /* Flow-Table and classifiers
>>> *
>>> @@ -579,7 +572,10 @@ struct dp_netdev_pmd_thread {
>>> * changes to 'classifiers' must be made while still holding the
>>> * 'flow_mutex'.
>>> */
>>> - struct ovs_mutex flow_mutex;
>>> + PADDED_MEMBERS(CACHE_LINE_SIZE,
>>> + struct ovs_mutex flow_mutex;
>>> + /* 8 pad bytes. */
>> Do we really want to add pad bytes left in this structure? They can easily be wrong is something changes elsewhere?
>> Guessing from some the differences you might have other patches applied?
>> Using the pahole tool I think the 8 here should be 16?
>>
>> union {
>> struct {
>> struct ovs_mutex flow_mutex; /* 0 48 */
>> }; /* 48 */
>> uint8_t pad12[64]; /* 64 */
>> }; /* 0 64 */
> I left 8 bytes here because we may have different size of ovs_mutex on different
> systems. The biggest value I saw was 56 bytes on my ARMv8 platform.
>
>>> + );
>>> PADDED_MEMBERS(CACHE_LINE_SIZE,
>>> struct cmap flow_table OVS_GUARDED; /* Flow table. */
>>> @@ -596,35 +592,50 @@ struct dp_netdev_pmd_thread {
>>> /* Used to count cycles. See 'cycles_counter_end()'. */
>>> unsigned long long last_cycles;
>>> - struct latch exit_latch; /* For terminating the pmd thread. */
>>> - );
>>> + /* 8 pad bytes. */
>>> + );
>>> PADDED_MEMBERS(CACHE_LINE_SIZE,
>>> /* Statistics. */
>>> struct dp_netdev_pmd_stats stats;
>>> + /* 8 pad bytes. */
>> Should this not be 24?
> Yes, you're right. It'll be 8 after applying of the last patch of the series.
> I can keep 24 in this patch and change this value to 8 at the last patch if
> it really matters. Do you think I should?
Don't think it should. I feel more like removing it at all as it assumes
CACHE_LINE_SIZE is always 64
>
>> union {
>> struct {
>> struct dp_netdev_pmd_stats stats; /* 0 40 */
>> }; /* 40 */
>> uint8_t pad14[64]; /* 64 */
>> }; /* 0 64
>>
>>> + );
>>> + PADDED_MEMBERS(CACHE_LINE_SIZE,
>>> + struct latch exit_latch; /* For terminating the pmd thread. */
>>> struct seq *reload_seq;
>>> uint64_t last_reload_seq;
>>> - atomic_bool reload; /* Do we need to reload ports? */
>>> - bool isolated;
>>> -
>>> + atomic_bool reload; /* Do we need to reload ports? */
>>> /* Set to true if the pmd thread needs to be reloaded. */
>>> bool need_reload;
>>> - /* 5 pad bytes. */
>>> + bool isolated;
>>> +
>>> + struct ovs_refcount ref_cnt; /* Every reference must be refcount'ed. */
>>> +
>>> + /* Queue id used by this pmd thread to send packets on all netdevs if
>>> + * XPS disabled for this netdev. All static_tx_qid's are unique and
>>> + * less than 'cmap_count(dp->poll_threads)'. */
>>> + uint32_t static_tx_qid;
>>> +
>>> + unsigned core_id; /* CPU core id of this pmd thread. */
>>> + int numa_id; /* numa node id of this pmd thread. */
>>> +
>>> + /* 20 pad bytes. */
>>> );
>>> PADDED_MEMBERS(CACHE_LINE_SIZE,
>>> - struct ovs_mutex port_mutex; /* Mutex for 'poll_list'
>>> - and 'tx_ports'. */
>>> - /* 16 pad bytes. */
>>> + /* Mutex for 'poll_list' and 'tx_ports'. */
>>> + struct ovs_mutex port_mutex;
>>> );
>>> +
>>> PADDED_MEMBERS(CACHE_LINE_SIZE,
>>> /* List of rx queues to poll. */
>>> struct hmap poll_list OVS_GUARDED;
>>> - /* Map of 'tx_port's used for transmission. Written by the main
>>> - * thread, read by the pmd thread. */
>>> + /* Map of 'tx_port's used for transmission.
>>> + * Written by the main thread, read by the pmd thread. */
>>> struct hmap tx_ports OVS_GUARDED;
>>> );
>>> +
>>> PADDED_MEMBERS(CACHE_LINE_SIZE,
>>> /* These are thread-local copies of 'tx_ports'. One contains only
>>> * tunnel ports (that support push_tunnel/pop_tunnel), the other
>>> @@ -648,9 +659,13 @@ struct dp_netdev_pmd_thread {
>>> * values and subtracts them from 'stats' and 'cycles' before
>>> * reporting to the user */
>>> unsigned long long stats_zero[DP_N_STATS];
>>> - uint64_t cycles_zero[PMD_N_CYCLES];
>>> /* 8 pad bytes. */
>>> );
>>> +
>>> + PADDED_MEMBERS(CACHE_LINE_SIZE,
>>> + uint64_t cycles_zero[PMD_N_CYCLES];
>>> + /* 48 pad bytes. */
>>> + );
>>> };
>>> /* Interface to netdev-based datapath. */
>>
>>
>>
>>
More information about the dev
mailing list