| Message ID | SY4PR01MB84386DBA1E71929ECC71C39DCDE2A@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 26 Aug 2023, at 8:01, 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. > That is a inefficient way to free memory which we have to call > rte_pktmbuf_free() > pkt_cnt times. > > To improve this, we can use rte_pktmbuf_free_bulk() function to free > arrays of > mbufs instead of freeing packets one by one. > > So, This patch change srtcm_policer to free pkts by bulk using > rte_pktmbuf_free_bulk(). > It gives us a slightly performance improves. Hi Lin, This patch looks good to me, however one small nit. //Eelco > Signed-off-by: Lin Huang <linhuang@ruijie.com.cn> > --- > lib/netdev-dpdk.c | 18 ++++++++++-------- > 1 file changed, 10 insertions(+), 8 deletions(-) > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index 8f1361e21..bc8204f7e 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -2542,13 +2542,13 @@ 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; > - struct rte_mbuf *pkt = NULL; > + struct rte_mbuf *should_steal_batch[NETDEV_MAX_BURST]; > uint64_t current_time = rte_rdtsc(); > + int i = 0, n = 0; > + int cnt = 0; > > for (i = 0; i < pkt_cnt; i++) { > - pkt = pkts[i]; > + struct rte_mbuf *pkt = pkts[i]; > /* Handle current packet */ > if (netdev_dpdk_srtcm_policer_pkt_handle(meter, profile, > pkt, current_time)) > { > @@ -2556,13 +2556,15 @@ srtcm_policer_run_single_packet(struct > rte_meter_srtcm *meter, > pkts[cnt] = pkt; > } > cnt++; > - } else { > - if (should_steal) { > - rte_pktmbuf_free(pkt); > - } > + } else if (should_steal) { > + should_steal_batch[n++] = pkt; Here, I would keep the nested else if as is, as the "else if" is mostly used for related/continued "if else if" statements. Here netdev_dpdk_srtcm_policer_pkt_handle() and should_steal are not really a continuation. > } > } > > + if (n) { > + rte_pktmbuf_free_bulk(should_steal_batch, n); > + } > + > return cnt; > }
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 8f1361e21..bc8204f7e 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -2542,13 +2542,13 @@ 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; - struct rte_mbuf *pkt = NULL; + struct rte_mbuf *should_steal_batch[NETDEV_MAX_BURST]; uint64_t current_time = rte_rdtsc(); + int i = 0, n = 0; + int cnt = 0; for (i = 0; i < pkt_cnt; i++) { - pkt = pkts[i]; + struct rte_mbuf *pkt = pkts[i]; /* Handle current packet */ if (netdev_dpdk_srtcm_policer_pkt_handle(meter, profile, pkt, current_time)) { @@ -2556,13 +2556,15 @@ srtcm_policer_run_single_packet(struct rte_meter_srtcm *meter, pkts[cnt] = pkt; } cnt++; - } else { - if (should_steal) { - rte_pktmbuf_free(pkt); - } + } else if (should_steal) { + should_steal_batch[n++] = pkt; } } + if (n) { + rte_pktmbuf_free_bulk(should_steal_batch, n); + } + return cnt; }