Re: [ovs-dev] [PATCH v1 1/1] netdev-dpdk: Fix egress policer error detection bug.

[Stokes, Ian]

From: Daniele Di Proietto [mailto:diproiet...@ovn.org]
Sent: Friday, August 05, 2016 2:15 AM
To: Stokes, Ian
Cc: dev@openvswitch.org
Subject: Re: [ovs-dev] [PATCH v1 1/1] netdev-dpdk: Fix egress policer error 
detection bug.

Thanks for the patch, comments inline

2016-08-02 9:37 GMT-07:00 Ian Stokes 
<ian.sto...@intel.com<mailto:ian.sto...@intel.com>>:
When egress policer is set as a QoS type for a port, an error may occur during
setup if incorrect parameters are used for the rte_meter. If this occurs
the egress policer construct and set functions should free any allocated
memory relevant to the policer and set the QoS configuration pointer to
null. The netdev_dpdk_set_qos function should check the error value returned
for any QoS construct/set calls with an assertion to avoid segfault.

Signed-off-by: Ian Stokes <ian.sto...@intel.com<mailto:ian.sto...@intel.com>>
---
 lib/netdev-dpdk.c |   29 -
 1 files changed, 28 insertions(+), 1 deletions(-)

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index c208f32..c382270 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -2679,12 +2679,19 @@ netdev_dpdk_set_qos(struct netdev *netdev,

 /* Install new QoS configuration. */
 error = new_ops->qos_construct(netdev, details);
-ovs_assert((error == 0) == (dev->qos_conf != NULL));
 }
 } else {
 error = new_ops->qos_construct(netdev, details);
+}
+
+if (!error) {
 ovs_assert((error == 0) == (dev->qos_conf != NULL));
 }
+else {
+VLOG_ERR("Failed to set QoS type %s on port %s, returned error %d",
+type, netdev->name, error);
+ovs_assert(dev->qos_conf == NULL);
+}

I think we can replace this with:

ovs_assert((error == 0) == (dev->qos_conf != NULL));
if (!error) {
   VLOG(...)
}

type should be aligned with " on the above line
Can we use rte_strerror to print a textual representation?


 ovs_mutex_unlock(>mutex);
 return error;
@@ -2726,6 +2733,15 @@ egress_policer_qos_construct(struct netdev *netdev,
 policer->app_srtcm_params.ebs = 0;
 err = rte_meter_srtcm_config(>egress_meter,
 >app_srtcm_params);
+
+if (err < 0) {
+/* Error occurred during rte_meter creation, destroy the policer
+ * and set the qos configuration for the netdev dpdk to NULL
+ */
+free(policer);
+dev->qos_conf = NULL;
+}
+

Can we return a positive error number instead of a negative one? This is more 
inline with the rest of OVS

 rte_spinlock_unlock(>qos_lock);

 return err;
@@ -2756,11 +2772,13 @@ static int
 egress_policer_qos_set(struct netdev *netdev, const struct smap *details)
 {
 struct egress_policer *policer;
+struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
 const char *cir_s;
 const char *cbs_s;
 int err = 0;

 policer = egress_policer_get__(netdev);
+rte_spinlock_lock(>qos_lock);
 cir_s = smap_get(details, "cir");
 cbs_s = smap_get(details, "cbs");
 policer->app_srtcm_params.cir = cir_s ? strtoull(cir_s, NULL, 10) : 0;
@@ -2769,6 +2787,15 @@ egress_policer_qos_set(struct netdev *netdev, const 
struct smap *details)
 err = rte_meter_srtcm_config(>egress_meter,
 >app_srtcm_params);

+if (err < 0) {
+/* Error occurred during rte_meter creation, destroy the policer
+ * and set the qos configuration for the netdev dpdk to NULL
+ */
+free(policer);
+dev->qos_conf = NULL;
+}
+rte_spinlock_unlock(>qos_lock);
+

Can we return a positive error number instead of a negative one? This is more 
inline with the rest of OVS
I guess we forgot to lock the spinlock here on the original patch and this 
commit fixes it. Can you document this in the commit message?
In the long term I'd like this to use RCU, as we wouldn't need so many critical 
sections, but it's fine to avoid it for now

 return err;
 }
Thanks for the review Daniele, I agree with the comments above and have sent a 
v2 patch
http://openvswitch.org/pipermail/dev/2016-August/077592.html
As regards the RCU, I had started work on this but didn’t have a chance to 
finish it off; it’s something I’ll look at again in future as it would be 
better than what we have now.

Thanks
Ian
Thanks,
Daniele

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v1 1/1] netdev-dpdk: Fix egress policer error detection bug.

