[dpdk-dev] [PATCH v2] example/ip_fragmentation: add fragmentation size support

[Ananyev, Konstantin]

> -----Original Message-----
> From: Jain Ashish-B46179 [mailto:ashish.jain at nxp.com]
> Sent: Friday, June 30, 2017 1:50 PM
> To: Ananyev, Konstantin <konstantin.ananyev at intel.com>; dev at dpdk.org
> Cc: Hemant Agrawal <hemant.agrawal at nxp.com>
> Subject: Re: [PATCH v2] example/ip_fragmentation: add fragmentation size support
> 
> On 6/30/2017 6:04 PM, Ananyev, Konstantin wrote:
> >
> >
> >> -----Original Message-----
> >> From: Ashish Jain [mailto:ashish.jain at nxp.com]
> >> Sent: Friday, June 30, 2017 1:18 PM
> >> To: Ananyev, Konstantin <konstantin.ananyev at intel.com>; dev at dpdk.org
> >> Cc: Ashish Jain <ashish.jain at nxp.com>; Hemant Agrawal <hemant.agrawal at nxp.com>
> >> Subject: [PATCH v2] example/ip_fragmentation: add fragmentation size support
> >>
> >> Adding support for determining fragmentation size for both
> >> ipv4 and ipv6 traffic dynamically through command line.
> >> It is helpful in testing to configure different fragmentation
> >> sizes and validate the packets.
> >>
> >> Signed-off-by: Ashish Jain <ashish.jain at nxp.com>
> >> Signed-off-by: Hemant Agrawal <hemant.agrawal at nxp.com>
> >> ---
> >> Changes in v2:
> >> * used strncmp while associating long options with short ones
> >> * added detailed usage for new added params
> >>
> >>   examples/ip_fragmentation/main.c | 96 ++++++++++++++++++++++++++++++++++++----
> >>   1 file changed, 88 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/examples/ip_fragmentation/main.c b/examples/ip_fragmentation/main.c
> >> index 2f45264..c0b36cd 100644
> >> --- a/examples/ip_fragmentation/main.c
> >> +++ b/examples/ip_fragmentation/main.c
> >> @@ -95,6 +95,16 @@
> >>   #define	IPV6_DEFAULT_PAYLOAD	(IPV6_MTU_DEFAULT - sizeof(struct ipv6_hdr))
> >>
> >>   /*
> >> + * Configure fragmentation size for IPv4 and IPv6 packets
> >> + */
> >> +static uint32_t frag_size_v4 = IPV4_MTU_DEFAULT;
> >> +static uint32_t frag_size_v6 = IPV6_MTU_DEFAULT;
> >> +#define MIN_IPV4_FRAG_SIZE 64
> >> +#define MAX_IPV4_FRAG_SIZE 9600
> >> +#define MIN_IPV6_FRAG_SIZE 1280
> >> +#define MAX_IPV6_FRAG_SIZE 9600
> >> +
> >> +/*
> >>    * Max number of fragments per packet expected - defined by config file.
> >>    */
> >>   #define	MAX_PACKET_FRAG RTE_LIBRTE_IP_FRAG_MAX_FRAG
> >> @@ -139,6 +149,9 @@ static struct ether_addr ports_eth_addr[RTE_MAX_ETHPORTS];
> >>
> >>   #define IPV6_ADDR_LEN 16
> >>
> >> +#define CMD_LINE_OPT_IPV4_FRAG_SIZE "frag_size_v4"
> >> +#define CMD_LINE_OPT_IPV6_FRAG_SIZE "frag_size_v6"
> >> +
> >>   /* mask of enabled ports */
> >>   static int enabled_port_mask = 0;
> >>
> >> @@ -300,14 +313,14 @@ l3fwd_simple_forward(struct rte_mbuf *m, struct lcore_queue_conf *qconf,
> >>   		}
> >>
> >>   		/* if we don't need to do any fragmentation */
> >> -		if (likely (IPV4_MTU_DEFAULT >= m->pkt_len)) {
> >> +		if (likely(frag_size_v4 >= m->pkt_len)) {
> >>   			qconf->tx_mbufs[port_out].m_table[len] = m;
> >>   			len2 = 1;
> >>   		} else {
> >>   			len2 = rte_ipv4_fragment_packet(m,
> >>   				&qconf->tx_mbufs[port_out].m_table[len],
> >>   				(uint16_t)(MBUF_TABLE_SIZE - len),
> >> -				IPV4_MTU_DEFAULT,
> >> +				frag_size_v4,
> >>   				rxq->direct_pool, rxq->indirect_pool);
> >>
> >>   			/* Free input packet */
> >> @@ -336,14 +349,14 @@ l3fwd_simple_forward(struct rte_mbuf *m, struct lcore_queue_conf *qconf,
> >>   		}
> >>
> >>   		/* if we don't need to do any fragmentation */
> >> -		if (likely (IPV6_MTU_DEFAULT >= m->pkt_len)) {
> >> +		if (likely(frag_size_v6 >= m->pkt_len)) {
> >>   			qconf->tx_mbufs[port_out].m_table[len] = m;
> >>   			len2 = 1;
> >>   		} else {
> >>   			len2 = rte_ipv6_fragment_packet(m,
> >>   				&qconf->tx_mbufs[port_out].m_table[len],
> >>   				(uint16_t)(MBUF_TABLE_SIZE - len),
> >> -				IPV6_MTU_DEFAULT,
> >> +				frag_size_v6,
> >>   				rxq->direct_pool, rxq->indirect_pool);
> >>
> >>   			/* Free input packet */
> >> @@ -489,8 +502,16 @@ print_usage(const char *prgname)
> >>   {
> >>   	printf("%s [EAL options] -- -p PORTMASK [-q NQ]\n"
> >>   	       "  -p PORTMASK: hexadecimal bitmask of ports to configure\n"
> >> -	       "  -q NQ: number of queue (=ports) per lcore (default is 1)\n",
> >> -	       prgname);
> >> +	       "  -q NQ: number of queue (=ports) per lcore (default is 1)\n"
> >> +	       "  --frag_size_v4=<num>: optional,  IPv4 fragment size in decimal,\n"
> >> +	       "\t Condition:(frag_size_v4 - 20) should be a multiple of 8\n"
> >> +	       "\t Min value: %d , Max value: %d, default is %d \n"
> >> +	       "  --frag_size_v6=<num>: optional,  IPv6 fragment size in decimal,\n"
> >> +	       "\t Condition:(frag_size_v6 - 40) should be a multiple of 8\n"
> >> +	       "\t Min value: %d , Max value: %d, default is %d \n",
> >> +	       prgname, MIN_IPV4_FRAG_SIZE, MAX_IPV4_FRAG_SIZE,
> >> +	       IPV4_MTU_DEFAULT, MIN_IPV6_FRAG_SIZE, MAX_IPV6_FRAG_SIZE,
> >> +	       IPV6_MTU_DEFAULT);
> >>   }
> >>
> >>   static int
> >> @@ -528,6 +549,29 @@ parse_nqueue(const char *q_arg)
> >>   	return n;
> >>   }
> >>
> >> +static int
> >> +parse_frag_size(const char *str, uint32_t min, uint32_t max,
> >> +		uint8_t hdr_size, uint32_t *val)
> >> +{
> >> +	char *end;
> >> +	uint64_t v;
> >> +
> >> +	/* parse decimal string */
> >> +	errno = 0;
> >> +	v = strtoul(str, &end, 10);
> >> +	if (errno != 0 || *end != '\0')
> >> +		return -EINVAL;
> >> +
> >> +	if (v < min || v > max)
> >> +		return -EINVAL;
> >> +
> >> +	if ((v - hdr_size) % 8)
> >> +		return -EINVAL;
> >> +
> >> +	*val = (uint32_t)v;
> >> +	return 0;
> >> +}
> >> +
> >>   /* Parse the argument given in the command line of the application */
> >>   static int
> >>   parse_args(int argc, char **argv)
> >> @@ -537,6 +581,8 @@ parse_args(int argc, char **argv)
> >>   	int option_index;
> >>   	char *prgname = argv[0];
> >>   	static struct option lgopts[] = {
> >> +		{"frag_size_v4", 1, 0, 0},
> >> +		{"frag_size_v6", 1, 0, 0},
> >>   		{NULL, 0, 0, 0}
> >>   	};
> >>
> >> @@ -568,8 +614,42 @@ parse_args(int argc, char **argv)
> >>
> >>   		/* long options */
> >>   		case 0:
> >> -			print_usage(prgname);
> >> -			return -1;
> >> +			if (!strncmp(lgopts[option_index].name,
> >> +				CMD_LINE_OPT_IPV4_FRAG_SIZE,
> >> +				sizeof(CMD_LINE_OPT_IPV4_FRAG_SIZE))) {
> >> +				ret = parse_frag_size(optarg,
> >> +						MIN_IPV4_FRAG_SIZE,
> >> +						MAX_IPV4_FRAG_SIZE,
> >> +						sizeof(struct ipv4_hdr),
> >> +						&frag_size_v4);
> >> +				if (ret) {
> >> +					printf("invalid value: \"%s\" for "
> >> +						"parameter %s\n",
> >> +						optarg,
> >> +						lgopts[option_index].name);
> >> +					print_usage(prgname);
> >> +					return ret;
> >> +				}
> >> +			}
> >> +			if (!strncmp(lgopts[option_index].name,
> >> +				CMD_LINE_OPT_IPV6_FRAG_SIZE,
> >> +				sizeof(CMD_LINE_OPT_IPV6_FRAG_SIZE))) {
> >> +				ret = parse_frag_size(optarg,
> >> +						MIN_IPV6_FRAG_SIZE,
> >> +						MAX_IPV6_FRAG_SIZE,
> >> +						sizeof(struct ipv6_hdr),
> >> +						&frag_size_v6);
> >> +				if (ret) {
> >> +					printf("invalid value: \"%s\" for "
> >> +						"parameter %s\n",
> >> +						optarg,
> >> +						lgopts[option_index].name);
> >> +					print_usage(prgname);
> >> +					return ret;
> >> +				}
> >> +			}
> >> +
> >> +			break;
> >
> > Seems you ignored my comments for v1?
> > Konstantin
> Sorry, I forgot to reply to this comment.
> 'break' here is needed to avoid fall through to the default case.
> If any other long option is passed in command line then application
> should exit.

Please read my previous commnets:
http://dpdk.org/dev/patchwork/patch/23775/
and either address them in a proper manner, or explain why do you think
they are inappropriate.
Till then - NACK.
Konstantin


> Ashish
> >
> >>
> >>   		default:
> >>   			print_usage(prgname);
> >> --
> >> 2.7.4
> >
> >
>