On 14 Jul 2023, at 13:46, Ilya Maximets wrote:
> On 7/14/23 13:03, Eelco Chaudron wrote:
>>
>>
>> On 1 Jul 2023, at 16:40, [email protected] wrote:
>>
>>> From: Lin Huang <[email protected]>
>>>
>>> Now srtcm_policer will free pkts, if packets are exceed rate limit.
>>> This patch change srtcm_policer not to free pkts, just count dropped
>>> packets.
>>>
>>
>> Thanks for this patch Lin, however, this patch is wrongly freeing packets.
>>
>> Your code assumes packets get dropped in order, but this is not the case.
>> The srtcm_policer_run_single_packet() function can drop the first packet,
>> pass the second, and drop the third.
>
> Hrm, you're right. We should be able to re-order packets though
> by doing pkts[i] = pkts[cnt]; before pkts[cnt] = pkt; in the
> original code. What do you think?
Yes, we could move the dropped ones to the end. As the order of freeing them
should not matter, or does it?
>>
>> Cheers,
>>
>> Eelco
>>
>>> Signed-off-by: Lin Huang <[email protected]>
>>> ---
>>> lib/netdev-dpdk.c | 30 +++++++++++++++---------------
>>> 1 file changed, 15 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>>> index 63dac689e..2f7f74395 100644
>>> --- a/lib/netdev-dpdk.c
>>> +++ b/lib/netdev-dpdk.c
>>> @@ -2399,8 +2399,7 @@ netdev_dpdk_srtcm_policer_pkt_handle(struct
>>> rte_meter_srtcm *meter,
>>> static int
>>> srtcm_policer_run_single_packet(struct rte_meter_srtcm *meter,
>>> struct rte_meter_srtcm_profile *profile,
>>> - struct rte_mbuf **pkts, int pkt_cnt,
>>> - bool should_steal)
>>> + struct rte_mbuf **pkts, int pkt_cnt)
>>> {
>>> int i = 0;
>>> int cnt = 0;
>>> @@ -2410,16 +2409,9 @@ srtcm_policer_run_single_packet(struct
>>> rte_meter_srtcm *meter,
>>> for (i = 0; i < pkt_cnt; i++) {
>>> pkt = pkts[i];
>>> /* Handle current packet */
>>> - if (netdev_dpdk_srtcm_policer_pkt_handle(meter, profile,
>>> - pkt, current_time)) {
>>> - if (cnt != i) {
>>> - pkts[cnt] = pkt;
>>> - }
>>> + if (!netdev_dpdk_srtcm_policer_pkt_handle(meter, profile,
>>> + pkt, current_time)) {
>>> cnt++;
>>> - } else {
>>> - if (should_steal) {
>>> - rte_pktmbuf_free(pkt);
>>> - }
>>> }
>>> }
>>>
>>> @@ -2435,10 +2427,14 @@ ingress_policer_run(struct ingress_policer
>>> *policer, struct rte_mbuf **pkts,
>>> rte_spinlock_lock(&policer->policer_lock);
>>> cnt = srtcm_policer_run_single_packet(&policer->in_policer,
>>> &policer->in_prof,
>>> - pkts, pkt_cnt, should_steal);
>>> + pkts, pkt_cnt);
>>> rte_spinlock_unlock(&policer->policer_lock);
>>>
>>> - return cnt;
>>> + if (should_steal && cnt) {
>>> + rte_pktmbuf_free_bulk(&pkts[pkt_cnt - cnt], cnt);
>>> + }
>>> +
>>> + return pkt_cnt - cnt;
>>> }
>>>
>>> static bool
>>> @@ -4976,9 +4972,13 @@ egress_policer_run(struct qos_conf *conf, struct
>>> rte_mbuf **pkts, int pkt_cnt,
>>>
>>> cnt = srtcm_policer_run_single_packet(&policer->egress_meter,
>>> &policer->egress_prof, pkts,
>>> - pkt_cnt, should_steal);
>>> + pkt_cnt);
>>>
>>> - return cnt;
>>> + if (should_steal && cnt) {
>>> + rte_pktmbuf_free_bulk(&pkts[pkt_cnt - cnt], cnt);
>>> + }
>>> +
>>> + return pkt_cnt - cnt;
>>> }
>>>
>>> static const struct dpdk_qos_ops egress_policer_ops = {
>>> --
>>> 2.39.1
>>>
>>> _______________________________________________
>>> dev mailing list
>>> [email protected]
>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
