[ovs-dev,v1] netdev-dpdk: Execute QoS Checking before copying to mbuf

Message ID 20170725102748.501-1-sysugaozhenyu@gmail.com
State New
Headers show

Commit Message

Gao Zhenyu July 25, 2017, 10:27 a.m.
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@gmail.com>
---
 lib/netdev-dpdk.c | 55 ++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 36 insertions(+), 19 deletions(-)

Comments

Gao Zhenyu Aug. 8, 2017, 1:36 a.m. | #1
ping....

Thanks
Zhenyu Gao

2017-07-25 18:27 GMT+08:00 Zhenyu Gao <sysugaozhenyu@gmail.com>:

> 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@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 ea17b97..bb8bd8f 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -258,7 +258,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 */
> @@ -1438,7 +1438,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;
> @@ -1454,7 +1455,9 @@ netdev_dpdk_policer_run(struct rte_meter_srtcm
> *meter,
>              }
>              cnt++;
>          } else {
> -            rte_pktmbuf_free(pkt);
> +            if (may_steal) {
> +                rte_pktmbuf_free(pkt);
> +            }
>          }
>      }
>
> @@ -1463,12 +1466,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;
> @@ -1572,7 +1576,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;
>      }
>
> @@ -1609,7 +1613,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;
>      }
>
> @@ -1627,13 +1631,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);
>      }
>
> @@ -1707,7 +1711,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 {
> @@ -1753,10 +1757,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++) {
> @@ -1785,21 +1801,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;
> @@ -1852,7 +1867,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);
> @@ -3061,13 +3076,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
>
>
Darrell Ball Aug. 8, 2017, 2:06 a.m. | #2
Hi Ian

Are you interested in reviewing this ?

Thanks Darrell

-----Original Message-----
From: <ovs-dev-bounces@openvswitch.org> on behalf of Gao Zhenyu <sysugaozhenyu@gmail.com>
Date: Monday, August 7, 2017 at 6:36 PM
To: Ben Pfaff <blp@ovn.org>, "Chandran, Sugesh" <sugesh.chandran@intel.com>, ovs dev <dev@openvswitch.org>
Subject: Re: [ovs-dev] [PATCH v1] netdev-dpdk: Execute QoS Checking before	copying to mbuf

    ping....
    
    Thanks
    Zhenyu Gao
    
    2017-07-25 18:27 GMT+08:00 Zhenyu Gao <sysugaozhenyu@gmail.com>:
    
    > 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@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 ea17b97..bb8bd8f 100644
    > --- a/lib/netdev-dpdk.c
    > +++ b/lib/netdev-dpdk.c
    > @@ -258,7 +258,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 */
    > @@ -1438,7 +1438,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;
    > @@ -1454,7 +1455,9 @@ netdev_dpdk_policer_run(struct rte_meter_srtcm
    > *meter,
    >              }
    >              cnt++;
    >          } else {
    > -            rte_pktmbuf_free(pkt);
    > +            if (may_steal) {
    > +                rte_pktmbuf_free(pkt);
    > +            }
    >          }
    >      }
    >
    > @@ -1463,12 +1466,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;
    > @@ -1572,7 +1576,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;
    >      }
    >
    > @@ -1609,7 +1613,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;
    >      }
    >
    > @@ -1627,13 +1631,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);
    >      }
    >
    > @@ -1707,7 +1711,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 {
    > @@ -1753,10 +1757,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++) {
    > @@ -1785,21 +1801,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;
    > @@ -1852,7 +1867,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);
    > @@ -3061,13 +3076,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@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=roZOnbVuoZgmZiJ5Yl6u9BT9dmjoSQBFIScSFBX2pTE&s=F-jBBvlqt9jYLPkVbaiqJoGKjt3zYp1sTA1LtUK6AJ0&e=
Stokes, Ian Aug. 8, 2017, 9:08 a.m. | #3
Hi Darrell,

I am, I've had a cursory look over it already but was planning to do a deeper dive later this week as I have a few concerns about the approach.

Ian

