[ovs-dev] [PATCH 1/2] dpif-netdev: Keep latest measured time for PMD thread.
Ilya Maximets
i.maximets at samsung.com
Fri Aug 11 13:15:43 UTC 2017
On 11.08.2017 11:13, Jan Scheurich wrote:
> Hi Ilya,
>
> I fully agree with storing 'now' as part of the pmd struct instead of passing it around as function arguments.
>
> For me struct dp_netdev_pmd_thread is *the* the PMD thread context in dpif-netdev. I don't really see the benefit of creating a sub-struct dp_netdev_pmd_thread_ctx and moving certain data into that. Can you explain your motivation?
Hello Jan,
IMHO all other fields in struct dp_netdev_pmd_thread has direct relation with the
thread itself (like core_id, need_reload) or they are it's direct accessories
(like mutexes, stats, caches, lists of polled ports). From the other hand, time and
last_cycles has no actual relation with thread and can be moved out of struct
dp_netdev_pmd_thread wihtout any logical issues. These fields are characteristics
of the outside environment. For example, 'emc_insert_min' which I'm introducing in
the next patch is actually the characteristics of the last received packet (maybe port)
but not the thread itself.
>
> BR, Jan
>
>> -----Original Message-----
>> From: ovs-dev-bounces at openvswitch.org [mailto:ovs-dev-bounces at openvswitch.org] On Behalf Of Ilya Maximets
>> Sent: Thursday, 10 August, 2017 18:55
>> To: ovs-dev at openvswitch.org
>> Cc: Ilya Maximets <i.maximets at samsung.com>; Heetae Ahn <heetae82.ahn at samsung.com>
>> Subject: [ovs-dev] [PATCH 1/2] dpif-netdev: Keep latest measured time for PMD thread.
>>
>> In current implementation 'now' variable updated once on each
>> receive cycle and passed through the whole datapath via function
>> arguments. It'll be better to keep this variable inside PMD
>> thread structure to be able to get it at any time. Such solution
>> will save the stack memory and simplify possible modifications
>> in current logic.
>>
>> This patch introduces new structure 'dp_netdev_pmd_thread_ctx'
>> contained by 'struct dp_netdev_pmd_thread' to store any processing
>> context of this PMD thread. For now, only time and cycles moved to
>> that structure. Can be extended in the future.
>>
>> Signed-off-by: Ilya Maximets <i.maximets at samsung.com>
>> ---
>>
>> Will be used in the next patch to not pass the probability
>> through the whole datapath.
>>
>> lib/dpif-netdev.c | 117 ++++++++++++++++++++++++++++++------------------------
>> 1 file changed, 65 insertions(+), 52 deletions(-)
>>
>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>> index e2cd931..6d42393 100644
>> --- a/lib/dpif-netdev.c
>> +++ b/lib/dpif-netdev.c
>> @@ -504,6 +504,16 @@ struct tx_port {
>> struct hmap_node node;
>> };
>>
>> +/* Contained by struct dp_netdev_pmd_thread's 'ctx' member. */
>> +struct dp_netdev_pmd_thread_ctx {
>> + /* Latest measured time. */
>> + long long now;
>> + /* Used to count cycles. See 'cycles_count_end()' */
>> + unsigned long long last_cycles;
>> +};
>> +
>> +static void pmd_thread_ctx_time_update(struct dp_netdev_pmd_thread *);
>> +
>> /* PMD: Poll modes drivers. PMD accesses devices via polling to eliminate
>> * the performance overhead of interrupt processing. Therefore netdev can
>> * not implement rx-wait for these devices. dpif-netdev needs to poll
>> @@ -556,8 +566,8 @@ struct dp_netdev_pmd_thread {
>> /* Cycles counters */
>> struct dp_netdev_pmd_cycles cycles;
>>
>> - /* Used to count cicles. See 'cycles_counter_end()' */
>> - unsigned long long last_cycles;
>> + /* Current context of the PMD thread. */
>> + struct dp_netdev_pmd_thread_ctx ctx;
>>
>> struct latch exit_latch; /* For terminating the pmd thread. */
>> struct seq *reload_seq;
>> @@ -630,8 +640,7 @@ static void dp_netdev_execute_actions(struct dp_netdev_pmd_thread *pmd,
>> struct dp_packet_batch *,
>> bool may_steal, const struct flow *flow,
>> const struct nlattr *actions,
>> - size_t actions_len,
>> - long long now);
>> + size_t actions_len);
>> static void dp_netdev_input(struct dp_netdev_pmd_thread *,
>> struct dp_packet_batch *, odp_port_t port_no);
>> static void dp_netdev_recirculate(struct dp_netdev_pmd_thread *,
>> @@ -679,9 +688,9 @@ dp_netdev_pmd_try_optimize(struct dp_netdev_pmd_thread *pmd);
>>
>> static void
>> dpif_netdev_xps_revalidate_pmd(const struct dp_netdev_pmd_thread *pmd,
>> - long long now, bool purge);
>> + bool purge);
>> static int dpif_netdev_xps_get_tx_qid(const struct dp_netdev_pmd_thread *pmd,
>> - struct tx_port *tx, long long now);
>> + struct tx_port *tx);
>>
>> static inline bool emc_entry_alive(struct emc_entry *ce);
>> static void emc_clear_entry(struct emc_entry *ce);
>> @@ -723,6 +732,12 @@ emc_cache_slow_sweep(struct emc_cache *flow_cache)
>> flow_cache->sweep_idx = (flow_cache->sweep_idx + 1) & EM_FLOW_HASH_MASK;
>> }
>>
>> +static inline void
>> +pmd_thread_ctx_time_update(struct dp_netdev_pmd_thread *pmd)
>> +{
>> + pmd->ctx.now = time_msec();
>> +}
>> +
>> /* Returns true if 'dpif' is a netdev or dummy dpif, false otherwise. */
>> bool
>> dpif_is_netdev(const struct dpif *dpif)
>> @@ -2839,6 +2854,9 @@ dpif_netdev_execute(struct dpif *dpif, struct dpif_execute *execute)
>> ovs_mutex_lock(&dp->non_pmd_mutex);
>> }
>>
>> + /* Update current time in PMD context. */
>> + pmd_thread_ctx_time_update(pmd);
>> +
>> /* The action processing expects the RSS hash to be valid, because
>> * it's always initialized at the beginning of datapath processing.
>> * In this case, though, 'execute->packet' may not have gone through
>> @@ -2851,8 +2869,7 @@ dpif_netdev_execute(struct dpif *dpif, struct dpif_execute *execute)
>>
>> dp_packet_batch_init_packet(&pp, execute->packet);
>> dp_netdev_execute_actions(pmd, &pp, false, execute->flow,
>> - execute->actions, execute->actions_len,
>> - time_msec());
>> + execute->actions, execute->actions_len);
>>
>> if (pmd->core_id == NON_PMD_CORE_ID) {
>> ovs_mutex_unlock(&dp->non_pmd_mutex);
>> @@ -3073,7 +3090,7 @@ cycles_count_start(struct dp_netdev_pmd_thread *pmd)
>> OVS_ACQUIRES(&cycles_counter_fake_mutex)
>> OVS_NO_THREAD_SAFETY_ANALYSIS
>> {
>> - pmd->last_cycles = cycles_counter();
>> + pmd->ctx.last_cycles = cycles_counter();
>> }
>>
>> /* Stop counting cycles and add them to the counter 'type' */
>> @@ -3083,7 +3100,7 @@ cycles_count_end(struct dp_netdev_pmd_thread *pmd,
>> OVS_RELEASES(&cycles_counter_fake_mutex)
>> OVS_NO_THREAD_SAFETY_ANALYSIS
>> {
>> - unsigned long long interval = cycles_counter() - pmd->last_cycles;
>> + unsigned long long interval = cycles_counter() - pmd->ctx.last_cycles;
>>
>> non_atomic_ullong_add(&pmd->cycles.n[type], interval);
>> }
>> @@ -3095,8 +3112,8 @@ cycles_count_intermediate(struct dp_netdev_pmd_thread *pmd,
>> OVS_NO_THREAD_SAFETY_ANALYSIS
>> {
>> unsigned long long new_cycles = cycles_counter();
>> - unsigned long long interval = new_cycles - pmd->last_cycles;
>> - pmd->last_cycles = new_cycles;
>> + unsigned long long interval = new_cycles - pmd->ctx.last_cycles;
>> + pmd->ctx.last_cycles = new_cycles;
>>
>> non_atomic_ullong_add(&pmd->cycles.n[type], interval);
>> }
>> @@ -3674,7 +3691,8 @@ dpif_netdev_run(struct dpif *dpif)
>> }
>> }
>> cycles_count_end(non_pmd, PMD_CYCLES_IDLE);
>> - dpif_netdev_xps_revalidate_pmd(non_pmd, time_msec(), false);
>> + pmd_thread_ctx_time_update(non_pmd);
>> + dpif_netdev_xps_revalidate_pmd(non_pmd, false);
>> ovs_mutex_unlock(&dp->non_pmd_mutex);
>>
>> dp_netdev_pmd_unref(non_pmd);
>> @@ -3725,7 +3743,7 @@ pmd_free_cached_ports(struct dp_netdev_pmd_thread *pmd)
>> struct tx_port *tx_port_cached;
>>
>> /* Free all used tx queue ids. */
>> - dpif_netdev_xps_revalidate_pmd(pmd, 0, true);
>> + dpif_netdev_xps_revalidate_pmd(pmd, true);
>>
>> HMAP_FOR_EACH_POP (tx_port_cached, node, &pmd->tnl_port_cache) {
>> free(tx_port_cached);
>> @@ -4308,7 +4326,8 @@ dp_netdev_configure_pmd(struct dp_netdev_pmd_thread *pmd, struct dp_netdev *dp,
>> ovs_mutex_init(&pmd->port_mutex);
>> cmap_init(&pmd->flow_table);
>> cmap_init(&pmd->classifiers);
>> - pmd->next_optimization = time_msec() + DPCLS_OPTIMIZATION_INTERVAL;
>> + pmd_thread_ctx_time_update(pmd);
>> + pmd->next_optimization = pmd->ctx.now + DPCLS_OPTIMIZATION_INTERVAL;
>> hmap_init(&pmd->poll_list);
>> hmap_init(&pmd->tx_ports);
>> hmap_init(&pmd->tnl_port_cache);
>> @@ -4621,19 +4640,18 @@ packet_batch_per_flow_init(struct packet_batch_per_flow *batch,
>>
>> static inline void
>> packet_batch_per_flow_execute(struct packet_batch_per_flow *batch,
>> - struct dp_netdev_pmd_thread *pmd,
>> - long long now)
>> + struct dp_netdev_pmd_thread *pmd)
>> {
>> struct dp_netdev_actions *actions;
>> struct dp_netdev_flow *flow = batch->flow;
>>
>> dp_netdev_flow_used(flow, batch->array.count, batch->byte_count,
>> - batch->tcp_flags, now);
>> + batch->tcp_flags, pmd->ctx.now);
>>
>> actions = dp_netdev_flow_get_actions(flow);
>>
>> dp_netdev_execute_actions(pmd, &batch->array, true, &flow->flow,
>> - actions->actions, actions->size, now);
>> + actions->actions, actions->size);
>> }
>>
>> static inline void
>> @@ -4730,7 +4748,7 @@ handle_packet_upcall(struct dp_netdev_pmd_thread *pmd,
>> struct dp_packet *packet,
>> const struct netdev_flow_key *key,
>> struct ofpbuf *actions, struct ofpbuf *put_actions,
>> - int *lost_cnt, long long now)
>> + int *lost_cnt)
>> {
>> struct ofpbuf *add_actions;
>> struct dp_packet_batch b;
>> @@ -4769,7 +4787,7 @@ handle_packet_upcall(struct dp_netdev_pmd_thread *pmd,
>> * we'll send the packet up twice. */
>> dp_packet_batch_init_packet(&b, packet);
>> dp_netdev_execute_actions(pmd, &b, true, &match.flow,
>> - actions->data, actions->size, now);
>> + actions->data, actions->size);
>>
>> add_actions = put_actions->size ? put_actions : actions;
>> if (OVS_LIKELY(error != ENOSPC)) {
>> @@ -4797,9 +4815,9 @@ static inline void
>> fast_path_processing(struct dp_netdev_pmd_thread *pmd,
>> struct dp_packet_batch *packets_,
>> struct netdev_flow_key *keys,
>> - struct packet_batch_per_flow batches[], size_t *n_batches,
>> - odp_port_t in_port,
>> - long long now)
>> + struct packet_batch_per_flow batches[],
>> + size_t *n_batches,
>> + odp_port_t in_port)
>> {
>> int cnt = packets_->count;
>> #if !defined(__CHECKER__) && !defined(_WIN32)
>> @@ -4856,7 +4874,7 @@ fast_path_processing(struct dp_netdev_pmd_thread *pmd,
>>
>> miss_cnt++;
>> handle_packet_upcall(pmd, packets[i], &keys[i], &actions,
>> - &put_actions, &lost_cnt, now);
>> + &put_actions, &lost_cnt);
>> }
>>
>> ofpbuf_uninit(&actions);
>> @@ -4913,18 +4931,19 @@ dp_netdev_input__(struct dp_netdev_pmd_thread *pmd,
>> OVS_ALIGNED_VAR(CACHE_LINE_SIZE)
>> struct netdev_flow_key keys[PKT_ARRAY_SIZE];
>> struct packet_batch_per_flow batches[PKT_ARRAY_SIZE];
>> - long long now = time_msec();
>> size_t n_batches;
>> odp_port_t in_port;
>>
>> + pmd_thread_ctx_time_update(pmd);
>> +
>> n_batches = 0;
>> emc_processing(pmd, packets, keys, batches, &n_batches,
>> md_is_valid, port_no);
>> if (!dp_packet_batch_is_empty(packets)) {
>> /* Get ingress port from first packet's metadata. */
>> in_port = packets->packets[0]->md.in_port.odp_port;
>> - fast_path_processing(pmd, packets, keys, batches, &n_batches,
>> - in_port, now);
>> + fast_path_processing(pmd, packets, keys,
>> + batches, &n_batches, in_port);
>> }
>>
>> /* All the flow batches need to be reset before any call to
>> @@ -4942,7 +4961,7 @@ dp_netdev_input__(struct dp_netdev_pmd_thread *pmd,
>> }
>>
>> for (i = 0; i < n_batches; i++) {
>> - packet_batch_per_flow_execute(&batches[i], pmd, now);
>> + packet_batch_per_flow_execute(&batches[i], pmd);
>> }
>> }
>>
>> @@ -4963,7 +4982,6 @@ dp_netdev_recirculate(struct dp_netdev_pmd_thread *pmd,
>>
>> struct dp_netdev_execute_aux {
>> struct dp_netdev_pmd_thread *pmd;
>> - long long now;
>> const struct flow *flow;
>> };
>>
>> @@ -4987,7 +5005,7 @@ dpif_netdev_register_upcall_cb(struct dpif *dpif, upcall_callback *cb,
>>
>> static void
>> dpif_netdev_xps_revalidate_pmd(const struct dp_netdev_pmd_thread *pmd,
>> - long long now, bool purge)
>> + bool purge)
>> {
>> struct tx_port *tx;
>> struct dp_netdev_port *port;
>> @@ -4997,7 +5015,7 @@ dpif_netdev_xps_revalidate_pmd(const struct dp_netdev_pmd_thread *pmd,
>> if (!tx->port->dynamic_txqs) {
>> continue;
>> }
>> - interval = now - tx->last_used;
>> + interval = pmd->ctx.now - tx->last_used;
>> if (tx->qid >= 0 && (purge || interval >= XPS_TIMEOUT_MS)) {
>> port = tx->port;
>> ovs_mutex_lock(&port->txq_used_mutex);
>> @@ -5010,18 +5028,14 @@ dpif_netdev_xps_revalidate_pmd(const struct dp_netdev_pmd_thread *pmd,
>>
>> static int
>> dpif_netdev_xps_get_tx_qid(const struct dp_netdev_pmd_thread *pmd,
>> - struct tx_port *tx, long long now)
>> + struct tx_port *tx)
>> {
>> struct dp_netdev_port *port;
>> long long interval;
>> int i, min_cnt, min_qid;
>>
>> - if (OVS_UNLIKELY(!now)) {
>> - now = time_msec();
>> - }
>> -
>> - interval = now - tx->last_used;
>> - tx->last_used = now;
>> + interval = pmd->ctx.now - tx->last_used;
>> + tx->last_used = pmd->ctx.now;
>>
>> if (OVS_LIKELY(tx->qid >= 0 && interval < XPS_TIMEOUT_MS)) {
>> return tx->qid;
>> @@ -5049,7 +5063,7 @@ dpif_netdev_xps_get_tx_qid(const struct dp_netdev_pmd_thread *pmd,
>>
>> ovs_mutex_unlock(&port->txq_used_mutex);
>>
>> - dpif_netdev_xps_revalidate_pmd(pmd, now, false);
>> + dpif_netdev_xps_revalidate_pmd(pmd, false);
>>
>> VLOG_DBG("Core %d: New TX queue ID %d for port \'%s\'.",
>> pmd->core_id, tx->qid, netdev_get_name(tx->port->netdev));
>> @@ -5100,7 +5114,7 @@ dp_execute_userspace_action(struct dp_netdev_pmd_thread *pmd,
>> struct dp_packet *packet, bool may_steal,
>> struct flow *flow, ovs_u128 *ufid,
>> struct ofpbuf *actions,
>> - const struct nlattr *userdata, long long now)
>> + const struct nlattr *userdata)
>> {
>> struct dp_packet_batch b;
>> int error;
>> @@ -5113,7 +5127,7 @@ dp_execute_userspace_action(struct dp_netdev_pmd_thread *pmd,
>> if (!error || error == ENOSPC) {
>> dp_packet_batch_init_packet(&b, packet);
>> dp_netdev_execute_actions(pmd, &b, may_steal, flow,
>> - actions->data, actions->size, now);
>> + actions->data, actions->size);
>> } else if (may_steal) {
>> dp_packet_delete(packet);
>> }
>> @@ -5129,7 +5143,6 @@ dp_execute_cb(void *aux_, struct dp_packet_batch *packets_,
>> struct dp_netdev_pmd_thread *pmd = aux->pmd;
>> struct dp_netdev *dp = pmd->dp;
>> int type = nl_attr_type(a);
>> - long long now = aux->now;
>> struct tx_port *p;
>>
>> switch ((enum ovs_action_attr)type) {
>> @@ -5141,7 +5154,7 @@ dp_execute_cb(void *aux_, struct dp_packet_batch *packets_,
>>
>> dynamic_txqs = p->port->dynamic_txqs;
>> if (dynamic_txqs) {
>> - tx_qid = dpif_netdev_xps_get_tx_qid(pmd, p, now);
>> + tx_qid = dpif_netdev_xps_get_tx_qid(pmd, p);
>> } else {
>> tx_qid = pmd->static_tx_qid;
>> }
>> @@ -5224,7 +5237,7 @@ dp_execute_cb(void *aux_, struct dp_packet_batch *packets_,
>> flow_extract(packet, &flow);
>> dpif_flow_hash(dp->dpif, &flow, sizeof flow, &ufid);
>> dp_execute_userspace_action(pmd, packet, may_steal, &flow,
>> - &ufid, &actions, userdata, now);
>> + &ufid, &actions, userdata);
>> }
>>
>> if (clone) {
>> @@ -5391,7 +5404,7 @@ dp_execute_cb(void *aux_, struct dp_packet_batch *packets_,
>>
>> case OVS_ACTION_ATTR_METER:
>> dp_netdev_run_meter(pmd->dp, packets_, nl_attr_get_u32(a),
>> - time_msec());
>> + pmd->ctx.now);
>> break;
>>
>> case OVS_ACTION_ATTR_PUSH_VLAN:
>> @@ -5420,10 +5433,9 @@ static void
>> dp_netdev_execute_actions(struct dp_netdev_pmd_thread *pmd,
>> struct dp_packet_batch *packets,
>> bool may_steal, const struct flow *flow,
>> - const struct nlattr *actions, size_t actions_len,
>> - long long now)
>> + const struct nlattr *actions, size_t actions_len)
>> {
>> - struct dp_netdev_execute_aux aux = { pmd, now, flow };
>> + struct dp_netdev_execute_aux aux = { pmd, flow };
>>
>> odp_execute_actions(&aux, packets, may_steal, actions,
>> actions_len, dp_execute_cb);
>> @@ -5749,9 +5761,9 @@ static inline void
>> dp_netdev_pmd_try_optimize(struct dp_netdev_pmd_thread *pmd)
>> {
>> struct dpcls *cls;
>> - long long int now = time_msec();
>>
>> - if (now > pmd->next_optimization) {
>> + pmd_thread_ctx_time_update(pmd);
>> + if (pmd->ctx.now > pmd->next_optimization) {
>> /* Try to obtain the flow lock to block out revalidator threads.
>> * If not possible, just try next time. */
>> if (!ovs_mutex_trylock(&pmd->flow_mutex)) {
>> @@ -5761,7 +5773,8 @@ dp_netdev_pmd_try_optimize(struct dp_netdev_pmd_thread *pmd)
>> }
>> ovs_mutex_unlock(&pmd->flow_mutex);
>> /* Start new measuring interval */
>> - pmd->next_optimization = now + DPCLS_OPTIMIZATION_INTERVAL;
>> + pmd->next_optimization = pmd->ctx.now
>> + + DPCLS_OPTIMIZATION_INTERVAL;
>> }
>> }
>> }
>> --
>> 2.7.4
>>
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
>
More information about the dev
mailing list