[ovs-dev] [PATCH v2 6/8] netdev-dpdk: retry with queue action

[Chandran, Sugesh]

Regards
_Sugesh


> -----Original Message-----
> From: ovs-dev-bounces at openvswitch.org [mailto:ovs-dev-
> bounces at openvswitch.org] On Behalf Of Yuanhan Liu
> Sent: Tuesday, September 5, 2017 10:23 AM
> To: dev at openvswitch.org
> Subject: [ovs-dev] [PATCH v2 6/8] netdev-dpdk: retry with queue action
> 
> From: Finn Christensen <fc at napatech.com>
> 
> AFAIK, most (if not all) NICs (including Mellanox and Intel) do not support a
> pure MARK action.  It's required to be used together with some other
> actions, like QUEUE.
> 
> To workaround it, retry with a queue action when first try failed.
[Sugesh] This means a NIC that doesn't support pure MARK, will
do every flow install twice no matter how many times the flow install failed before for the same reason. 
The retry can be avoided if we know what a port/NIC can support. It leads to a need of feature discovery of a NIC
at the init time.
> 
> Moreover, some Intel's NIC (say XL710) needs the QUEUE action set before
> the MARK action.
> 
> Co-authored-by: Yuanhan Liu <yliu at fridaylinux.org>
> Signed-off-by: Finn Christensen <fc at napatech.com>
> Signed-off-by: Yuanhan Liu <yliu at fridaylinux.org>
> ---
> 
> v2: - workarounded the queue action issue, which sets the queue index
>       to 0 blindly.
>     - added some comments regarding to the retry with queue action
> ---
>  lib/netdev-dpdk.c | 59
> ++++++++++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 52 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 37b0f99..320fe80
> 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -3324,6 +3324,7 @@ static struct ovs_mutex ufid_lock =
> OVS_MUTEX_INITIALIZER;  struct ufid_dpdk_flow_data {
>      struct hmap_node node;
>      ovs_u128 ufid;
> +    int rxq;
>      struct rte_flow *rte_flow;
>  };
> 
> @@ -3366,7 +3367,8 @@ del_ufid_dpdk_flow_mapping(const ovs_u128
> *ufid)
> 
>  /* Add ufid to dpdk_flow mapping */
>  static inline void
> -add_ufid_dpdk_flow_mapping(const ovs_u128 *ufid, struct rte_flow
> *rte_flow)
> +add_ufid_dpdk_flow_mapping(const ovs_u128 *ufid, struct rte_flow
> *rte_flow,
> +                           int rxq)
>  {
>      size_t hash = hash_bytes(ufid, sizeof(*ufid), 0);
>      struct ufid_dpdk_flow_data *data = xzalloc(sizeof(*data)); @@ -3381,6
> +3383,7 @@ add_ufid_dpdk_flow_mapping(const ovs_u128 *ufid, struct
> rte_flow *rte_flow)
> 
>      data->ufid = *ufid;
>      data->rte_flow = rte_flow;
> +    data->rxq = rxq;
> 
>      ovs_mutex_lock(&ufid_lock);
>      hmap_insert(&ufid_dpdk_flow, &data->node, hash); @@ -3664,15
> +3667,51 @@ netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
>      }
>      add_flow_action(&actions, RTE_FLOW_ACTION_TYPE_END, NULL);
> 
> +    bool tried = 0;
> +    struct rte_flow_action_queue queue;
> +again:
>      flow = rte_flow_create(dev->port_id, &flow_attr, patterns.items,
>                             actions.actions, &error);
> -    if (!flow) {
> +    if (!flow && !tried && actions_len) {
> +        /*
> +         * Seems most (if not all) NICs do not support a pure MARK
> +         * action. It's required to be used together with some other
> +         * actions, such as QUEUE.
> +         *
> +         * Thus, if flow creation fails, here we try again with
> +         * QUEUE action.
> +         *
> +         */
> +        if (info->rxq < 0 || info->rxq >= netdev->n_rxq) {
> +            VLOG_ERR("invalid queue index: %d\n", info->rxq);
> +            ret = -1;
> +            goto out;
> +        }
> +        queue.index = info->rxq;
> +
> +        /* re-build the action */
> +        actions.cnt = 0;
> +        /*
> +         * NOTE: Intel PMD driver needs the QUEUE action set before
> +         * the MARK action.
> +         */
> +        add_flow_action(&actions, RTE_FLOW_ACTION_TYPE_QUEUE,
> &queue);
> +        add_flow_action(&actions, RTE_FLOW_ACTION_TYPE_MARK, &mark);
> +        add_flow_action(&actions, RTE_FLOW_ACTION_TYPE_END, NULL);
> +
> +        VLOG_INFO("failed to create rte flow, "
> +                  "try again with QUEUE action (queue index = %d)\n",
> +                  info->rxq);
> +        tried = true;
> +
> +        goto again;
> +    } else if (!flow) {
>          VLOG_ERR("rte flow creat error: %u : message : %s\n",
>                   error.type, error.message);
>          ret = -1;
>          goto out;
>      }
> -    add_ufid_dpdk_flow_mapping(ufid, flow);
> +    add_ufid_dpdk_flow_mapping(ufid, flow, info->rxq);
>      VLOG_INFO("installed flow %p by ufid "UUID_FMT"\n",
>                flow, UUID_ARGS((struct uuid *)ufid));
> 
> @@ -3784,17 +3823,23 @@ netdev_dpdk_flow_put(struct netdev *netdev,
> struct match *match,
>                       const ovs_u128 *ufid, struct offload_info *info,
>                       struct dpif_flow_stats *stats OVS_UNUSED)  {
> -    struct rte_flow *rte_flow;
> +    struct ufid_dpdk_flow_data *flow_data;
>      int ret;
> 
>      /*
>       * If an old rte_flow exists, it means it's a flow modification.
>       * Here destroy the old rte flow first before adding a new one.
>       */
> -    rte_flow = get_rte_flow_by_ufid(ufid);
> -    if (rte_flow) {
> +    flow_data = find_ufid_dpdk_flow_mapping(ufid);
> +    if (flow_data && flow_data->rte_flow) {
> +        /*
> +         * there is no rxq given for flow modification. Instead, we
> +         * retrieve it from the rxq firstly registered here (by flow
> +         * add operation).
> +         */
> +        info->rxq = flow_data->rxq;
>          ret = netdev_dpdk_destroy_rte_flow(netdev_dpdk_cast(netdev),
> -                                           ufid, rte_flow);
> +                                           ufid, flow_data->rte_flow);
>          if (ret < 0)
>              return ret;
>      }
> --
> 2.7.4
> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev