The reason is below code prevent vPMD be used and I have comment as below.

> > > + 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/


> -----Original Message-----
> From: Kar, Subarna
> Sent: Friday, December 7, 2018 12:58 PM
> To: Tu, Lijuan <lijuan...@intel.com>; Zhang, Qi Z <qi.z.zh...@intel.com>;
> dev@dpdk.org
> Cc: Wu, Jingjing <jingjing...@intel.com>; Gasparakis, Joseph
> <joseph.gaspara...@intel.com>
> Subject: RE: [dpdk-dev] [PATCH]
> net-i40e-Match-on-outer-and-inner-headers-for-tunnel
> 
> Can I ask what kind of packets you are sending? Because your packets
> shouldn't hit my code. This is to accept flows for tunneled pkts on outer and
> inner headers.
> 
> -----Original Message-----
> From: Tu, Lijuan
> Sent: Thursday, December 6, 2018 8:52 PM
> To: Zhang, Qi Z <qi.z.zh...@intel.com>; Kar, Subarna
> <subarna....@intel.com>; dev@dpdk.org; Tu, Lijuan <lijuan...@intel.com>
> Cc: Wu, Jingjing <jingjing...@intel.com>; Gasparakis, Joseph
> <joseph.gaspara...@intel.com>
> Subject: RE: [dpdk-dev] [PATCH]
> net-i40e-Match-on-outer-and-inner-headers-for-tunnel
> 
> Hi Subarna,
> 
> This patch cause a big performance drop , ~30% with single core performance
> test case.
> 
> > -----Original Message-----
> > From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Zhang, Qi Z
> > Sent: Thursday, December 6, 2018 7:45 PM
> > To: Kar, Subarna <subarna....@intel.com>; dev@dpdk.org
> > Cc: Wu, Jingjing <jingjing...@intel.com>; Gasparakis, Joseph
> > <joseph.gaspara...@intel.com>
> > Subject: Re: [dpdk-dev] [PATCH]
> > net-i40e-Match-on-outer-and-inner-headers-for-tunnel
> >
> > 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