> -----Original Message-----
> From: Darrell Ball [mailto:dball@vmware.com]
> Sent: Tuesday, August 8, 2017 3:07 AM
> To: Gao Zhenyu <sysugaozhenyu@gmail.com>; Ben Pfaff <blp@ovn.org>;
> Chandran, Sugesh <sugesh.chandran@intel.com>; ovs dev
> <dev@openvswitch.org>; Stokes, Ian <ian.stokes@intel.com>
> Subject: Re: [ovs-dev] [PATCH v1] netdev-dpdk: Execute QoS Checking before
> copying to mbuf
> 
> Hi Ian
> 
> Are you interested in reviewing this ?
> 
> Thanks Darrell
> 
> -----Original Message-----
> From: <ovs-dev-bounces@openvswitch.org> on behalf of Gao Zhenyu
> <sysugaozhenyu@gmail.com>
> Date: Monday, August 7, 2017 at 6:36 PM
> To: Ben Pfaff <blp@ovn.org>, "Chandran, Sugesh"
> <sugesh.chandran@intel.com>, ovs dev <dev@openvswitch.org>
> Subject: Re: [ovs-dev] [PATCH v1] netdev-dpdk: Execute QoS Checking before
> 	copying to mbuf
> 
>     ping....
> 
>     Thanks
>     Zhenyu Gao
> 
>     2017-07-25 18:27 GMT+08:00 Zhenyu Gao <sysugaozhenyu@gmail.com>:
> 
>     > 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@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 ea17b97..bb8bd8f 100644
>     > --- a/lib/netdev-dpdk.c
>     > +++ b/lib/netdev-dpdk.c
>     > @@ -258,7 +258,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 */
>     > @@ -1438,7 +1438,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;
>     > @@ -1454,7 +1455,9 @@ netdev_dpdk_policer_run(struct rte_meter_srtcm
>     > *meter,
>     >              }
>     >              cnt++;
>     >          } else {
>     > -            rte_pktmbuf_free(pkt);
>     > +            if (may_steal) {
>     > +                rte_pktmbuf_free(pkt);
>     > +            }
>     >          }
>     >      }
>     >
>     > @@ -1463,12 +1466,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;
>     > @@ -1572,7 +1576,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;
>     >      }
>     >
>     > @@ -1609,7 +1613,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;
>     >      }
>     >
>     > @@ -1627,13 +1631,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);
>     >      }
>     >
>     > @@ -1707,7 +1711,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 {
>     > @@ -1753,10 +1757,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++) {
>     > @@ -1785,21 +1801,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;
>     > @@ -1852,7 +1867,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);
>     > @@ -3061,13 +3076,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@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=roZOnbVuoZgmZiJ5Yl6u9BT9dmjoSQBFIScSFBX2pTE&s=F-
> jBBvlqt9jYLPkVbaiqJoGKjt3zYp1sTA1LtUK6AJ0&e=
>
Gao Zhenyu Aug. 8, 2017, 9:34 a.m. | #4
Thanks for the review. Please let me know if you have any concern on it. :)

Thanks
Zhenyu Gao

2017-08-08 17:08 GMT+08:00 Stokes, Ian <ian.stokes@intel.com>:

> Hi Darrell,
>
> I am, I've had a cursory look over it already but was planning to do a
> deeper dive later this week as I have a few concerns about the approach.
>
> Ian
>
> > -----Original Message-----
> > From: Darrell Ball [mailto:dball@vmware.com]
> > Sent: Tuesday, August 8, 2017 3:07 AM
> > To: Gao Zhenyu <sysugaozhenyu@gmail.com>; Ben Pfaff <blp@ovn.org>;
> > Chandran, Sugesh <sugesh.chandran@intel.com>; ovs dev
> > <dev@openvswitch.org>; Stokes, Ian <ian.stokes@intel.com>
> > Subject: Re: [ovs-dev] [PATCH v1] netdev-dpdk: Execute QoS Checking
> before
> > copying to mbuf
> >
> > Hi Ian
> >
> > Are you interested in reviewing this ?
> >
> > Thanks Darrell
> >
> > -----Original Message-----
> > From: <ovs-dev-bounces@openvswitch.org> on behalf of Gao Zhenyu
> > <sysugaozhenyu@gmail.com>
> > Date: Monday, August 7, 2017 at 6:36 PM
> > To: Ben Pfaff <blp@ovn.org>, "Chandran, Sugesh"
> > <sugesh.chandran@intel.com>, ovs dev <dev@openvswitch.org>
> > Subject: Re: [ovs-dev] [PATCH v1] netdev-dpdk: Execute QoS Checking
> before
> >       copying to mbuf
> >
> >     ping....
> >
> >     Thanks
> >     Zhenyu Gao
> >
> >     2017-07-25 18:27 GMT+08:00 Zhenyu Gao <sysugaozhenyu@gmail.com>:
> >
> >     > 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@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 ea17b97..bb8bd8f 100644
> >     > --- a/lib/netdev-dpdk.c
> >     > +++ b/lib/netdev-dpdk.c
> >     > @@ -258,7 +258,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 */
> >     > @@ -1438,7 +1438,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;
> >     > @@ -1454,7 +1455,9 @@ netdev_dpdk_policer_run(struct
> rte_meter_srtcm
> >     > *meter,
> >     >              }
> >     >              cnt++;
> >     >          } else {
> >     > -            rte_pktmbuf_free(pkt);
> >     > +            if (may_steal) {
> >     > +                rte_pktmbuf_free(pkt);
> >     > +            }
> >     >          }
> >     >      }
> >     >
> >     > @@ -1463,12 +1466,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;
> >     > @@ -1572,7 +1576,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;
> >     >      }
> >     >
> >     > @@ -1609,7 +1613,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;
> >     >      }
> >     >
> >     > @@ -1627,13 +1631,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);
> >     >      }
> >     >
> >     > @@ -1707,7 +1711,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 {
> >     > @@ -1753,10 +1757,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++) {
> >     > @@ -1785,21 +1801,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;
> >     > @@ -1852,7 +1867,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);
> >     > @@ -3061,13 +3076,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@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=roZOnbVuoZgmZiJ5Yl6u9BT9dmjoSQBFIScSFBX2pTE&s=F-
> > jBBvlqt9jYLPkVbaiqJoGKjt3zYp1sTA1LtUK6AJ0&e=
> >
>
>
Gao Zhenyu Aug. 18, 2017, 1:28 a.m. | #5
Hi Ian,

   This patch is pending for a long time. May I know if I should revise it?


