Hi Subarna:

> -----Original Message-----
> From: Kar, Subarna
> Sent: Wednesday, December 5, 2018 7:13 PM
> To: dev@dpdk.org
> Cc: Zhang, Qi Z <qi.z.zh...@intel.com>; Wu, Jingjing <jingjing...@intel.com>;
> Kar, Subarna <subarna....@intel.com>
> Subject: [PATCH] net-i40e-Match-on-outer-and-inner-headers-for-tunnel

The title is not following DPDK style
it could be 
net/i40e: match on outer and inner headers for tunnel.

Also please add detail commit log for such a large patch.

Btw, the patch is too large, it's better to separate it into small ones

So far what I can suggestion is, at least you can separate into 2 patches
One for new feature in fdir low level and one for bridge rte_flow to fdir.

Below are some captures
> 
> From: Subarna Kar <subarna....@intel.com>
> 
> ---
>  drivers/net/i40e/i40e_ethdev.c |  14 +-
>  drivers/net/i40e/i40e_ethdev.h | 109 +++++-
>  drivers/net/i40e/i40e_fdir.c   | 392 +++++++++++++++++++--
>  drivers/net/i40e/i40e_flow.c   | 781
> ++++++++++++++++++++++++++++++++++++++++-
>  4 files changed, 1250 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> index 7030eb1..0e9f22d 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -1788,6 +1788,7 @@ i40e_dev_configure(struct rte_eth_dev *dev)
>        * legacy filter API is deprecated, the following codes should also be
>        * removed.
>        */
> +     dev->data->dev_conf.fdir_conf.mode = RTE_FDIR_MODE_PERFECT;

This is not correct, this will prevent vPMD be used for all cases, 
I know vPMD should not be used when the new flow be enabled, 
and below patch has some issue will break this, but we should fix on that patch 
anyway.
https://patches.dpdk.org/patch/45697/

> diff --git a/drivers/net/i40e/i40e_ethdev.h b/drivers/net/i40e/i40e_ethdev.h
> index 11ecfc3..f7311ce 100644
> --- a/drivers/net/i40e/i40e_ethdev.h
> +++ b/drivers/net/i40e/i40e_ethdev.h
> @@ -466,6 +466,92 @@ struct i40e_vmdq_info {
>  #define I40E_FLEX_WORD_MASK(off) (0x80 >> (off))
>  #define I40E_FDIR_IPv6_TC_OFFSET     20
> 
>  static inline int
> +i40e_flow_fdir_fill_outer_header(const struct i40e_fdir_input *fdir_input,
> +                              unsigned char *raw_pkt)
> +{
> +     struct ipv4_hdr *ip;
> +     struct ipv6_hdr *ip6;
> +     struct udp_hdr *udp;
> +     struct rte_flow_item_gre *gre;
> +     struct rte_flow_item_mpls *mpls;
> +     struct rte_flow_item_vxlan *vxlan;
> +     uint16_t *ether_type;
> +     uint8_t len = 2 * sizeof(struct ether_addr);
> +     raw_pkt += 2 * sizeof(struct ether_addr);
> +     ether_type = (uint16_t *)raw_pkt;
> +     raw_pkt += sizeof(uint16_t);
> +     len += sizeof(uint16_t);
> +
> +
> +     //we don't need ETH, so direct skip to IP

It's better to use /* xxx */ to follow coding style 
And same comment for all other places 
(except the case that you see nearby comment is already broken and you just 
follow it :))

> +             vxlan = (struct rte_flow_item_vxlan *)raw_pkt;
> +             if (fdir_input->flow_ext.iip_type == I40E_FDIR_IPTYPE_IPV4) {
> +                     vxlan->vni[0] =
> +                             fdir_input->flow.vxlanudp4_flow.vxlan.vni[0];
> +                     vxlan->vni[1] =
> +                             fdir_input->flow.vxlanudp4_flow.vxlan.vni[1];
> +                     vxlan->vni[2] =
> +                             fdir_input->flow.vxlanudp4_flow.vxlan.vni[2];
> +             } else {
> +                     vxlan->vni[0] =
> +                             fdir_input->flow.vxlan6udp4_flow.vxlan.vni[0];
> +                     vxlan->vni[1] =
> +                             fdir_input->flow.vxlan6udp4_flow.vxlan.vni[1];
> +                     vxlan->vni[2] =
> +                             fdir_input->flow.vxlan6udp4_flow.vxlan.vni[2];
> +             }
> +             vxlan->flags = VXLAN_DEFAULT_FLAG;
> +             len += sizeof(struct rte_flow_item_vxlan);
> +     } else {
> +      //now put MPLS info for MPLSoUDP and MPLSoGRE

The intend is not correct, replace spaces with tab

> +
> +static int
> +i40e_flow_parse_fdir_tunnel_pattern(struct rte_eth_dev *dev,
> +                                 const struct rte_flow_item *pattern,
> +                                 struct rte_flow_error *error,
> +                                 struct i40e_fdir_filter_conf *filter)

There is couple intend issue in this function, please double check.

> 
>       i = 0;
> @@ -4621,9 +5382,17 @@ i40e_flow_validate(struct rte_eth_dev *dev,
>                       rte_free(items);
>                       return -rte_errno;
>               }
> -             if (parse_filter)
> -                     ret = parse_filter(dev, attr, items, actions,
> +             if (parse_filter) {
> +                     if (parse_filter != TUNNEL_FDIR_FUNCTION &&
> +                              rss == true) {

s/rss == true/rss 

> +                             ret = i40e_parse_rss_filter(dev, attr, pattern,
> +                                             actions, &cons_filter, error);
> +                             return ret;
> +                     } else {
> +                             ret = parse_filter(dev, attr, items, actions,
>                                          error, &cons_filter);
> +                     }
> +             }
>               flag = true;
>       } while ((ret < 0) && (i < RTE_DIM(i40e_supported_patterns)));
> 
> --
> 2.7.4

Reply via email to