> -----Original Message-----
> From: Ilya Maximets <i.maxim...@ovn.org>
> Sent: Thursday, April 6, 2023 8:21 PM
> To: lin huang <mit...@outlook.com>; d...@openvswitch.org
> Cc: i.maxim...@ovn.org; Lin Huang <linhu...@ruijie.com.cn>; Simon
> Horman <simon.hor...@corigine.com>
> Subject: Re: [PATCH v1 2/2] netdev-dpdk: add netdev-dpdk ingress pkts
> policer.
>
> 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?
>
Yes. We can add 2 more configuration options in egress-policer.
It seems like a packet will do bps police and then pps police in that order.
> Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
