DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Zhang, Qi Z" <qi.z.zhang@intel.com>
To: Thomas Monjalon <thomas@monjalon.net>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
	"Yigit, Ferruh" <ferruh.yigit@intel.com>,
	"orika@nvidia.com" <orika@nvidia.com>,
	"getelson@nvidia.com" <getelson@nvidia.com>,
	"andrew.rybchenko@oktetlabs.ru" <andrew.rybchenko@oktetlabs.ru>,
	"ajit.khaparde@broadcom.com" <ajit.khaparde@broadcom.com>,
	"jerinj@marvell.com" <jerinj@marvell.com>
Subject: Re: [dpdk-dev] [PATCH] ethdev: refine API description
Date: Mon, 18 Jan 2021 04:01:06 +0000
Message-ID: <6f81345b4dc7435381a5ca4ea2fc420a@intel.com> (raw)
In-Reply-To: <4389911.iuVQrazR7g@thomas>

Hi Thomas:
	Thanks for review, comment in line

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Friday, January 15, 2021 11:52 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; Yigit, Ferruh <ferruh.yigit@intel.com>; orika@nvidia.com;
> getelson@nvidia.com; andrew.rybchenko@oktetlabs.ru;
> ajit.khaparde@broadcom.com; jerinj@marvell.com
> Subject: Re: [dpdk-dev] [PATCH] ethdev: refine API description
> 
> Hi,
> 
> It seems we need to clarify how the API for UDP tunnel works with rte_flow.
> Thanks for starting this patch.
> I ask some questions below for writing a clear and exact API definition.
> 
> 12/01/2021 12:47, Qi Zhang:
> > Refine the description for rte_eth_dev_udp_tunnel_port_add.
> > Claim this is an API for device (or port) level configuration.
> >
> > Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> > ---
> > --- a/lib/librte_ethdev/rte_ethdev.h
> > +++ b/lib/librte_ethdev/rte_ethdev.h
> > @@ -4030,6 +4030,16 @@ rte_eth_dev_rss_hash_conf_get(uint16_t
> port_id,
> >   * to change or add more UDP port for the tunnel. So the offloading
> function
> >   * can take effect on the packets with the specific UDP port.
> >   *
> > + * Due to different requirements from different use cases, NICs may
> > + have a
> > + * different way to identify a UDP port as a tunnel type. Some NIC
> > + takes this
> > + * as a device (or port) level configure while some NIC takes this as
> > + a flow
> > + * based configure.
> 
> I think this assumption is too vague to be useful.
> It brings more confusion than it explains.

OK, let's focus on the API impact as you suggested.
> 
> > + *
> > + * This API is for the first case and typically it will only be
> > + implemented
> > + * on a PF driver or a VF driver which have privilege right to
> > + configure for
> 
> What is a privileged VF exactly?

Yes, looks like "a privileged VF" is not generic enough here, actually it should be a driver that implement DPDK ethdev API and able to perform device/port level configure, 
for example use rte_flow to steer traffic to specific VF, configure UDP tunnel port .... it is something like a "host driver"
It could be a pure user pace DPDK PF driver, or a vdev / SR-IOV driver back ended by a kernel driver but be granted to have privilege to access more device resource through some sw/hw channel.

> 
> > + * other VFs. For the second case, a tunnel configure could be
> > + embedded in a
> > + * rte_flow rule.
> 
> I suggest we make the explanation more API-oriented.
> 
> First thing to explain: this API will have effect on rte_flow rules only, am I right?

I think it should not only impact the rte_flow, but also may impact the packet type that extract from Rx Descriptor to mbuf on some devices
Because without this configure, the parser is not able to recognize the specific tunnel type

> 
> What does it mean for a flow rule matching on a specific tunnel?
> Let's take an example config:
> 	rte_eth_dev_udp_tunnel_port_add [UDP X] [tunnel T]
> 	rte_flow_create [tunnel T] [action A]
> 	rte_flow_create [UDP Y] [tunnel T] [action B] Then action for these

Some driver may not allow to specific a tunnel port in rte_flow as rte_eth_dev_udp_tunnel_port_add is prefer API to handle this.

> packets:
> 	1/ [UDP X] - no tunnel header
> -> A (rte_eth_udp_tunnel skips the tunnel header check) or none, tunnel
> -> header is checked according to rte_flow?

If a packet only have udp tunnel port but don't have tunnel header, so the packet will still not be recognized as a an tunnel packet by the parser, the pattern is not matched and action A should not be executed.

> 	2/ [UDP Y] - no tunnel header
> -> none (flow rule requires a tunnel header)

Yes, expected result

> 	3/ [UDP X] [tunnel T]
> -> A

Yes, expected result

> 	4/ [UDP Y] [tunnel T]
> -> B

Yes, expected result, if the rule #2 is allowed to create

> 	5/ [UDP X] [tunnel U]
> -> A (rte_eth_udp_tunnel skips the tunnel header check) or none, tunnel
> -> header is checked according to rte_flow?

Should be none.

> 	6/ [UDP Y] [tunnel U]
> -> none

Yes expected result.
> 
> Last question, how it plays with rte_flow_tunnel_match?
> Should we replace rte_eth_udp_tunnel with rte_flow_tunnel_match?

My understanding is rte_flow_tunnel_match is just a help function, it help to generate a set of rte_flow_items to feed a new rule creation, so looks like it is not expected to have any interaction with hardware? So I didn't figure out how can we use it to replace,  But maybe we can introduce something new like rte_flow_tunnel_create as we already have a rte_flow_tunnel structure which looks more generic than the existing API that only take udp port for tunnel configure.
> 


  reply	other threads:[~2021-01-18  4:01 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-01-12 11:47 Qi Zhang
2021-01-15 15:51 ` Thomas Monjalon
2021-01-18  4:01   ` Zhang, Qi Z [this message]
2021-01-18  9:49     ` Thomas Monjalon
2021-01-19  0:47       ` Zhang, Qi Z
2021-01-19  3:19 ` [dpdk-dev] [PATCH v2] ethdev: refine doxygen for add UDP tunnel port API Qi Zhang
2021-01-27 11:34   ` Ferruh Yigit
2021-01-27 22:46     ` Thomas Monjalon
2021-02-03 20:02 ` [dpdk-dev] [PATCH v3 1/1] ethdev: refine doxygen comment of UDP tunnel API Thomas Monjalon
2021-02-06 10:40   ` Andrew Rybchenko
2021-02-10 11:20     ` Ferruh Yigit
2021-02-10 19:10       ` Thomas Monjalon

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=6f81345b4dc7435381a5ca4ea2fc420a@intel.com \
    --to=qi.z.zhang@intel.com \
    --cc=ajit.khaparde@broadcom.com \
    --cc=andrew.rybchenko@oktetlabs.ru \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=getelson@nvidia.com \
    --cc=jerinj@marvell.com \
    --cc=orika@nvidia.com \
    --cc=thomas@monjalon.net \
    /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

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git