Thanks
Zhenyu Gao

2017-08-08 17:34 GMT+08:00 Gao Zhenyu <sysugaozhenyu@gmail.com>:

> Thanks for the review. Please let me know if you have any concern on it. :)
>
> Thanks
> Zhenyu Gao
>
> 2017-08-08 17:08 GMT+08:00 Stokes, Ian <ian.stokes@intel.com>:
>
>> Hi Darrell,
>>
>> I am, I've had a cursory look over it already but was planning to do a
>> deeper dive later this week as I have a few concerns about the approach.
>>
>> Ian
>>
>> > -----Original Message-----
>> > From: Darrell Ball [mailto:dball@vmware.com]
>> > Sent: Tuesday, August 8, 2017 3:07 AM
>> > To: Gao Zhenyu <sysugaozhenyu@gmail.com>; Ben Pfaff <blp@ovn.org>;
>> > Chandran, Sugesh <sugesh.chandran@intel.com>; ovs dev
>> > <dev@openvswitch.org>; Stokes, Ian <ian.stokes@intel.com>
>> > Subject: Re: [ovs-dev] [PATCH v1] netdev-dpdk: Execute QoS Checking
>> before
>> > copying to mbuf
>> >
>> > Hi Ian
>> >
>> > Are you interested in reviewing this ?
>> >
>> > Thanks Darrell
>> >
>> > -----Original Message-----
>> > From: <ovs-dev-bounces@openvswitch.org> on behalf of Gao Zhenyu
>> > <sysugaozhenyu@gmail.com>
>> > Date: Monday, August 7, 2017 at 6:36 PM
>> > To: Ben Pfaff <blp@ovn.org>, "Chandran, Sugesh"
>> > <sugesh.chandran@intel.com>, ovs dev <dev@openvswitch.org>
>> > Subject: Re: [ovs-dev] [PATCH v1] netdev-dpdk: Execute QoS Checking
>> before
>> >       copying to mbuf
>> >
>> >     ping....
>> >
>> >     Thanks
>> >     Zhenyu Gao
>> >
>> >     2017-07-25 18:27 GMT+08:00 Zhenyu Gao <sysugaozhenyu@gmail.com>:
>> >
>> >     > 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@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 ea17b97..bb8bd8f 100644
>> >     > --- a/lib/netdev-dpdk.c
>> >     > +++ b/lib/netdev-dpdk.c
>> >     > @@ -258,7 +258,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 */
>> >     > @@ -1438,7 +1438,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;
>> >     > @@ -1454,7 +1455,9 @@ netdev_dpdk_policer_run(struct
>> rte_meter_srtcm
>> >     > *meter,
>> >     >              }
>> >     >              cnt++;
>> >     >          } else {
>> >     > -            rte_pktmbuf_free(pkt);
>> >     > +            if (may_steal) {
>> >     > +                rte_pktmbuf_free(pkt);
>> >     > +            }
>> >     >          }
>> >     >      }
>> >     >
>> >     > @@ -1463,12 +1466,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;
>> >     > @@ -1572,7 +1576,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;
>> >     >      }
>> >     >
>> >     > @@ -1609,7 +1613,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;
>> >     >      }
>> >     >
>> >     > @@ -1627,13 +1631,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);
>> >     >      }
>> >     >
>> >     > @@ -1707,7 +1711,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 {
>> >     > @@ -1753,10 +1757,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++) {
>> >     > @@ -1785,21 +1801,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;
>> >     > @@ -1852,7 +1867,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);
>> >     > @@ -3061,13 +3076,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@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=roZOnbVuoZgmZiJ5Yl6u9BT9dmjoSQBFIScSFBX2pTE&s=F-
>> > jBBvlqt9jYLPkVbaiqJoGKjt3zYp1sTA1LtUK6AJ0&e=
>> >
>>
>>
>
Gao Zhenyu Aug. 28, 2017, 6:16 a.m. | #6
ping...  Comments are welcome.

