Re: [ovs-dev] [PATCH 05/20] netdev-dpdk: Improve HW offload flow debuggability
On Wed, Nov 20, 2019 at 9:05 PM Eli Britstein wrote:
>
> Add debug prints when creating and destroying rte flows.
>
> Signed-off-by: Eli Britstein
> Reviewed-by: Oz Shlomo
> ---
> lib/netdev-dpdk.c | 29 +++
> lib/netdev-offload-dpdk-flow.c| 168
> +++---
> lib/netdev-offload-dpdk-private.h | 5 ++
> 3 files changed, 136 insertions(+), 66 deletions(-)
>
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 02120a379..673cbfbd6 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -67,6 +67,7 @@
> #include "unixctl.h"
> #include "util.h"
> #include "uuid.h"
> +#include "netdev-offload-dpdk-private.h"
>
> enum {VIRTIO_RXQ, VIRTIO_TXQ, VIRTIO_QNUM};
>
> @@ -4452,6 +4453,15 @@ netdev_dpdk_rte_flow_destroy(struct netdev *netdev,
>
> ovs_mutex_lock(&dev->mutex);
> ret = rte_flow_destroy(dev->port_id, rte_flow, error);
> +if (!ret) {
> +VLOG_DBG("Destroy rte_flow %p: netdev=%s, port=%d\n",
> + rte_flow, netdev_get_name(netdev), dev->port_id);
> +} else {
> +VLOG_ERR("Destroy rte_flow %p: netdev=%s, port=%d\n"
> + "FAILED. error %u : message : %s",
> + rte_flow, netdev_get_name(netdev), dev->port_id,
> + error->type, error->message);
> +}
1. Ratelimiting macros (_RL) should be used for VLOG_DBG/ERR logs.
2. Can we move the logging code outside the lock ?
Thanks,
-Harsha
> ovs_mutex_unlock(&dev->mutex);
> return ret;
> }
> @@ -4465,9 +4475,28 @@ netdev_dpdk_rte_flow_create(struct netdev *netdev,
> {
> struct rte_flow *flow;
> struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
> +struct ds s;
>
> ovs_mutex_lock(&dev->mutex);
> flow = rte_flow_create(dev->port_id, attr, items, actions, error);
> +ds_init(&s);
> +if (flow) {
> +VLOG_DBG("Create rte_flow: netdev=%s, port=%d\n"
> + "%s"
> + "Flow handle=%p\n",
> + netdev_get_name(netdev), dev->port_id,
> + ds_cstr(netdev_dpdk_flow_ds_put_flow(&s, attr, items,
> + actions)), flow);
> +} else {
> +VLOG_ERR("Create rte_flow: netdev=%s, port=%d\n"
> + "%s"
> + "FAILED. error %u : message : %s\n",
> + netdev_get_name(netdev), dev->port_id,
> + ds_cstr(netdev_dpdk_flow_ds_put_flow(&s, attr, items,
> + actions)),
> + error->type, error->message);
> +}
> +ds_destroy(&s);
> ovs_mutex_unlock(&dev->mutex);
> return flow;
> }
> diff --git a/lib/netdev-offload-dpdk-flow.c b/lib/netdev-offload-dpdk-flow.c
> index c8c3e28ea..19c933932 100644
> --- a/lib/netdev-offload-dpdk-flow.c
> +++ b/lib/netdev-offload-dpdk-flow.c
> @@ -61,73 +61,71 @@ netdev_dpdk_flow_actions_free(struct flow_actions
> *actions)
> }
>
> static void
> -dump_flow_pattern(struct rte_flow_item *item)
> +ds_put_flow_attr(struct ds *s, const struct rte_flow_attr *attr)
> {
> -struct ds s;
> -
> -if (!VLOG_IS_DBG_ENABLED() || item->type == RTE_FLOW_ITEM_TYPE_END) {
> -return;
> -}
> -
> -ds_init(&s);
> +ds_put_format(s,
> + " Attributes: "
> + "ingress=%d, egress=%d, prio=%d, group=%d, transfer=%d\n",
> + attr->ingress, attr->egress, attr->priority, attr->group,
> + attr->transfer);
> +}
>
> +static void
> +ds_put_flow_pattern(struct ds *s, const struct rte_flow_item *item)
> +{
> if (item->type == RTE_FLOW_ITEM_TYPE_ETH) {
> const struct rte_flow_item_eth *eth_spec = item->spec;
> const struct rte_flow_item_eth *eth_mask = item->mask;
>
> -ds_put_cstr(&s, "rte flow eth pattern:\n");
> +ds_put_cstr(s, "rte flow eth pattern:\n");
> if (eth_spec) {
> -ds_put_format(&s,
> +ds_put_format(s,
>" Spec: src="ETH_ADDR_FMT", dst="ETH_ADDR_FMT", "
>"type=0x%04" PRIx16"\n",
>ETH_ADDR_BYTES_ARGS(eth_spec->src.addr_bytes),
>ETH_ADDR_BYTES_ARGS(eth_spec->dst.addr_bytes),
>ntohs(eth_spec->type));
> } else {
> -ds_put_cstr(&s, " Spec = null\n");
> +ds_put_cstr(s, " Spec = null\n");
> }
> if (eth_mask) {
> -ds_put_format(&s,
> +ds_put_format(s,
>" Mask: src="ETH_ADDR_FMT", dst="ETH_ADDR_FMT", "
>"type=0x%04"PRIx16"\n",
>ETH_ADDR_BYTES_ARGS(eth_mask->src.addr_bytes),
>ETH_ADDR_BYTES_ARGS(eth_mask->dst.addr_bytes),
>ntohs(eth_mask->type));
> } else {
> -ds_put_cstr(&s, " Mask = null\
[ovs-dev] [PATCH 05/20] netdev-dpdk: Improve HW offload flow debuggability
Add debug prints when creating and destroying rte flows.
Signed-off-by: Eli Britstein
Reviewed-by: Oz Shlomo
---
lib/netdev-dpdk.c | 29 +++
lib/netdev-offload-dpdk-flow.c| 168 +++---
lib/netdev-offload-dpdk-private.h | 5 ++
3 files changed, 136 insertions(+), 66 deletions(-)
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 02120a379..673cbfbd6 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -67,6 +67,7 @@
#include "unixctl.h"
#include "util.h"
#include "uuid.h"
+#include "netdev-offload-dpdk-private.h"
enum {VIRTIO_RXQ, VIRTIO_TXQ, VIRTIO_QNUM};
@@ -4452,6 +4453,15 @@ netdev_dpdk_rte_flow_destroy(struct netdev *netdev,
ovs_mutex_lock(&dev->mutex);
ret = rte_flow_destroy(dev->port_id, rte_flow, error);
+if (!ret) {
+VLOG_DBG("Destroy rte_flow %p: netdev=%s, port=%d\n",
+ rte_flow, netdev_get_name(netdev), dev->port_id);
+} else {
+VLOG_ERR("Destroy rte_flow %p: netdev=%s, port=%d\n"
+ "FAILED. error %u : message : %s",
+ rte_flow, netdev_get_name(netdev), dev->port_id,
+ error->type, error->message);
+}
ovs_mutex_unlock(&dev->mutex);
return ret;
}
@@ -4465,9 +4475,28 @@ netdev_dpdk_rte_flow_create(struct netdev *netdev,
{
struct rte_flow *flow;
struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
+struct ds s;
ovs_mutex_lock(&dev->mutex);
flow = rte_flow_create(dev->port_id, attr, items, actions, error);
+ds_init(&s);
+if (flow) {
+VLOG_DBG("Create rte_flow: netdev=%s, port=%d\n"
+ "%s"
+ "Flow handle=%p\n",
+ netdev_get_name(netdev), dev->port_id,
+ ds_cstr(netdev_dpdk_flow_ds_put_flow(&s, attr, items,
+ actions)), flow);
+} else {
+VLOG_ERR("Create rte_flow: netdev=%s, port=%d\n"
+ "%s"
+ "FAILED. error %u : message : %s\n",
+ netdev_get_name(netdev), dev->port_id,
+ ds_cstr(netdev_dpdk_flow_ds_put_flow(&s, attr, items,
+ actions)),
+ error->type, error->message);
+}
+ds_destroy(&s);
ovs_mutex_unlock(&dev->mutex);
return flow;
}
diff --git a/lib/netdev-offload-dpdk-flow.c b/lib/netdev-offload-dpdk-flow.c
index c8c3e28ea..19c933932 100644
--- a/lib/netdev-offload-dpdk-flow.c
+++ b/lib/netdev-offload-dpdk-flow.c
@@ -61,73 +61,71 @@ netdev_dpdk_flow_actions_free(struct flow_actions *actions)
}
static void
-dump_flow_pattern(struct rte_flow_item *item)
+ds_put_flow_attr(struct ds *s, const struct rte_flow_attr *attr)
{
-struct ds s;
-
-if (!VLOG_IS_DBG_ENABLED() || item->type == RTE_FLOW_ITEM_TYPE_END) {
-return;
-}
-
-ds_init(&s);
+ds_put_format(s,
+ " Attributes: "
+ "ingress=%d, egress=%d, prio=%d, group=%d, transfer=%d\n",
+ attr->ingress, attr->egress, attr->priority, attr->group,
+ attr->transfer);
+}
+static void
+ds_put_flow_pattern(struct ds *s, const struct rte_flow_item *item)
+{
if (item->type == RTE_FLOW_ITEM_TYPE_ETH) {
const struct rte_flow_item_eth *eth_spec = item->spec;
const struct rte_flow_item_eth *eth_mask = item->mask;
-ds_put_cstr(&s, "rte flow eth pattern:\n");
+ds_put_cstr(s, "rte flow eth pattern:\n");
if (eth_spec) {
-ds_put_format(&s,
+ds_put_format(s,
" Spec: src="ETH_ADDR_FMT", dst="ETH_ADDR_FMT", "
"type=0x%04" PRIx16"\n",
ETH_ADDR_BYTES_ARGS(eth_spec->src.addr_bytes),
ETH_ADDR_BYTES_ARGS(eth_spec->dst.addr_bytes),
ntohs(eth_spec->type));
} else {
-ds_put_cstr(&s, " Spec = null\n");
+ds_put_cstr(s, " Spec = null\n");
}
if (eth_mask) {
-ds_put_format(&s,
+ds_put_format(s,
" Mask: src="ETH_ADDR_FMT", dst="ETH_ADDR_FMT", "
"type=0x%04"PRIx16"\n",
ETH_ADDR_BYTES_ARGS(eth_mask->src.addr_bytes),
ETH_ADDR_BYTES_ARGS(eth_mask->dst.addr_bytes),
ntohs(eth_mask->type));
} else {
-ds_put_cstr(&s, " Mask = null\n");
+ds_put_cstr(s, " Mask = null\n");
}
-}
-
-if (item->type == RTE_FLOW_ITEM_TYPE_VLAN) {
+} else if (item->type == RTE_FLOW_ITEM_TYPE_VLAN) {
const struct rte_flow_item_vlan *vlan_spec = item->spec;
const struct rte_flow_item_vlan *vlan_mask = item->mask;
-ds_put_cstr(&s, "rte flow vlan pattern:\n");
+ds_put_cstr(s, "rte flow vlan pattern:\n");
