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-boun...@openvswitch.org 
>> [mailto:ovs-dev-boun...@openvswitch.org] On Behalf Of Ilya Maximets
>> Sent: Thursday, 10 August, 2017 18:55
>> To: ovs-dev@openvswitch.org
>> Cc: Ilya Maximets <i.maxim...@samsung.com>; Heetae Ahn 
>> <heetae82....@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.maxim...@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
>> d...@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> 
> 
> 
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to