Thanks
Zhenyu Gao

2017-08-18 9:28 GMT+08:00 Gao Zhenyu <sysugaozhenyu@gmail.com>:

> Hi Ian,
>
>    This patch is pending for a long time. May I know if I should revise it?
>
>
> Thanks
> Zhenyu Gao
>
> 2017-08-08 17:34 GMT+08:00 Gao Zhenyu <sysugaozhenyu@gmail.com>:
>
>> Thanks for the review. Please let me know if you have any concern on it.
>> :)
>>
>> Thanks
>> Zhenyu Gao
>>
>> 2017-08-08 17:08 GMT+08:00 Stokes, Ian <ian.stokes@intel.com>:
>>
>>> Hi Darrell,
>>>
>>> I am, I've had a cursory look over it already but was planning to do a
>>> deeper dive later this week as I have a few concerns about the approach.
>>>
>>> Ian
>>>
>>> > -----Original Message-----
>>> > From: Darrell Ball [mailto:dball@vmware.com]
>>> > Sent: Tuesday, August 8, 2017 3:07 AM
>>> > To: Gao Zhenyu <sysugaozhenyu@gmail.com>; Ben Pfaff <blp@ovn.org>;
>>> > Chandran, Sugesh <sugesh.chandran@intel.com>; ovs dev
>>> > <dev@openvswitch.org>; Stokes, Ian <ian.stokes@intel.com>
>>> > Subject: Re: [ovs-dev] [PATCH v1] netdev-dpdk: Execute QoS Checking
>>> before
>>> > copying to mbuf
>>> >
>>> > Hi Ian
>>> >
>>> > Are you interested in reviewing this ?
>>> >
>>> > Thanks Darrell
>>> >
>>> > -----Original Message-----
>>> > From: <ovs-dev-bounces@openvswitch.org> on behalf of Gao Zhenyu
>>> > <sysugaozhenyu@gmail.com>
>>> > Date: Monday, August 7, 2017 at 6:36 PM
>>> > To: Ben Pfaff <blp@ovn.org>, "Chandran, Sugesh"
>>> > <sugesh.chandran@intel.com>, ovs dev <dev@openvswitch.org>
>>> > Subject: Re: [ovs-dev] [PATCH v1] netdev-dpdk: Execute QoS Checking
>>> before
>>> >       copying to mbuf
>>> >
>>> >     ping....
>>> >
>>> >     Thanks
>>> >     Zhenyu Gao
>>> >
>>> >     2017-07-25 18:27 GMT+08:00 Zhenyu Gao <sysugaozhenyu@gmail.com>:
>>> >
>>> >     > 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@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 ea17b97..bb8bd8f 100644
>>> >     > --- a/lib/netdev-dpdk.c
>>> >     > +++ b/lib/netdev-dpdk.c
>>> >     > @@ -258,7 +258,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
>>> */
>>> >     > @@ -1438,7 +1438,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;
>>> >     > @@ -1454,7 +1455,9 @@ netdev_dpdk_policer_run(struct
>>> rte_meter_srtcm
>>> >     > *meter,
>>> >     >              }
>>> >     >              cnt++;
>>> >     >          } else {
>>> >     > -            rte_pktmbuf_free(pkt);
>>> >     > +            if (may_steal) {
>>> >     > +                rte_pktmbuf_free(pkt);
>>> >     > +            }
>>> >     >          }
>>> >     >      }
>>> >     >
>>> >     > @@ -1463,12 +1466,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;
>>> >     > @@ -1572,7 +1576,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;
>>> >     >      }
>>> >     >
>>> >     > @@ -1609,7 +1613,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;
>>> >     >      }
>>> >     >
>>> >     > @@ -1627,13 +1631,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);
>>> >     >      }
>>> >     >
>>> >     > @@ -1707,7 +1711,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 {
>>> >     > @@ -1753,10 +1757,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++) {
>>> >     > @@ -1785,21 +1801,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;
>>> >     > @@ -1852,7 +1867,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);
>>> >     > @@ -3061,13 +3076,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@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=roZOnbVuoZgmZiJ5Yl6u9BT9dmjoSQBFIScSFBX2pTE&s=F-
>>> > jBBvlqt9jYLPkVbaiqJoGKjt3zYp1sTA1LtUK6AJ0&e=
>>> >
>>>
>>>
>>
>
Stokes, Ian Aug. 28, 2017, 1:12 p.m. | #7
Hi Zhenyu,

