[ovs-dev] [PATCH v2] netdev-dpdk: Execute QoS Checking before copying to mbuf

Gao Zhenyu sysugaozhenyu at gmail.com
Wed Sep 6 06:39:35 UTC 2017


Thanks Derrell, it makes sence to me and I think the code path is more
clear.

Thanks
Zhenyu Gao

2017-09-06 0:24 GMT+08:00 Darrell Ball <dball at vmware.com>:

> Hi Gao
>
> How about the following incremental ?
>
> Thanks Darrell
>
>
>
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 4808d38..648d719 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -1843,64 +1843,58 @@ dpdk_do_tx_copy(struct netdev *netdev, int qid,
> struct dp_packet_batch *batch)
>  #endif
>      struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
>      struct rte_mbuf *pkts[PKT_ARRAY_SIZE];
> -    int txcnt_eth = batch->count;
> -    int dropped = 0;
> -    int newcnt = 0;
> -    int i;
> +    uint32_t cnt = batch->count;
> +    uint32_t dropped = 0;
>
>      if (dev->type != DPDK_DEV_VHOST) {
>          /* Check if QoS has been configured for this netdev. */
> -        txcnt_eth = netdev_dpdk_qos_run(dev,
> -                                        (struct rte_mbuf
> **)batch->packets,
> -                                        txcnt_eth, false);
> -        if (txcnt_eth == 0) {
> -            dropped = batch->count;
> -            goto out;
> -        }
> +        cnt = netdev_dpdk_qos_run(dev, (struct rte_mbuf **)
> batch->packets,
> +                                  cnt, false);
> +        dropped += batch->count - cnt;
>      }
>
>      dp_packet_batch_apply_cutlen(batch);
>
> -    for (i = 0; i < batch->count; i++) {
> -        int size = dp_packet_size(batch->packets[i]);
> +    uint32_t txcnt = 0;
> +
> +    for (uint32_t i = 0; i < cnt; i++) {
> +
> +        uint32_t size = dp_packet_size(batch->packets[i]);
>
>          if (OVS_UNLIKELY(size > dev->max_packet_len)) {
> -            VLOG_WARN_RL(&rl, "Too big size %d max_packet_len %d",
> -                         (int) size, dev->max_packet_len);
> +            VLOG_WARN_RL(&rl, "Too big size %u max_packet_len %d",
> +                         size, dev->max_packet_len);
>
>              dropped++;
>              continue;
>          }
>
> -        pkts[newcnt] = rte_pktmbuf_alloc(dev->dpdk_mp->mp);
> +        pkts[txcnt] = rte_pktmbuf_alloc(dev->dpdk_mp->mp);
>
> -        if (!pkts[newcnt]) {
> -            dropped += batch->count - i;
> +        if (!pkts[txcnt]) {
> +            dropped += cnt - i;
>              break;
>          }
>
>          /* We have to do a copy for now */
> -        memcpy(rte_pktmbuf_mtod(pkts[newcnt], void *),
> +        memcpy(rte_pktmbuf_mtod(pkts[txcnt], void *),
>                 dp_packet_data(batch->packets[i]), size);
>
> -        rte_pktmbuf_data_len(pkts[newcnt]) = size;
> -        rte_pktmbuf_pkt_len(pkts[newcnt]) = size;
> +        rte_pktmbuf_data_len(pkts[txcnt]) = size;
> +        rte_pktmbuf_pkt_len(pkts[txcnt]) = size;
>
> -        newcnt++;
> -        if (dev->type != DPDK_DEV_VHOST && newcnt >= txcnt_eth) {
> -            dropped += batch->count - i - 1;
> -            break;
> -        }
> +        txcnt++;
>      }
>
> -    if (dev->type == DPDK_DEV_VHOST) {
> -        __netdev_dpdk_vhost_send(netdev, qid, (struct dp_packet **) pkts,
> -                                 newcnt);
> -    } else {
> -        dropped += netdev_dpdk_eth_tx_burst(dev, qid, pkts, newcnt);
> +    if (OVS_LIKELY(txcnt)) {
> +        if (dev->type == DPDK_DEV_VHOST) {
> +            __netdev_dpdk_vhost_send(netdev, qid, (struct dp_packet **)
> pkts,
> +                                     txcnt);
> +        } else {
> +            dropped += netdev_dpdk_eth_tx_burst(dev, qid, pkts, txcnt);
> +        }
>      }
>
> -out:
>      if (OVS_UNLIKELY(dropped)) {
>          rte_spinlock_lock(&dev->stats_lock);
>          dev->stats.tx_dropped += dropped;
>
>
> //////////////////////////////////////////////////////////////
>
>
> On 8/29/17, 4:39 AM, "ovs-dev-bounces at openvswitch.org on behalf of Zhenyu
> Gao" <ovs-dev-bounces at openvswitch.org on behalf of sysugaozhenyu at gmail.com>
> wrote:
>
>     In dpdk_do_tx_copy function, all packets were copied to mbuf first,
>     but QoS checking may drop some of them.
>     Move the QoS checking in front of copying data to mbuf, it helps to
>     reduce useless copy.
>
>     Signed-off-by: Zhenyu Gao <sysugaozhenyu at gmail.com>
>     ---
>      lib/netdev-dpdk.c | 55 ++++++++++++++++++++++++++++++
> ++++++-------------------
>      1 file changed, 36 insertions(+), 19 deletions(-)
>
>     diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>     index 1aaf6f7..50874b8 100644
>     --- a/lib/netdev-dpdk.c
>     +++ b/lib/netdev-dpdk.c
>     @@ -279,7 +279,7 @@ struct dpdk_qos_ops {
>           * For all QoS implementations it should always be non-null.
>           */
>          int (*qos_run)(struct qos_conf *qos_conf, struct rte_mbuf **pkts,
>     -                   int pkt_cnt);
>     +                   int pkt_cnt, bool may_steal);
>      };
>
>      /* dpdk_qos_ops for each type of user space QoS implementation */
>     @@ -1501,7 +1501,8 @@ netdev_dpdk_policer_pkt_handle(struct
> rte_meter_srtcm *meter,
>
>      static int
>      netdev_dpdk_policer_run(struct rte_meter_srtcm *meter,
>     -                        struct rte_mbuf **pkts, int pkt_cnt)
>     +                        struct rte_mbuf **pkts, int pkt_cnt,
>     +                        bool may_steal)
>      {
>          int i = 0;
>          int cnt = 0;
>     @@ -1517,7 +1518,9 @@ netdev_dpdk_policer_run(struct rte_meter_srtcm
> *meter,
>                  }
>                  cnt++;
>              } else {
>     -            rte_pktmbuf_free(pkt);
>     +            if (may_steal) {
>     +                rte_pktmbuf_free(pkt);
>     +            }
>              }
>          }
>
>     @@ -1526,12 +1529,13 @@ netdev_dpdk_policer_run(struct rte_meter_srtcm
> *meter,
>
>      static int
>      ingress_policer_run(struct ingress_policer *policer, struct rte_mbuf
> **pkts,
>     -                    int pkt_cnt)
>     +                    int pkt_cnt, bool may_steal)
>      {
>          int cnt = 0;
>
>          rte_spinlock_lock(&policer->policer_lock);
>     -    cnt = netdev_dpdk_policer_run(&policer->in_policer, pkts,
> pkt_cnt);
>     +    cnt = netdev_dpdk_policer_run(&policer->in_policer, pkts,
>     +                                  pkt_cnt, may_steal);
>          rte_spinlock_unlock(&policer->policer_lock);
>
>          return cnt;
>     @@ -1635,7 +1639,7 @@ netdev_dpdk_vhost_rxq_recv(struct netdev_rxq
> *rxq,
>              dropped = nb_rx;
>              nb_rx = ingress_policer_run(policer,
>                                          (struct rte_mbuf **)
> batch->packets,
>     -                                    nb_rx);
>     +                                    nb_rx, true);
>              dropped -= nb_rx;
>          }
>
>     @@ -1672,7 +1676,7 @@ netdev_dpdk_rxq_recv(struct netdev_rxq *rxq,
> struct dp_packet_batch *batch)
>              dropped = nb_rx;
>              nb_rx = ingress_policer_run(policer,
>                                          (struct rte_mbuf **)
> batch->packets,
>     -                                    nb_rx);
>     +                                    nb_rx, true);
>              dropped -= nb_rx;
>          }
>
>     @@ -1690,13 +1694,13 @@ netdev_dpdk_rxq_recv(struct netdev_rxq *rxq,
> struct dp_packet_batch *batch)
>
>      static inline int
>      netdev_dpdk_qos_run(struct netdev_dpdk *dev, struct rte_mbuf **pkts,
>     -                    int cnt)
>     +                    int cnt, bool may_steal)
>      {
>          struct qos_conf *qos_conf = ovsrcu_get(struct qos_conf *,
> &dev->qos_conf);
>
>          if (qos_conf) {
>              rte_spinlock_lock(&qos_conf->lock);
>     -        cnt = qos_conf->ops->qos_run(qos_conf, pkts, cnt);
>     +        cnt = qos_conf->ops->qos_run(qos_conf, pkts, cnt, may_steal);
>              rte_spinlock_unlock(&qos_conf->lock);
>          }
>
>     @@ -1770,7 +1774,7 @@ __netdev_dpdk_vhost_send(struct netdev *netdev,
> int qid,
>
>          cnt = netdev_dpdk_filter_packet_len(dev, cur_pkts, cnt);
>          /* Check has QoS has been configured for the netdev */
>     -    cnt = netdev_dpdk_qos_run(dev, cur_pkts, cnt);
>     +    cnt = netdev_dpdk_qos_run(dev, cur_pkts, cnt, true);
>          dropped = total_pkts - cnt;
>
>          do {
>     @@ -1816,10 +1820,22 @@ dpdk_do_tx_copy(struct netdev *netdev, int
> qid, struct dp_packet_batch *batch)
>      #endif
>          struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
>          struct rte_mbuf *pkts[PKT_ARRAY_SIZE];
>     +    int txcnt_eth = batch->count;
>          int dropped = 0;
>          int newcnt = 0;
>          int i;
>
>     +    if (dev->type != DPDK_DEV_VHOST) {
>     +        /* Check if QoS has been configured for this netdev. */
>     +        txcnt_eth = netdev_dpdk_qos_run(dev,
>     +                                        (struct rte_mbuf
> **)batch->packets,
>     +                                        txcnt_eth, false);
>     +        if (txcnt_eth == 0) {
>     +            dropped = batch->count;
>     +            goto out;
>     +        }
>     +    }
>     +
>          dp_packet_batch_apply_cutlen(batch);
>
>          for (i = 0; i < batch->count; i++) {
>     @@ -1848,21 +1864,20 @@ dpdk_do_tx_copy(struct netdev *netdev, int
> qid, struct dp_packet_batch *batch)
>              rte_pktmbuf_pkt_len(pkts[newcnt]) = size;
>
>              newcnt++;
>     +        if (dev->type != DPDK_DEV_VHOST && newcnt >= txcnt_eth) {
>     +            dropped += batch->count - i - 1;
>     +            break;
>     +        }
>          }
>
>          if (dev->type == DPDK_DEV_VHOST) {
>              __netdev_dpdk_vhost_send(netdev, qid, (struct dp_packet **)
> pkts,
>                                       newcnt);
>          } else {
>     -        unsigned int qos_pkts = newcnt;
>     -
>     -        /* Check if QoS has been configured for this netdev. */
>     -        newcnt = netdev_dpdk_qos_run(dev, pkts, newcnt);
>     -
>     -        dropped += qos_pkts - newcnt;
>              dropped += netdev_dpdk_eth_tx_burst(dev, qid, pkts, newcnt);
>          }
>
>     +out:
>          if (OVS_UNLIKELY(dropped)) {
>              rte_spinlock_lock(&dev->stats_lock);
>              dev->stats.tx_dropped += dropped;
>     @@ -1915,7 +1930,7 @@ netdev_dpdk_send__(struct netdev_dpdk *dev, int
> qid,
>              dp_packet_batch_apply_cutlen(batch);
>
>              cnt = netdev_dpdk_filter_packet_len(dev, pkts, cnt);
>     -        cnt = netdev_dpdk_qos_run(dev, pkts, cnt);
>     +        cnt = netdev_dpdk_qos_run(dev, pkts, cnt, true);
>              dropped = batch->count - cnt;
>
>              dropped += netdev_dpdk_eth_tx_burst(dev, qid, pkts, cnt);
>     @@ -3132,13 +3147,15 @@ egress_policer_qos_is_equal(const struct
> qos_conf *conf,
>      }
>
>      static int
>     -egress_policer_run(struct qos_conf *conf, struct rte_mbuf **pkts, int
> pkt_cnt)
>     +egress_policer_run(struct qos_conf *conf, struct rte_mbuf **pkts, int
> pkt_cnt,
>     +                   bool may_steal)
>      {
>          int cnt = 0;
>          struct egress_policer *policer =
>              CONTAINER_OF(conf, struct egress_policer, qos_conf);
>
>     -    cnt = netdev_dpdk_policer_run(&policer->egress_meter, pkts,
> pkt_cnt);
>     +    cnt = netdev_dpdk_policer_run(&policer->egress_meter, pkts,
>     +                                  pkt_cnt, may_steal);
>
>          return cnt;
>      }
>     --
>     1.8.3.1
>
>     _______________________________________________
>     dev mailing list
>     dev at openvswitch.org
>     https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.
> openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=
> uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=
> EZzkW98aZhxGnYEEmB3u5e3vaFA90qSr3aIM1t-ggFY&s=
> fHRBBnkb29ujBAr27aGj1MCWkVzjfKqmJz3R9wEg99o&e=
>
>
>
>
>


More information about the dev mailing list