[dpdk-dev,v2,8/9] examples/l3fwd: add parse-ptype option

Message ID 1482996643-113253-9-git-send-email-jianfeng.tan@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Yuanhan Liu
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel compilation fail Compilation issues

Commit Message

Jianfeng Tan Dec. 29, 2016, 7:30 a.m. UTC
  To support those devices that do not provide packet type info when
receiving packets, add a new option, --parse-ptype, to analyze
packet type in the Rx callback.

Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
---
 examples/l3fwd-power/main.c | 60 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 59 insertions(+), 1 deletion(-)
  

Comments

Yuanhan Liu Dec. 30, 2016, 6:39 a.m. UTC | #1
On Thu, Dec 29, 2016 at 07:30:42AM +0000, Jianfeng Tan wrote:
> To support those devices that do not provide packet type info when
> receiving packets, add a new option, --parse-ptype, to analyze
> packet type in the Rx callback.

I think this would be needed for all PMD drivers don't have the PTYPE
support. For these, --parse-ptype looks like a mandatory option in
the l3fwd example. I didn't find such option given in your test guide
though, which is weird. Mistake?

Besides that, is there a way to query whether a PMD supports PTYPE
or not? If not, we should add a software one by our own. This could
be done without introducing yet another option.

> 
> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
> ---
>  examples/l3fwd-power/main.c | 60 ++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 59 insertions(+), 1 deletion(-)
> 
> diff --git a/examples/l3fwd-power/main.c b/examples/l3fwd-power/main.c
> index b65d683..44843ec 100644
> --- a/examples/l3fwd-power/main.c
> +++ b/examples/l3fwd-power/main.c
> @@ -164,6 +164,8 @@ static uint32_t enabled_port_mask = 0;
>  static int promiscuous_on = 0;
>  /* NUMA is enabled by default. */
>  static int numa_on = 1;
> +static int parse_ptype; /**< Parse packet type using rx callback, and */
> +			/**< disabled by default */
>  
>  enum freq_scale_hint_t
>  {
> @@ -607,6 +609,48 @@ get_ipv4_dst_port(struct ipv4_hdr *ipv4_hdr, uint8_t portid,
>  #endif
>  
>  static inline void
> +parse_ptype_one(struct rte_mbuf *m)
> +{
> +	struct ether_hdr *eth_hdr;
> +	uint32_t packet_type = RTE_PTYPE_UNKNOWN;
> +	uint16_t ether_type;
> +
> +	eth_hdr = rte_pktmbuf_mtod(m, struct ether_hdr *);
> +	ether_type = eth_hdr->ether_type;
> +	if (ether_type == rte_cpu_to_be_16(ETHER_TYPE_IPv4))
> +		packet_type |= RTE_PTYPE_L3_IPV4_EXT_UNKNOWN;
> +	else if (ether_type == rte_cpu_to_be_16(ETHER_TYPE_IPv6))
> +		packet_type |= RTE_PTYPE_L3_IPV6_EXT_UNKNOWN;

BTW, we have rte_net_get_ptype(). Looks like a better option?

	--yliu
  
Jianfeng Tan Dec. 30, 2016, 7:30 a.m. UTC | #2
Hi,

> -----Original Message-----
> From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> Sent: Friday, December 30, 2016 2:40 PM
> To: Tan, Jianfeng
> Cc: dev@dpdk.org; stephen@networkplumber.org
> Subject: Re: [PATCH v2 8/9] examples/l3fwd: add parse-ptype option
> 
> On Thu, Dec 29, 2016 at 07:30:42AM +0000, Jianfeng Tan wrote:
> > To support those devices that do not provide packet type info when
> > receiving packets, add a new option, --parse-ptype, to analyze
> > packet type in the Rx callback.
> 
> I think this would be needed for all PMD drivers don't have the PTYPE
> support. For these, --parse-ptype looks like a mandatory option in
> the l3fwd example. I didn't find such option given in your test guide
> though, which is weird. Mistake?

Oops, my fault, it should be used with this option. As I just cared about if packets are received, instead of what types of packets are received, so I missed that. I'll fix it.

> 
> Besides that, is there a way to query whether a PMD supports PTYPE
> or not? 

Yes, we have API rte_eth_dev_get_supported_ptypes() to do that. This patch is to emulate the commit 71a7e2424e07 ("examples/l3fwd: fix using packet type blindly").

As we talked offline, I'll leverage this API to query if device support needed ptypes, if not, register callback to analysis ptypes. This avoids to use another option. But for record, this also leads to un-consistent behavior with l3fwd.

Thanks,
Jianfeng
> If not, we should add a software one by our own. This could
> be done without introducing yet another option.
> 
> >
> > Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
> > ---
> >  examples/l3fwd-power/main.c | 60
> ++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 59 insertions(+), 1 deletion(-)
> >
> > diff --git a/examples/l3fwd-power/main.c b/examples/l3fwd-
> power/main.c
> > index b65d683..44843ec 100644
> > --- a/examples/l3fwd-power/main.c
> > +++ b/examples/l3fwd-power/main.c
> > @@ -164,6 +164,8 @@ static uint32_t enabled_port_mask = 0;
> >  static int promiscuous_on = 0;
> >  /* NUMA is enabled by default. */
> >  static int numa_on = 1;
> > +static int parse_ptype; /**< Parse packet type using rx callback, and */
> > +			/**< disabled by default */
> >
> >  enum freq_scale_hint_t
> >  {
> > @@ -607,6 +609,48 @@ get_ipv4_dst_port(struct ipv4_hdr *ipv4_hdr,
> uint8_t portid,
> >  #endif
> >
> >  static inline void
> > +parse_ptype_one(struct rte_mbuf *m)
> > +{
> > +	struct ether_hdr *eth_hdr;
> > +	uint32_t packet_type = RTE_PTYPE_UNKNOWN;
> > +	uint16_t ether_type;
> > +
> > +	eth_hdr = rte_pktmbuf_mtod(m, struct ether_hdr *);
> > +	ether_type = eth_hdr->ether_type;
> > +	if (ether_type == rte_cpu_to_be_16(ETHER_TYPE_IPv4))
> > +		packet_type |= RTE_PTYPE_L3_IPV4_EXT_UNKNOWN;
> > +	else if (ether_type == rte_cpu_to_be_16(ETHER_TYPE_IPv6))
> > +		packet_type |= RTE_PTYPE_L3_IPV6_EXT_UNKNOWN;
> 
> BTW, we have rte_net_get_ptype(). Looks like a better option?
> 
> 	--yliu
  
Yuanhan Liu Jan. 3, 2017, 6:59 a.m. UTC | #3
On Fri, Dec 30, 2016 at 07:30:19AM +0000, Tan, Jianfeng wrote:
> Hi,
> 
> > -----Original Message-----
> > From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> > Sent: Friday, December 30, 2016 2:40 PM
> > To: Tan, Jianfeng
> > Cc: dev@dpdk.org; stephen@networkplumber.org
> > Subject: Re: [PATCH v2 8/9] examples/l3fwd: add parse-ptype option
> > 
> > On Thu, Dec 29, 2016 at 07:30:42AM +0000, Jianfeng Tan wrote:
> > > To support those devices that do not provide packet type info when
> > > receiving packets, add a new option, --parse-ptype, to analyze
> > > packet type in the Rx callback.
> > 
> > I think this would be needed for all PMD drivers don't have the PTYPE
> > support. For these, --parse-ptype looks like a mandatory option in
> > the l3fwd example. I didn't find such option given in your test guide
> > though, which is weird. Mistake?
> 
> Oops, my fault, it should be used with this option. As I just cared about if packets are received, instead of what types of packets are received, so I missed that. I'll fix it.
> 
> > 
> > Besides that, is there a way to query whether a PMD supports PTYPE
> > or not? 
> 
> Yes, we have API rte_eth_dev_get_supported_ptypes() to do that. This patch is to emulate the commit 71a7e2424e07 ("examples/l3fwd: fix using packet type blindly").
> 
> As we talked offline, I'll leverage this API to query if device support needed ptypes, if not, register callback to analysis ptypes. This avoids to use another option.

...
> But for record, this also leads to un-consistent behavior with l3fwd.

Yes, and I think we should keep it consistent to l3fwd and all its
variants. Just think it this way: those variants don't do that will
not work with virtio PMD (and others don't have the PTYPE support).

That said, you could split this patch set to two sets: one for
adding the support of virtio interrupt, another one for fixing the
l3fwd (and its variants) for some PMDs don't have PTYPE support.

OTOH, that's the typical issue we will meet when we make same code
multiple copies, one for demonstrating one specific feature (or
something like that): it's a bit painful if we need change something
common in all copies.

	--yliu
  

Patch

diff --git a/examples/l3fwd-power/main.c b/examples/l3fwd-power/main.c
index b65d683..44843ec 100644
--- a/examples/l3fwd-power/main.c
+++ b/examples/l3fwd-power/main.c
@@ -164,6 +164,8 @@  static uint32_t enabled_port_mask = 0;
 static int promiscuous_on = 0;
 /* NUMA is enabled by default. */
 static int numa_on = 1;
+static int parse_ptype; /**< Parse packet type using rx callback, and */
+			/**< disabled by default */
 
 enum freq_scale_hint_t
 {
@@ -607,6 +609,48 @@  get_ipv4_dst_port(struct ipv4_hdr *ipv4_hdr, uint8_t portid,
 #endif
 
 static inline void
+parse_ptype_one(struct rte_mbuf *m)
+{
+	struct ether_hdr *eth_hdr;
+	uint32_t packet_type = RTE_PTYPE_UNKNOWN;
+	uint16_t ether_type;
+
+	eth_hdr = rte_pktmbuf_mtod(m, struct ether_hdr *);
+	ether_type = eth_hdr->ether_type;
+	if (ether_type == rte_cpu_to_be_16(ETHER_TYPE_IPv4))
+		packet_type |= RTE_PTYPE_L3_IPV4_EXT_UNKNOWN;
+	else if (ether_type == rte_cpu_to_be_16(ETHER_TYPE_IPv6))
+		packet_type |= RTE_PTYPE_L3_IPV6_EXT_UNKNOWN;
+
+	m->packet_type = packet_type;
+}
+
+static uint16_t
+cb_parse_ptype(uint8_t port __rte_unused, uint16_t queue __rte_unused,
+	       struct rte_mbuf *pkts[], uint16_t nb_pkts,
+	       uint16_t max_pkts __rte_unused,
+	       void *user_param __rte_unused)
+{
+	unsigned int i;
+
+	for (i = 0; i < nb_pkts; ++i)
+		parse_ptype_one(pkts[i]);
+
+	return nb_pkts;
+}
+
+static int
+add_cb_parse_ptype(uint8_t portid, uint16_t queueid)
+{
+	printf("Port %d: softly parse packet type info\n", portid);
+	if (rte_eth_add_rx_callback(portid, queueid, cb_parse_ptype, NULL))
+		return 0;
+
+	printf("Failed to add rx callback: port=%d\n", portid);
+	return -1;
+}
+
+static inline void
 l3fwd_simple_forward(struct rte_mbuf *m, uint8_t portid,
 				struct lcore_conf *qconf)
 {
@@ -1108,7 +1152,8 @@  print_usage(const char *prgname)
 		"  --config (port,queue,lcore): rx queues configuration\n"
 		"  --no-numa: optional, disable numa awareness\n"
 		"  --enable-jumbo: enable jumbo frame"
-		" which max packet len is PKTLEN in decimal (64-9600)\n",
+		" which max packet len is PKTLEN in decimal (64-9600)\n"
+		"  --parse-ptype: parse packet type by software\n",
 		prgname);
 }
 
@@ -1202,6 +1247,8 @@  parse_config(const char *q_arg)
 	return 0;
 }
 
+#define CMD_LINE_OPT_PARSE_PTYPE "parse-ptype"
+
 /* Parse the argument given in the command line of the application */
 static int
 parse_args(int argc, char **argv)
@@ -1214,6 +1261,7 @@  parse_args(int argc, char **argv)
 		{"config", 1, 0, 0},
 		{"no-numa", 0, 0, 0},
 		{"enable-jumbo", 0, 0, 0},
+		{CMD_LINE_OPT_PARSE_PTYPE, 0, 0, 0},
 		{NULL, 0, 0, 0}
 	};
 
@@ -1284,6 +1332,13 @@  parse_args(int argc, char **argv)
 				(unsigned int)port_conf.rxmode.max_rx_pkt_len);
 			}
 
+			if (!strncmp(lgopts[option_index].name,
+				     CMD_LINE_OPT_PARSE_PTYPE,
+				     sizeof(CMD_LINE_OPT_PARSE_PTYPE))) {
+				printf("soft parse-ptype is enabled\n");
+				parse_ptype = 1;
+			}
+
 			break;
 
 		default:
@@ -1716,6 +1771,9 @@  main(int argc, char **argv)
 				rte_exit(EXIT_FAILURE,
 					"rte_eth_rx_queue_setup: err=%d, "
 						"port=%d\n", ret, portid);
+
+			if (parse_ptype && add_cb_parse_ptype(portid, queueid))
+				rte_exit(EXIT_FAILURE, "Fail to add ptype cb\n");
 		}
 	}