Sincere apologies for the delay, I’ll be reviewing the patch this week and hope to have feedback by Friday for you. If you could rebase that would be helpful for testing.

Thanks
Ian

From: Gao Zhenyu [mailto:sysugaozhenyu@gmail.com]

Sent: Friday, August 18, 2017 2:28 AM
To: Stokes, Ian <ian.stokes@intel.com>
Cc: Darrell Ball <dball@vmware.com>; Ben Pfaff <blp@ovn.org>; Chandran, Sugesh <sugesh.chandran@intel.com>; ovs dev <dev@openvswitch.org>
Subject: Re: [ovs-dev] [PATCH v1] netdev-dpdk: Execute QoS Checking before copying to mbuf

Hi Ian,
   This patch is pending for a long time. May I know if I should revise it?

Thanks
Zhenyu Gao

2017-08-08 17:34 GMT+08:00 Gao Zhenyu <sysugaozhenyu@gmail.com<mailto:sysugaozhenyu@gmail.com>>:
Thanks for the review. Please let me know if you have any concern on it. :)
Thanks
Zhenyu Gao

2017-08-08 17:08 GMT+08:00 Stokes, Ian <ian.stokes@intel.com<mailto:ian.stokes@intel.com>>:
Hi Darrell,

I am, I've had a cursory look over it already but was planning to do a deeper dive later this week as I have a few concerns about the approach.

Ian

> -----Original Message-----

> From: Darrell Ball [mailto:dball@vmware.com<mailto:dball@vmware.com>]

> Sent: Tuesday, August 8, 2017 3:07 AM

> To: Gao Zhenyu <sysugaozhenyu@gmail.com<mailto:sysugaozhenyu@gmail.com>>; Ben Pfaff <blp@ovn.org<mailto:blp@ovn.org>>;

> Chandran, Sugesh <sugesh.chandran@intel.com<mailto:sugesh.chandran@intel.com>>; ovs dev

> <dev@openvswitch.org<mailto:dev@openvswitch.org>>; Stokes, Ian <ian.stokes@intel.com<mailto:ian.stokes@intel.com>>

> Subject: Re: [ovs-dev] [PATCH v1] netdev-dpdk: Execute QoS Checking before

> copying to mbuf

>

> Hi Ian

>

> Are you interested in reviewing this ?

>

> Thanks Darrell

>

> -----Original Message-----

> From: <ovs-dev-bounces@openvswitch.org<mailto:ovs-dev-bounces@openvswitch.org>> on behalf of Gao Zhenyu

> <sysugaozhenyu@gmail.com<mailto:sysugaozhenyu@gmail.com>>

> Date: Monday, August 7, 2017 at 6:36 PM

> To: Ben Pfaff <blp@ovn.org<mailto:blp@ovn.org>>, "Chandran, Sugesh"

> <sugesh.chandran@intel.com<mailto:sugesh.chandran@intel.com>>, ovs dev <dev@openvswitch.org<mailto:dev@openvswitch.org>>

> Subject: Re: [ovs-dev] [PATCH v1] netdev-dpdk: Execute QoS Checking before

>       copying to mbuf

>

>     ping....

>

>     Thanks

>     Zhenyu Gao

>

>     2017-07-25 18:27 GMT+08:00 Zhenyu Gao <sysugaozhenyu@gmail.com<mailto:sysugaozhenyu@gmail.com>>:

>

>     > 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@gmail.com<mailto:sysugaozhenyu@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 ea17b97..bb8bd8f 100644

