On 4/4/23 19:24, Ilya Maximets wrote:
> On 4/4/23 19:08, lin huang wrote:
>> Hi Ilya,
>>
>> Thanks for looking at this patch and for your feedback.
>> There are some explanations about your comments.
>>
>>> -----Original Message-----
>>> From: Ilya Maximets <i.maxim...@ovn.org>
>>> Sent: Tuesday, April 4, 2023 11:08 PM
>>> To: mit...@outlook.com; d...@openvswitch.org
>>> Cc: i.maxim...@ovn.org; Lin Huang <linhu...@ruijie.com.cn>
>>> Subject: Re: [PATCH v1 2/2] netdev-dpdk: add netdev-dpdk ingress pkts
>>> policer.
>>>
>>> On 4/2/23 11:30, mit...@outlook.com wrote:
>>>> From: Lin Huang <linhu...@ruijie.com.cn>
>>>>
>>>> add netdev-dpdk ingress pkts policer.
>>>
>>> Same here. Please, add some 'why?' and 'how?' to the
>>> commit message. And a NEWS entry.
>>
>> Yes, I will add more information about my work later.
>>
>>>
>>>>
>>>> Signed-off-by: Lin Huang <linhu...@ruijie.com.cn>
>>>> ---
>>>> lib/netdev-dpdk.c | 115
>>> ++++++++++++++++++++++++++++++++++++++++++++--
>>>
>>> We also need an update to the vswitch.xml, as it currently
>>> states that pkts policing is not supported by DPDK ports.
>>>
>>> Also, for both patches it might make sense to add very basic
>>> configuration tests to the tests/system-dpdk.at.
>>>
>>>> 1 file changed, 111 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>>>> index 6709c459c..b645b3907 100644
>>>> --- a/lib/netdev-dpdk.c
>>>> +++ b/lib/netdev-dpdk.c
>>>> @@ -411,6 +411,11 @@ struct ingress_policer {
>>>> rte_spinlock_t policer_lock;
>>>> };
>>>>
>>>> +struct ingress_pkts_policer {
>>>> + struct token_bucket tb;
>>>> + rte_spinlock_t policer_lock;
>>>> +};
>>>
>>> I didn't read the whole patch, but there is some code duplication
>>> that is maybe unnecessary.
>>>
>>> What if instead of creating a separate entity for the pkts policer,
>>> we merge the structure above into 'struct ingress_policer', i.e.
>>> add a token bucket into the existing structure? And then we'll call
>>> token_bucket_withdraw right from the srtcm_policer_run_single_packet,
>>> depending on which type of policer is configured. We can add a flag
>>> to the 'struct ingress_policer' stating which kind of policer is
>>> configured or both. The function will need to be renamed, of course.
>>>
>>> This way we could avoid reading both policers on every receive and
>>> save some lines of code.
>>>
>>> What do you think?
>>
>> Good idea!
>>
>> We can refactor srtcm_policer_run_single_packet to
>> ingress_policer_run_single_packet which can
>> do pps or kpkts policer depend on the flag in 'struct ingress_policer'.
>
> Or both. OVS technically allows both ways of policing to be set
> at the same time. Otherwise, sounds good to me. Thanks!
Simon's comment about introduction of a new QoS type got me thinking.
Maybe we should apply the same approach to the egress policer as well?
i.e. not introduce the new policer type, but add 2 more configuration
options to the existing one and modify the srtcm_policer_run_single_packet()
to call a token bucket or netdev_dpdk_srtcm_policer_pkt_handle depending on
what is enabled.
Also the srtcm_policer_run_single_packet() is already used for both
ingress and egress policers. We can keep using it for both if both
will support byte and pkts rate.
What do you think?
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
