On 6/21/23 18:07, Ilya Maximets wrote: > 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?
Also, having 10+ threads waiting on the same lock doesn't sound like a particularly efficient setup. If the locking here indeed a problem, maybe you can try the following patch in your scenario: https://patchwork.ozlabs.org/project/openvswitch/patch/[email protected]/ ? > > 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