>     > --- a/lib/netdev-dpdk.c

>     > +++ b/lib/netdev-dpdk.c

>     > @@ -258,7 +258,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 */

>     > @@ -1438,7 +1438,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;

>     > @@ -1454,7 +1455,9 @@ netdev_dpdk_policer_run(struct rte_meter_srtcm

>     > *meter,

>     >              }

>     >              cnt++;

>     >          } else {

>     > -            rte_pktmbuf_free(pkt);

>     > +            if (may_steal) {

>     > +                rte_pktmbuf_free(pkt);

>     > +            }

>     >          }

>     >      }

>     >

>     > @@ -1463,12 +1466,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;

>     > @@ -1572,7 +1576,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;

>     >      }

>     >

>     > @@ -1609,7 +1613,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;

>     >      }

>     >

>     > @@ -1627,13 +1631,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);

>     >      }

>     >

>     > @@ -1707,7 +1711,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 {

>     > @@ -1753,10 +1757,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++) {

>     > @@ -1785,21 +1801,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;

>     > @@ -1852,7 +1867,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);

>     > @@ -3061,13 +3076,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@openvswitch.org<mailto:dev@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=roZOnbVuoZgmZiJ5Yl6u9BT9dmjoSQBFIScSFBX2pTE&s=F-

> jBBvlqt9jYLPkVbaiqJoGKjt3zYp1sTA1LtUK6AJ0&e=

>
Gao Zhenyu Aug. 29, 2017, 12:33 p.m. | #8
Hi Ian,

   Please take a look at v2 https://patchwork.ozlabs.org/patch/807059/  :)
   No code change, just rebase to latest master.

Thanks
Zhenyu Gao

2017-08-28 21:12 GMT+08:00 Stokes, Ian <ian.stokes@intel.com>:

> Hi Zhenyu,
>
>
>
> Sincere apologies for the delay, I’ll be reviewing the patch this week and
> hope to have feedback by Friday for you. If you could rebase that would be
> helpful for testing.
>
>
>
> Thanks
>
> Ian
>
>
>
> *From:* Gao Zhenyu [mailto:sysugaozhenyu@gmail.com]
> *Sent:* Friday, August 18, 2017 2:28 AM
> *To:* Stokes, Ian <ian.stokes@intel.com>
> *Cc:* Darrell Ball <dball@vmware.com>; Ben Pfaff <blp@ovn.org>; Chandran,
> Sugesh <sugesh.chandran@intel.com>; ovs dev <dev@openvswitch.org>
>
> *Subject:* Re: [ovs-dev] [PATCH v1] netdev-dpdk: Execute QoS Checking
> before copying to mbuf
>
>
>
> Hi Ian,
>
>    This patch is pending for a long time. May I know if I should revise it?
>
> Thanks
>
> Zhenyu Gao
>
>
>
> 2017-08-08 17:34 GMT+08:00 Gao Zhenyu <sysugaozhenyu@gmail.com>:
>
> Thanks for the review. Please let me know if you have any concern on it. :)
>
> Thanks
>
> Zhenyu Gao
>
>
>
> 2017-08-08 17:08 GMT+08:00 Stokes, Ian <ian.stokes@intel.com>:
>
> Hi Darrell,
>
> I am, I've had a cursory look over it already but was planning to do a
> deeper dive later this week as I have a few concerns about the approach.
>
> Ian
>
>
> > -----Original Message-----
> > From: Darrell Ball [mailto:dball@vmware.com]
> > Sent: Tuesday, August 8, 2017 3:07 AM
> > To: Gao Zhenyu <sysugaozhenyu@gmail.com>; Ben Pfaff <blp@ovn.org>;
> > Chandran, Sugesh <sugesh.chandran@intel.com>; ovs dev
> > <dev@openvswitch.org>; Stokes, Ian <ian.stokes@intel.com>
> > Subject: Re: [ovs-dev] [PATCH v1] netdev-dpdk: Execute QoS Checking
> before
> > copying to mbuf
> >
> > Hi Ian
> >
> > Are you interested in reviewing this ?
> >
> > Thanks Darrell
> >
> > -----Original Message-----
> > From: <ovs-dev-bounces@openvswitch.org> on behalf of Gao Zhenyu
> > <sysugaozhenyu@gmail.com>
> > Date: Monday, August 7, 2017 at 6:36 PM
> > To: Ben Pfaff <blp@ovn.org>, "Chandran, Sugesh"
> > <sugesh.chandran@intel.com>, ovs dev <dev@openvswitch.org>
> > Subject: Re: [ovs-dev] [PATCH v1] netdev-dpdk: Execute QoS Checking
> before
> >       copying to mbuf
> >
> >     ping....
> >
> >     Thanks
> >     Zhenyu Gao
> >
> >     2017-07-25 18:27 GMT+08:00 Zhenyu Gao <sysugaozhenyu@gmail.com>:
> >
> >     > 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@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 ea17b97..bb8bd8f 100644
> >     > --- a/lib/netdev-dpdk.c
> >     > +++ b/lib/netdev-dpdk.c
> >     > @@ -258,7 +258,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 */
> >     > @@ -1438,7 +1438,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;
> >     > @@ -1454,7 +1455,9 @@ netdev_dpdk_policer_run(struct
> rte_meter_srtcm
> >     > *meter,
> >     >              }
> >     >              cnt++;
> >     >          } else {
> >     > -            rte_pktmbuf_free(pkt);
> >     > +            if (may_steal) {
> >     > +                rte_pktmbuf_free(pkt);
> >     > +            }
> >     >          }
> >     >      }
> >     >
> >     > @@ -1463,12 +1466,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;
> >     > @@ -1572,7 +1576,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;
> >     >      }
> >     >
> >     > @@ -1609,7 +1613,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;
> >     >      }
> >     >
> >     > @@ -1627,13 +1631,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);
> >     >      }
> >     >
> >     > @@ -1707,7 +1711,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 {
> >     > @@ -1753,10 +1757,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++) {
> >     > @@ -1785,21 +1801,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;
> >     > @@ -1852,7 +1867,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);
> >     > @@ -3061,13 +3076,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@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=roZOnbVuoZgmZiJ5Yl6u9BT9dmjoSQBFIScSFBX2pTE&s=F-
> > jBBvlqt9jYLPkVbaiqJoGKjt3zYp1sTA1LtUK6AJ0&e=
> >
>
>
>
>
>
Stokes, Ian Aug. 29, 2017, 1:41 p.m. | #9
Thanks for rebasing Zhenyu,

