| Message ID | SY4PR01MB84380FCA69629CCBB46674EBCD0DA@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 Wed, Aug 09, 2023 at 12:19:20AM +0800, miterv@outlook.com wrote: > From: Lin Huang <linhuang@ruijie.com.cn> > > Currently srtcm_policer free packet one by one, if packets are exceed rate limit. > This patch change srtcm_policer to free pkts by bulk using rte_pktmbuf_free_bulk(). Hi Lin Huang, I think it would be useful to describe why this change is being made: what it the benefit of using rte_pktmbuf_free_bulk() here? > Signed-off-by: Lin Huang <linhuang@ruijie.com.cn> > --- > lib/netdev-dpdk.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index 8f1361e21..7e3020a7d 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -2542,10 +2542,11 @@ srtcm_policer_run_single_packet(struct rte_meter_srtcm *meter, > struct rte_mbuf **pkts, int pkt_cnt, > bool should_steal) > { > - int i = 0; > int cnt = 0; > + int i = 0, n = 0; > struct rte_mbuf *pkt = NULL; > uint64_t current_time = rte_rdtsc(); > + struct rte_mbuf *batch[NETDEV_MAX_BURST] = {0}; Is it necessary to zero this buffer? Also, not strictly related to this patch, but if the declaration of pkt was moved into the for loop then initialising it to NULL, which also seems unnecessary, could cleanly be dropped. > > for (i = 0; i < pkt_cnt; i++) { > pkt = pkts[i]; > @@ -2557,12 +2558,14 @@ srtcm_policer_run_single_packet(struct rte_meter_srtcm *meter, > } > cnt++; > } else { > - if (should_steal) { > - rte_pktmbuf_free(pkt); > - } > + batch[n++] = pkt; Should this be guarded by should steal? > } > } > > + if (should_steal && n) { If so, n (i.e. n != 0) is probably a sufficient condition here. > + rte_pktmbuf_free_bulk(batch, n); > + } > + > return cnt; > } > > -- > 2.39.3 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 8f1361e21..7e3020a7d 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -2542,10 +2542,11 @@ srtcm_policer_run_single_packet(struct rte_meter_srtcm *meter, struct rte_mbuf **pkts, int pkt_cnt, bool should_steal) { - int i = 0; int cnt = 0; + int i = 0, n = 0; struct rte_mbuf *pkt = NULL; uint64_t current_time = rte_rdtsc(); + struct rte_mbuf *batch[NETDEV_MAX_BURST] = {0}; for (i = 0; i < pkt_cnt; i++) { pkt = pkts[i]; @@ -2557,12 +2558,14 @@ srtcm_policer_run_single_packet(struct rte_meter_srtcm *meter, } cnt++; } else { - if (should_steal) { - rte_pktmbuf_free(pkt); - } + batch[n++] = pkt; } } + if (should_steal && n) { + rte_pktmbuf_free_bulk(batch, n); + } + return cnt; }