> On 28.07.2017 10:20, Darrell Ball wrote: > > I have not tested yet > > > > However, I would have expected something max latency config. to be >specific to netdev-dpdk port types > > IMHO, if we can make it generic, we must make it generic. > >[Darrell] >The first question I ask myself is - is this functionality intrinsically >generic or is >it not ? >It is clearly not and trying to make it artificially so would do the following: > >1) We end up designing something the wrong way where it partially works. >2) Breaks other features present and future that really do intersect. > > > Making of this > functionality netdev-dpdk specific will brake ability to test it using > unit tests. As the change is complex and has a lot of pitfalls like > possible packet stucks and possible latency issues, this code should be > covered by unit tests to simplify the support and modifications. > (And it's already partly covered because it is generic. And I fixed many > minor issues while developing through unit test failures.) > >[Darrell] >Most of dpdk is not tested by our unit tests because it cannot be simulated >well at the moment. This is orthogonal to the basic question however.
Darrell is right and the unit tests we have currently don't test DPDK datapath well. So having this changes in netdev layer shouldn't impact the unit tests much. While I share your other concern that changes in netdev layer will be little complex and slightly painful for future code changes, this max latency config introduced in dpif layer may not hold good to different port types and users may potentially introduce conflicting changes in netdev layer in future to suit their use cases. > > > In the future this can be used also to improve performance of netdev-linux > by replacing sendmsg() with batched sendmmsg(). This should significantly > increase performance of flood actions while MACs are not learned yet in > action NORMAL. > > > This type of code also seems to intersect with present and future QoS >considerations in netdev-dpdk > > Maybe, but there are also some related features in mail-list like rx queue > prioritization which are implemented in generic way on dpif-netdev layer. If you are referring to rxq prioritization work by Billy (https://mail.openvswitch.org/pipermail/ovs-dev/2017-July/336001.html), this feature is more implemented in netdev layer with very minimal updates to dpif layer. BTW, dp_execute_cb() is getting cluttered with this patch. - Bhanuprakash. > > > > > -----Original Message----- > > From: Ilya Maximets <[email protected]> > > Date: Wednesday, July 26, 2017 at 8:21 AM > > To: "[email protected]" <[email protected]>, >Bhanuprakash Bodireddy <[email protected]> > > Cc: Heetae Ahn <[email protected]>, Ben Pfaff ><[email protected]>, Antonio Fischetti <[email protected]>, Eelco >Chaudron <[email protected]>, Ciara Loftus <[email protected]>, >Kevin Traynor <[email protected]>, Darrell Ball <[email protected]>, >Ilya Maximets <[email protected]> > > Subject: [PATCH RFC v2 4/4] dpif-netdev: Time based output batching. > > > > This allows to collect packets from more than one RX burst > > and send them together with a configurable maximum latency. > > > > 'other_config:output-max-latency' can be used to configure > > time that a packet can wait in output batch for sending. > > > > Signed-off-by: Ilya Maximets <[email protected]> > > --- > > > > millisecon granularity is used for now. Can be easily switched to use > > microseconds instead. > > > > lib/dpif-netdev.c | 97 >+++++++++++++++++++++++++++++++++++++++++++--------- > > vswitchd/vswitch.xml | 15 ++++++++ > > 2 files changed, 95 insertions(+), 17 deletions(-) > > > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > > index 07c7dad..e5f8a3d 100644 > > --- a/lib/dpif-netdev.c > > +++ b/lib/dpif-netdev.c > > @@ -84,6 +84,9 @@ VLOG_DEFINE_THIS_MODULE(dpif_netdev); > > #define MAX_RECIRC_DEPTH 5 > > DEFINE_STATIC_PER_THREAD_DATA(uint32_t, recirc_depth, 0) > > > > +/* Use instant packet send by default. */ > > +#define DEFAULT_OUTPUT_MAX_LATENCY 0 > > + > > /* Configuration parameters. */ > > enum { MAX_FLOWS = 65536 }; /* Maximum number of flows in flow >table. */ > > enum { MAX_METERS = 65536 }; /* Maximum number of meters. */ > > @@ -261,6 +264,9 @@ struct dp_netdev { > > struct hmap ports; > > struct seq *port_seq; /* Incremented whenever a port > changes. >*/ > > > > + /* The time that a packet can wait in output batch for sending. > */ > > + atomic_uint32_t output_max_latency; > > + > > /* Meters. */ > > struct ovs_mutex meter_locks[N_METER_LOCKS]; > > struct dp_meter *meters[MAX_METERS]; /* Meter bands. */ > > @@ -498,6 +504,7 @@ struct tx_port { > > int qid; > > long long last_used; > > struct hmap_node node; > > + long long output_time; > > struct dp_packet_batch output_pkts; > > }; > > > > @@ -570,6 +577,9 @@ struct dp_netdev_pmd_thread { > > * than 'cmap_count(dp->poll_threads)'. */ > > const int static_tx_qid; > > > > + /* Number of filled output batches. */ > > + int n_output_batches; > > + > > struct ovs_mutex port_mutex; /* Mutex for 'poll_list' and > 'tx_ports'. >*/ > > /* List of rx queues to poll. */ > > struct hmap poll_list OVS_GUARDED; > > @@ -663,9 +673,9 @@ static void dp_netdev_add_rxq_to_pmd(struct >dp_netdev_pmd_thread *pmd, > > static void dp_netdev_del_rxq_from_pmd(struct >dp_netdev_pmd_thread *pmd, > > struct rxq_poll *poll) > > OVS_REQUIRES(pmd->port_mutex); > > -static void > > +static int > > dp_netdev_pmd_flush_output_packets(struct >dp_netdev_pmd_thread *pmd, > > - long long now); > > + long long now, bool force); > > static void reconfigure_datapath(struct dp_netdev *dp) > > OVS_REQUIRES(dp->port_mutex); > > static bool dp_netdev_pmd_try_ref(struct dp_netdev_pmd_thread >*pmd); > > @@ -1188,6 +1198,7 @@ create_dp_netdev(const char *name, const >struct dpif_class *class, > > conntrack_init(&dp->conntrack); > > > > atomic_init(&dp->emc_insert_min, >DEFAULT_EM_FLOW_INSERT_MIN); > > + atomic_init(&dp->output_max_latency, >DEFAULT_OUTPUT_MAX_LATENCY); > > > > cmap_init(&dp->poll_threads); > > ovs_mutex_init_recursive(&dp->non_pmd_mutex); > > @@ -2843,7 +2854,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, now); > > - dp_netdev_pmd_flush_output_packets(pmd, now); > > + dp_netdev_pmd_flush_output_packets(pmd, now, true); > > > > if (pmd->core_id == NON_PMD_CORE_ID) { > > ovs_mutex_unlock(&dp->non_pmd_mutex); > > @@ -2892,6 +2903,16 @@ dpif_netdev_set_config(struct dpif *dpif, >const struct smap *other_config) > > smap_get_ullong(other_config, "emc-insert-inv-prob", > > DEFAULT_EM_FLOW_INSERT_INV_PROB); > > uint32_t insert_min, cur_min; > > + uint32_t output_max_latency, cur_max_latency; > > + > > + output_max_latency = smap_get_int(other_config, "output-max- >latency", > > + DEFAULT_OUTPUT_MAX_LATENCY); > > + atomic_read_relaxed(&dp->output_max_latency, >&cur_max_latency); > > + if (output_max_latency != cur_max_latency) { > > + atomic_store_relaxed(&dp->output_max_latency, >output_max_latency); > > + VLOG_INFO("Output maximum latency set to %"PRIu32" ms", > > + output_max_latency); > > + } > > > > if (!nullable_string_is_equal(dp->pmd_cmask, cmask)) { > > free(dp->pmd_cmask); > > @@ -3092,11 +3113,12 @@ cycles_count_intermediate(struct >dp_netdev_pmd_thread *pmd, > > non_atomic_ullong_add(&pmd->cycles.n[type], interval); > > } > > > > -static void > > +static int > > dp_netdev_pmd_flush_output_on_port(struct >dp_netdev_pmd_thread *pmd, > > struct tx_port *p, long long now) > > { > > int tx_qid; > > + int output_cnt; > > bool dynamic_txqs; > > > > dynamic_txqs = p->port->dynamic_txqs; > > @@ -3106,21 +3128,39 @@ >dp_netdev_pmd_flush_output_on_port(struct dp_netdev_pmd_thread >*pmd, > > tx_qid = pmd->static_tx_qid; > > } > > > > + output_cnt = dp_packet_batch_size(&p->output_pkts); > > netdev_send(p->port->netdev, tx_qid, &p->output_pkts, >dynamic_txqs); > > dp_packet_batch_init(&p->output_pkts); > > + > > + if (output_cnt) { > > + ovs_assert(pmd->n_output_batches > 0); > > + pmd->n_output_batches--; > > + } > > + return output_cnt; > > } > > > > -static void > > +static int > > dp_netdev_pmd_flush_output_packets(struct >dp_netdev_pmd_thread *pmd, > > - long long now) > > + long long now, bool force) > > { > > struct tx_port *p; > > + int output_cnt = 0; > > + > > + if (!pmd->n_output_batches) { > > + return 0; > > + } > > + > > + if (!now) { > > + now = time_msec(); > > + } > > > > HMAP_FOR_EACH (p, node, &pmd->send_port_cache) { > > - if (!dp_packet_batch_is_empty(&p->output_pkts)) { > > - dp_netdev_pmd_flush_output_on_port(pmd, p, now); > > + if (!dp_packet_batch_is_empty(&p->output_pkts) > > + && (force || p->output_time <= now)) { > > + output_cnt += dp_netdev_pmd_flush_output_on_port(pmd, p, >now); > > } > > } > > + return output_cnt; > > } > > > > static int > > @@ -3130,7 +3170,7 @@ dp_netdev_process_rxq_port(struct >dp_netdev_pmd_thread *pmd, > > { > > struct dp_packet_batch batch; > > int error; > > - int batch_cnt = 0; > > + int batch_cnt = 0, output_cnt = 0; > > > > dp_packet_batch_init(&batch); > > error = netdev_rxq_recv(rx, &batch); > > @@ -3141,7 +3181,7 @@ dp_netdev_process_rxq_port(struct >dp_netdev_pmd_thread *pmd, > > > > batch_cnt = batch.count; > > dp_netdev_input(pmd, &batch, port_no, now); > > - dp_netdev_pmd_flush_output_packets(pmd, now); > > + output_cnt = dp_netdev_pmd_flush_output_packets(pmd, now, >false); > > } else if (error != EAGAIN && error != EOPNOTSUPP) { > > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, > 5); > > > > @@ -3149,7 +3189,7 @@ dp_netdev_process_rxq_port(struct >dp_netdev_pmd_thread *pmd, > > netdev_rxq_get_name(rx), ovs_strerror(error)); > > } > > > > - return batch_cnt; > > + return batch_cnt + output_cnt; > > } > > > > static struct tx_port * > > @@ -3685,6 +3725,8 @@ pmd_free_cached_ports(struct >dp_netdev_pmd_thread *pmd) > > { > > struct tx_port *tx_port_cached; > > > > + /* Flush all the queued packets. */ > > + dp_netdev_pmd_flush_output_packets(pmd, 0, true); > > /* Free all used tx queue ids. */ > > dpif_netdev_xps_revalidate_pmd(pmd, 0, true); > > > > @@ -3759,7 +3801,6 @@ pmd_thread_main(void *f_) > > bool exiting; > > int poll_cnt; > > int i; > > - int process_packets = 0; > > > > poll_list = NULL; > > > > @@ -3788,8 +3829,10 @@ reload: > > > > cycles_count_start(pmd); > > for (;;) { > > + int process_packets = 0; > > + > > for (i = 0; i < poll_cnt; i++) { > > - process_packets = > > + process_packets += > > dp_netdev_process_rxq_port(pmd, poll_list[i].rx, > > poll_list[i].port_no); > > cycles_count_intermediate(pmd, > > @@ -3797,6 +3840,16 @@ reload: > > : > PMD_CYCLES_IDLE); > > } > > > > + if (!process_packets) { > > + /* We didn't receive anything in the process loop. > > + * Check if we need to send something. */ > > + process_packets = >dp_netdev_pmd_flush_output_packets(pmd, > > + 0, > false); > > + cycles_count_intermediate(pmd, > > + process_packets ? > PMD_CYCLES_PROCESSING > > + : > PMD_CYCLES_IDLE); > > + } > > + > > if (lc++ > 1024) { > > bool reload; > > > > @@ -4234,6 +4287,8 @@ dp_netdev_configure_pmd(struct >dp_netdev_pmd_thread *pmd, struct dp_netdev *dp, > > pmd->numa_id = numa_id; > > pmd->need_reload = false; > > > > + pmd->n_output_batches = 0; > > + > > *CONST_CAST(int *, &pmd->static_tx_qid) = cmap_count(&dp- >>poll_threads); > > > > ovs_refcount_init(&pmd->ref_cnt); > > @@ -4418,6 +4473,7 @@ dp_netdev_add_port_tx_to_pmd(struct >dp_netdev_pmd_thread *pmd, > > > > tx->port = port; > > tx->qid = -1; > > + tx->output_time = 0LL; > > dp_packet_batch_init(&tx->output_pkts); > > > > hmap_insert(&pmd->tx_ports, &tx->node, hash_port_no(tx->port- >>port_no)); > > @@ -5094,11 +5150,18 @@ dp_execute_cb(void *aux_, struct >dp_packet_batch *packets_, > > dp_netdev_pmd_flush_output_on_port(pmd, p, now); > > } > > #endif > > + if (dp_packet_batch_is_empty(&p->output_pkts)) { > > + uint32_t cur_max_latency; > > + > > + atomic_read_relaxed(&dp->output_max_latency, >&cur_max_latency); > > + p->output_time = now + cur_max_latency; > > > > - if (OVS_UNLIKELY(dp_packet_batch_size(&p->output_pkts) > > - + dp_packet_batch_size(packets_) > >NETDEV_MAX_BURST)) { > > - /* Some packets was generated while input batch > processing. > > - * Flush here to avoid overflow. */ > > + if (dp_packet_batch_size(packets_)) { > > + pmd->n_output_batches++; > > + } > > + } else if (dp_packet_batch_size(&p->output_pkts) > > + + dp_packet_batch_size(packets_) > >NETDEV_MAX_BURST) { > > + /* Flush here to avoid overflow. */ > > dp_netdev_pmd_flush_output_on_port(pmd, p, now); > > } > > > > diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml > > index 074535b..23930f0 100644 > > --- a/vswitchd/vswitch.xml > > +++ b/vswitchd/vswitch.xml > > @@ -344,6 +344,21 @@ > > </p> > > </column> > > > > + <column name="other_config" key="output-max-latency" > > + type='{"type": "integer", "minInteger": 0, > "maxInteger": 1000}'> > > + <p> > > + Specifies the time in milliseconds that a packet can wait > in output > > + batch for sending i.e. amount of time that packet can > spend in an > > + intermediate output queue before sending to netdev. > > + This option can be used to configure balance between > throughput > > + and latency. Lower values decreases latency while higher > values > > + may be useful to achieve higher performance. > > + </p> > > + <p> > > + Defaults to 0 i.e. instant packet sending (latency > optimized). > > + </p> > > + </column> > > + > > <column name="other_config" key="n-handler-threads" > > type='{"type": "integer", "minInteger": 1}'> > > <p> > > -- > > 2.7.4 > > > > > > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