Will look at this version for testing and review this week.

Ian

From: Gao Zhenyu [mailto:sysugaozhenyu@gmail.com]

Sent: Tuesday, August 29, 2017 1:33 PM
To: Stokes, Ian <ian.stokes@intel.com>
Cc: Darrell Ball <dball@vmware.com>; Ben Pfaff <blp@ovn.org>; Chandran, Sugesh <sugesh.chandran@intel.com>; ovs dev <dev@openvswitch.org>
Subject: Re: [ovs-dev] [PATCH v1] netdev-dpdk: Execute QoS Checking before copying to mbuf

Hi Ian,
   Please take a look at v2 https://patchwork.ozlabs.org/patch/807059/  :)
   No code change, just rebase to latest master.
Thanks
Zhenyu Gao

2017-08-28 21:12 GMT+08:00 Stokes, Ian <ian.stokes@intel.com<mailto:ian.stokes@intel.com>>:
Hi Zhenyu,

Sincere apologies for the delay, I’ll be reviewing the patch this week and hope to have feedback by Friday for you. If you could rebase that would be helpful for testing.

Thanks
Ian

From: Gao Zhenyu [mailto:sysugaozhenyu@gmail.com<mailto:sysugaozhenyu@gmail.com>]

Sent: Friday, August 18, 2017 2:28 AM
To: Stokes, Ian <ian.stokes@intel.com<mailto:ian.stokes@intel.com>>
Cc: Darrell Ball <dball@vmware.com<mailto:dball@vmware.com>>; Ben Pfaff <blp@ovn.org<mailto:blp@ovn.org>>; Chandran, Sugesh <sugesh.chandran@intel.com<mailto:sugesh.chandran@intel.com>>; ovs dev <dev@openvswitch.org<mailto:dev@openvswitch.org>>

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

Hi Ian,
   This patch is pending for a long time. May I know if I should revise it?
Thanks
Zhenyu Gao

2017-08-08 17:34 GMT+08:00 Gao Zhenyu <sysugaozhenyu@gmail.com<mailto:sysugaozhenyu@gmail.com>>:
Thanks for the review. Please let me know if you have any concern on it. :)
Thanks
Zhenyu Gao

2017-08-08 17:08 GMT+08:00 Stokes, Ian <ian.stokes@intel.com<mailto:ian.stokes@intel.com>>:
Hi Darrell,

