From: "Zhang, Qi Z" <qi.z.zhang@intel.com>
To: "Kar, Subarna" <subarna.kar@intel.com>, "dev@dpdk.org" <dev@dpdk.org>
Cc: "Wu, Jingjing" <jingjing.wu@intel.com>,
"Gasparakis, Joseph" <joseph.gasparakis@intel.com>
Subject: Re: [dpdk-dev] [PATCH] net-i40e-Match-on-outer-and-inner-headers-for-tunnel
Date: Thu, 6 Dec 2018 11:44:46 +0000 [thread overview]
Message-ID: <039ED4275CED7440929022BC67E70611532FA86B@SHSMSX103.ccr.corp.intel.com> (raw)
In-Reply-To: <1544008393-72281-1-git-send-email-subarna.kar@intel.com>
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.zhang@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>;
> Kar, Subarna <subarna.kar@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.kar@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
next prev parent reply other threads:[~2018-12-06 11:44 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-12-05 11:13 subarna.kar
2018-12-06 11:44 ` Zhang, Qi Z [this message]
2018-12-07 4:52 ` Tu, Lijuan
2018-12-07 4:58 ` Kar, Subarna
2018-12-07 5:05 ` Tu, Lijuan
2018-12-07 5:09 ` Zhang, Qi Z
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=039ED4275CED7440929022BC67E70611532FA86B@SHSMSX103.ccr.corp.intel.com \
--to=qi.z.zhang@intel.com \
--cc=dev@dpdk.org \
--cc=jingjing.wu@intel.com \
--cc=joseph.gasparakis@intel.com \
--cc=subarna.kar@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).