On 5/31/23 17:41, [email protected] wrote: > From: Lin Huang <[email protected]> > > Currently, a meter's timestamp 'now' is set by 'pmd->ctx.now' which updated > by pmd_thread_ctx_time_update(). > > Before processing of the new packet batch: > - dpif_netdev_execute() > - dp_netdev_process_rxq_port() > > There is a problem when user want to police the rate to a low pps by meter. > For example, When the traffic is more than 3Mpps, policing the traffic to > 1M pps, the real rate will be 3Mpps not 1Mpps. > > The key point is that a meter's timestamp isn't update in real time. > For example, PMD thread A and B are polled at the same time (t1). > Thread A starts to run meter to police traffic. At the same time, thread B > pause at 'ovs_mutex_lock(&meter->lock)' to wait thread A release the meter > lock. This elapsed a lot of time, especially we have more than 10 PMD threads.
Shouldn't the infrequent time update cause more drops instead? Best regards, Ilya Maximets. > > After thread A release the meter lock, thread B start to count the time diff. > - long_delta_t = now - meter->used' --> long_delta_t = t1 - t1 = 0. > - band->bucket += (uint64_t) delta_t * band->rate --> band->bucket = 0. > - band_exceeded_pkt = band->bucket / 1000; --> band_exceeded_pkt = 0. > > Fix this problem by update the meter timestamp every time. > > Test-by: Zhang Yuhuang <[email protected]> > Signed-off-by: Lin Huang <[email protected]> > --- > lib/dpif-netdev.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > index 70b953ae6..dbb275cf8 100644 > --- a/lib/dpif-netdev.c > +++ b/lib/dpif-netdev.c > @@ -7169,12 +7169,13 @@ dpif_netdev_meter_get_features(const struct dpif * > dpif OVS_UNUSED, > * that exceed a band are dropped in-place. */ > static void > dp_netdev_run_meter(struct dp_netdev *dp, struct dp_packet_batch *packets_, > - uint32_t meter_id, long long int now) > + uint32_t meter_id) > { > struct dp_meter *meter; > struct dp_meter_band *band; > struct dp_packet *packet; > long long int long_delta_t; /* msec */ > + long long int now; > uint32_t delta_t; /* msec */ > const size_t cnt = dp_packet_batch_size(packets_); > uint32_t bytes, volume; > @@ -7197,8 +7198,12 @@ dp_netdev_run_meter(struct dp_netdev *dp, struct > dp_packet_batch *packets_, > memset(exceeded_rate, 0, cnt * sizeof *exceeded_rate); > > ovs_mutex_lock(&meter->lock); > + > + /* Update now */ > + now = time_msec(); > + > /* All packets will hit the meter at the same time. */ > - long_delta_t = now / 1000 - meter->used / 1000; /* msec */ > + long_delta_t = now - meter->used; /* msec */ > > if (long_delta_t < 0) { > /* This condition means that we have several threads fighting for a > @@ -9170,8 +9175,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), > - pmd->ctx.now); > + dp_netdev_run_meter(pmd->dp, packets_, nl_attr_get_u32(a)); > break; > > case OVS_ACTION_ATTR_PUSH_VLAN: _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
