| Message ID | SY4PR01MB8438DA528FC3E2C35D7EAF32CD2BA@SY4PR01MB8438.ausprd01.prod.outlook.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | netdev-dpdk: Add support for userspace port-based packet-per-second policing. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On 1 Jul 2023, at 16:40, miterv@outlook.com wrote: > From: Lin Huang <linhuang@ruijie.com.cn> > > 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. Cheers, Eelco > Signed-off-by: Lin Huang <linhuang@ruijie.com.cn> > --- > 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 7/14/23 13:03, Eelco Chaudron wrote: > > > On 1 Jul 2023, at 16:40, miterv@outlook.com wrote: > >> From: Lin Huang <linhuang@ruijie.com.cn> >> >> 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? > > Cheers, > > Eelco > >> Signed-off-by: Lin Huang <linhuang@ruijie.com.cn> >> --- >> 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 >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
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, miterv@outlook.com wrote: >> >>> From: Lin Huang <linhuang@ruijie.com.cn> >>> >>> 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 <linhuang@ruijie.com.cn> >>> --- >>> 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 >>> dev@openvswitch.org >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>
On 7/14/23 15:21, Eelco Chaudron wrote: > > > 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, miterv@outlook.com wrote: >>> >>>> From: Lin Huang <linhuang@ruijie.com.cn> >>>> >>>> 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? Should be fine to free in any order. Best regards, Ilya Maximets.
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 = {