I am, I've had a cursory look over it already but was planning to do a deeper dive later this week as I have a few concerns about the approach.

Ian

> -----Original Message-----

> From: Darrell Ball [mailto:dball@vmware.com<mailto:dball@vmware.com>]

> Sent: Tuesday, August 8, 2017 3:07 AM

> To: Gao Zhenyu <sysugaozhenyu@gmail.com<mailto:sysugaozhenyu@gmail.com>>; Ben Pfaff <blp@ovn.org<mailto:blp@ovn.org>>;

> Chandran, Sugesh <sugesh.chandran@intel.com<mailto:sugesh.chandran@intel.com>>; ovs dev

> <dev@openvswitch.org<mailto:dev@openvswitch.org>>; Stokes, Ian <ian.stokes@intel.com<mailto:ian.stokes@intel.com>>

> Subject: Re: [ovs-dev] [PATCH v1] netdev-dpdk: Execute QoS Checking before

> copying to mbuf

>

> Hi Ian

>

> Are you interested in reviewing this ?

>

> Thanks Darrell

>

> -----Original Message-----

> From: <ovs-dev-bounces@openvswitch.org<mailto:ovs-dev-bounces@openvswitch.org>> on behalf of Gao Zhenyu

> <sysugaozhenyu@gmail.com<mailto:sysugaozhenyu@gmail.com>>

> Date: Monday, August 7, 2017 at 6:36 PM

> To: Ben Pfaff <blp@ovn.org<mailto:blp@ovn.org>>, "Chandran, Sugesh"

> <sugesh.chandran@intel.com<mailto:sugesh.chandran@intel.com>>, ovs dev <dev@openvswitch.org<mailto:dev@openvswitch.org>>

> Subject: Re: [ovs-dev] [PATCH v1] netdev-dpdk: Execute QoS Checking before

>       copying to mbuf

>

>     ping....

>

>     Thanks

>     Zhenyu Gao

>

>     2017-07-25 18:27 GMT+08:00 Zhenyu Gao <sysugaozhenyu@gmail.com<mailto:sysugaozhenyu@gmail.com>>:

>

>     > 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@gmail.com<mailto:sysugaozhenyu@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 ea17b97..bb8bd8f 100644

>     > --- a/lib/netdev-dpdk.c

>     > +++ b/lib/netdev-dpdk.c

>     > @@ -258,7 +258,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 */

>     > @@ -1438,7 +1438,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;

>     > @@ -1454,7 +1455,9 @@ netdev_dpdk_policer_run(struct rte_meter_srtcm

>     > *meter,

>     >              }

>     >              cnt++;

>     >          } else {

>     > -            rte_pktmbuf_free(pkt);

>     > +            if (may_steal) {

>     > +                rte_pktmbuf_free(pkt);

>     > +            }

>     >          }

>     >      }

>     >

>     > @@ -1463,12 +1466,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;

>     > @@ -1572,7 +1576,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;

>     >      }

>     >

>     > @@ -1609,7 +1613,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;

>     >      }

>     >

>     > @@ -1627,13 +1631,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);

>     >      }

>     >

>     > @@ -1707,7 +1711,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 {

>     > @@ -1753,10 +1757,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++) {

>     > @@ -1785,21 +1801,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;

>     > @@ -1852,7 +1867,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);

>     > @@ -3061,13 +3076,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@openvswitch.org<mailto:dev@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=roZOnbVuoZgmZiJ5Yl6u9BT9dmjoSQBFIScSFBX2pTE&s=F-

> jBBvlqt9jYLPkVbaiqJoGKjt3zYp1sTA1LtUK6AJ0&e=

>

Patch

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index ea17b97..bb8bd8f 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -258,7 +258,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 */
@@ -1438,7 +1438,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;
@@ -1454,7 +1455,9 @@  netdev_dpdk_policer_run(struct rte_meter_srtcm *meter,
             }
             cnt++;
         } else {
-            rte_pktmbuf_free(pkt);
+            if (may_steal) {
+                rte_pktmbuf_free(pkt);
+            }
         }
     }
 
@@ -1463,12 +1466,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;
@@ -1572,7 +1576,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;
     }
 
@@ -1609,7 +1613,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;
     }
 
@@ -1627,13 +1631,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);
     }
 
@@ -1707,7 +1711,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 {
@@ -1753,10 +1757,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++) {
@@ -1785,21 +1801,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;
@@ -1852,7 +1867,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);
@@ -3061,13 +3076,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;
 }