[Daniele Di Proietto]

Thanks for the patch, comments inline

2016-08-02 9:37 GMT-07:00 Ian Stokes :

> When egress policer is set as a QoS type for a port, an error may occur
> during
> setup if incorrect parameters are used for the rte_meter. If this occurs
> the egress policer construct and set functions should free any allocated
> memory relevant to the policer and set the QoS configuration pointer to
> null. The netdev_dpdk_set_qos function should check the error value
> returned
> for any QoS construct/set calls with an assertion to avoid segfault.
>
> Signed-off-by: Ian Stokes 
> ---
>  lib/netdev-dpdk.c |   29 -
>  1 files changed, 28 insertions(+), 1 deletions(-)
>
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index c208f32..c382270 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -2679,12 +2679,19 @@ netdev_dpdk_set_qos(struct netdev *netdev,
>
>  /* Install new QoS configuration. */
>  error = new_ops->qos_construct(netdev, details);
> -ovs_assert((error == 0) == (dev->qos_conf != NULL));
>  }
>  } else {
>  error = new_ops->qos_construct(netdev, details);
> +}
> +
> +if (!error) {
>  ovs_assert((error == 0) == (dev->qos_conf != NULL));
>  }
> +else {
> +VLOG_ERR("Failed to set QoS type %s on port %s, returned error
> %d",
> +type, netdev->name, error);
> +ovs_assert(dev->qos_conf == NULL);
> +}
>

I think we can replace this with:

ovs_assert((error == 0) == (dev->qos_conf != NULL));
if (!error) {
   VLOG(...)
}

type should be aligned with " on the above line

Can we use rte_strerror to print a textual representation?


>
>  ovs_mutex_unlock(>mutex);
>  return error;
> @@ -2726,6 +2733,15 @@ egress_policer_qos_construct(struct netdev *netdev,
>  policer->app_srtcm_params.ebs = 0;
>  err = rte_meter_srtcm_config(>egress_meter,
>  >app_srtcm_params);
> +
> +if (err < 0) {
> +/* Error occurred during rte_meter creation, destroy the policer
> + * and set the qos configuration for the netdev dpdk to NULL
> + */
> +free(policer);
> +dev->qos_conf = NULL;
> +}
> +
>

Can we return a positive error number instead of a negative one? This is
more inline with the rest of OVS

 rte_spinlock_unlock(>qos_lock);
>
>  return err;
> @@ -2756,11 +2772,13 @@ static int
>  egress_policer_qos_set(struct netdev *netdev, const struct smap *details)
>  {
>  struct egress_policer *policer;
> +struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
>  const char *cir_s;
>  const char *cbs_s;
>  int err = 0;
>
>  policer = egress_policer_get__(netdev);
> +rte_spinlock_lock(>qos_lock);
>  cir_s = smap_get(details, "cir");
>  cbs_s = smap_get(details, "cbs");
>  policer->app_srtcm_params.cir = cir_s ? strtoull(cir_s, NULL, 10) : 0;
> @@ -2769,6 +2787,15 @@ egress_policer_qos_set(struct netdev *netdev, const
> struct smap *details)
>  err = rte_meter_srtcm_config(>egress_meter,
>  >app_srtcm_params);
>
> +if (err < 0) {
> +/* Error occurred during rte_meter creation, destroy the policer
> + * and set the qos configuration for the netdev dpdk to NULL
> + */
> +free(policer);
> +dev->qos_conf = NULL;
> +}
> +rte_spinlock_unlock(>qos_lock);
> +
>

Can we return a positive error number instead of a negative one? This is
more inline with the rest of OVS

I guess we forgot to lock the spinlock here on the original patch and this
commit fixes it. Can you document this in the commit message?

In the long term I'd like this to use RCU, as we wouldn't need so many
critical sections, but it's fine to avoid it for now



>  return err;
>  }
>
>

Thanks,

Daniele
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev