diff mbox series

[1/3] net: new ipv6 header extension parsing function

Message ID 20190508104717.13448-1-marcinx.smoczynski@intel.com (mailing list archive)
State Superseded, archived
Delegated to: akhil goyal
Headers
Series [1/3] net: new ipv6 header extension parsing function |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/intel-Performance-Testing success Performance Testing PASS
ci/mellanox-Performance-Testing success Performance Testing PASS
ci/Intel-compilation fail Compilation issues

Commit Message

Marcin Smoczynski May 8, 2019, 10:47 a.m. UTC 
  Introduce new function for IPv6 header extension parsing able to
determine extension length and next protocol number.

This function is helpful when implementing IPv6 header traversing.

Signed-off-by: Marcin Smoczynski <marcinx.smoczynski@intel.com>
---
 lib/librte_net/rte_ip.h | 49 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

Comments

Ananyev, Konstantin May 14, 2019, 12:48 p.m. UTC | #1 
> 
> Introduce new function for IPv6 header extension parsing able to
> determine extension length and next protocol number.
> 
> This function is helpful when implementing IPv6 header traversing.
> 
> Signed-off-by: Marcin Smoczynski <marcinx.smoczynski@intel.com>
> ---

Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

> 2.21.0.windows.1
Akhil Goyal June 20, 2019, 11:40 a.m. UTC | #2 
Hi Marcin,

> 
> Introduce new function for IPv6 header extension parsing able to
> determine extension length and next protocol number.
> 
> This function is helpful when implementing IPv6 header traversing.
> 
> Signed-off-by: Marcin Smoczynski <marcinx.smoczynski@intel.com>
> ---
>  lib/librte_net/rte_ip.h | 49 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h
> index f9b909090..be64da662 100644
> --- a/lib/librte_net/rte_ip.h
> +++ b/lib/librte_net/rte_ip.h
> @@ -425,6 +425,55 @@ rte_ipv6_udptcp_cksum(const struct ipv6_hdr
> *ipv6_hdr, const void *l4_hdr)
>  	return (uint16_t)cksum;
>  }
> 
> +/* IPv6 fragmentation header size */
> +#define RTE_IPV6_FRAG_HDR_SIZE 8
> +
> +/**
> + * Parse next IPv6 header extension
> + *
> + * This function checks if proto number is an IPv6 extensions and parses its
> + * data if so, providing information on next header and extension length.
> + *
> + * @param p
> + *   Pointer to an extension raw data.
> + * @param proto
> + *   Protocol number extracted from the "next header" field from
> + *   the IPv6 header or the previous extension.
> + * @param ext_len
> + *   Extension data length.
> + * @return
> + *   next protocol number if proto is an IPv6 extension, -EINVAL otherwise
> + */
> +static inline int __rte_experimental


Rte_experimental may not be required for inline functions.


> +rte_ipv6_get_next_ext(uint8_t *p, int proto, size_t *ext_len)
> +{
> +	int next_proto;
> +
> +	switch (proto) {
> +	case IPPROTO_AH:
> +		next_proto = *p++;
> +		*ext_len = (*p + 2) * sizeof(uint32_t);
> +		break;
> +
> +	case IPPROTO_HOPOPTS:
> +	case IPPROTO_ROUTING:
> +	case IPPROTO_DSTOPTS:
> +		next_proto = *p++;
> +		*ext_len = (*p + 1) * sizeof(uint64_t);
> +		break;
> +
> +	case IPPROTO_FRAGMENT:
I see that there is some compilation issues with respect to IPPROTO_xxx fields. These are reported in patchworks if you need details.

Could you please fix these and send next rev.

Thanks,
Akhil
Ananyev, Konstantin June 20, 2019, 5:40 p.m. UTC | #3 
Hi Akhil,

> 
> Hi Marcin,
> 
> >
> > Introduce new function for IPv6 header extension parsing able to
> > determine extension length and next protocol number.
> >
> > This function is helpful when implementing IPv6 header traversing.
> >
> > Signed-off-by: Marcin Smoczynski <marcinx.smoczynski@intel.com>
> > ---
> >  lib/librte_net/rte_ip.h | 49 +++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 49 insertions(+)
> >
> > diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h
> > index f9b909090..be64da662 100644
> > --- a/lib/librte_net/rte_ip.h
> > +++ b/lib/librte_net/rte_ip.h
> > @@ -425,6 +425,55 @@ rte_ipv6_udptcp_cksum(const struct ipv6_hdr
> > *ipv6_hdr, const void *l4_hdr)
> >  	return (uint16_t)cksum;
> >  }
> >
> > +/* IPv6 fragmentation header size */
> > +#define RTE_IPV6_FRAG_HDR_SIZE 8
> > +
> > +/**
> > + * Parse next IPv6 header extension
> > + *
> > + * This function checks if proto number is an IPv6 extensions and parses its
> > + * data if so, providing information on next header and extension length.
> > + *
> > + * @param p
> > + *   Pointer to an extension raw data.
> > + * @param proto
> > + *   Protocol number extracted from the "next header" field from
> > + *   the IPv6 header or the previous extension.
> > + * @param ext_len
> > + *   Extension data length.
> > + * @return
> > + *   next protocol number if proto is an IPv6 extension, -EINVAL otherwise
> > + */
> > +static inline int __rte_experimental
> 
> 
> Rte_experimental may not be required for inline functions.

AFAIK we do need that tag for both inline and non-inline functions,
till API will be transferred to 'stable' state:

$ find lib -type f | xargs grep __rte_experimental | grep inline | wc -l
57

> 
> 
> > +rte_ipv6_get_next_ext(uint8_t *p, int proto, size_t *ext_len)
> > +{
> > +	int next_proto;
> > +
> > +	switch (proto) {
> > +	case IPPROTO_AH:
> > +		next_proto = *p++;
> > +		*ext_len = (*p + 2) * sizeof(uint32_t);
> > +		break;
> > +
> > +	case IPPROTO_HOPOPTS:
> > +	case IPPROTO_ROUTING:
> > +	case IPPROTO_DSTOPTS:
> > +		next_proto = *p++;
> > +		*ext_len = (*p + 1) * sizeof(uint64_t);
> > +		break;
> > +
> > +	case IPPROTO_FRAGMENT:
> I see that there is some compilation issues with respect to IPPROTO_xxx fields. These are reported in patchworks if you need details.
> 
> Could you please fix these and send next rev.
> 
> Thanks,
> Akhil
Akhil Goyal June 21, 2019, 8:01 a.m. UTC | #4 
Hi Konstantin,

> 
> Hi Akhil,
> 
> >
> > Hi Marcin,
> >
> > >
> > > Introduce new function for IPv6 header extension parsing able to
> > > determine extension length and next protocol number.
> > >
> > > This function is helpful when implementing IPv6 header traversing.
> > >
> > > Signed-off-by: Marcin Smoczynski <marcinx.smoczynski@intel.com>
> > > ---
> > >  lib/librte_net/rte_ip.h | 49
> +++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 49 insertions(+)
> > >
> > > diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h
> > > index f9b909090..be64da662 100644
> > > --- a/lib/librte_net/rte_ip.h
> > > +++ b/lib/librte_net/rte_ip.h
> > > @@ -425,6 +425,55 @@ rte_ipv6_udptcp_cksum(const struct ipv6_hdr
> > > *ipv6_hdr, const void *l4_hdr)
> > >  	return (uint16_t)cksum;
> > >  }
> > >
> > > +/* IPv6 fragmentation header size */
> > > +#define RTE_IPV6_FRAG_HDR_SIZE 8
> > > +
> > > +/**
> > > + * Parse next IPv6 header extension
> > > + *
> > > + * This function checks if proto number is an IPv6 extensions and parses its
> > > + * data if so, providing information on next header and extension length.
> > > + *
> > > + * @param p
> > > + *   Pointer to an extension raw data.
> > > + * @param proto
> > > + *   Protocol number extracted from the "next header" field from
> > > + *   the IPv6 header or the previous extension.
> > > + * @param ext_len
> > > + *   Extension data length.
> > > + * @return
> > > + *   next protocol number if proto is an IPv6 extension, -EINVAL otherwise
> > > + */
> > > +static inline int __rte_experimental
> >
> >
> > Rte_experimental may not be required for inline functions.
> 
> AFAIK we do need that tag for both inline and non-inline functions,
> till API will be transferred to 'stable' state:
> 
> $ find lib -type f | xargs grep __rte_experimental | grep inline | wc -l
> 57

My first impression was that this API was not exposed. Inline APIs are normally not exposed, 
So I thought rte_experimental is not required. I believe only exposed APIs
Need to be tagged with rte_experimental.

Now since this API is exposed to the application as well, it should have an entry in the .map file.
Otherwise shared build will fail I think for application.

> 
> >
> >
> > > +rte_ipv6_get_next_ext(uint8_t *p, int proto, size_t *ext_len)
> > > +{
> > > +	int next_proto;
> > > +
> > > +	switch (proto) {
> > > +	case IPPROTO_AH:
> > > +		next_proto = *p++;
> > > +		*ext_len = (*p + 2) * sizeof(uint32_t);
> > > +		break;
> > > +
> > > +	case IPPROTO_HOPOPTS:
> > > +	case IPPROTO_ROUTING:
> > > +	case IPPROTO_DSTOPTS:
> > > +		next_proto = *p++;
> > > +		*ext_len = (*p + 1) * sizeof(uint64_t);
> > > +		break;
> > > +
> > > +	case IPPROTO_FRAGMENT:
> > I see that there is some compilation issues with respect to IPPROTO_xxx fields.
> These are reported in patchworks if you need details.
> >
> > Could you please fix these and send next rev.
> >
> > Thanks,
> > Akhil
Marcin Smoczynski June 24, 2019, 11:45 a.m. UTC | #5 
Hi Akhil,
I've double checked and there is no problem with linking application against shared libraries, because inline functions are being inlined during compilation and before linking is done. To be more specific: there is not such symbol as rte_ipv6_get_next_ext in ipsec-secgw.o (which uses this function in prepare_one_packet function).

There are a number of inline experimental functions defined today  and they are mostly (43 out of 59) not listed in maps, e.g. rte_ticketlock functions family defined here:
lib/librte_eal/common/include/generic/rte_ticketlock.h

Is there any particular reason we should put inlines in maps? Maps are used by linker to control versions of shared libraries and inlines are not processed by linker.

I am aware this patchset causes build failures on BSDs - this is a project-wide issue which I have addressed here:
http://patchwork.dpdk.org/patch/53406/
I'll put a note in the next version of this patchset's cover that it depends on abovementioned patchset.

Marcin
> 
> Hi Konstantin,
> 
> >
> > Hi Akhil,
> >
> > >
> > > Hi Marcin,
> > >
> > > >
> > > > Introduce new function for IPv6 header extension parsing able to
> > > > determine extension length and next protocol number.
> > > >
> > > > This function is helpful when implementing IPv6 header traversing.
> > > >
> > > > Signed-off-by: Marcin Smoczynski <marcinx.smoczynski@intel.com>
> > > > ---
> > > >  lib/librte_net/rte_ip.h | 49
> > +++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 49 insertions(+)
> > > >
> > > > diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h
> > > > index f9b909090..be64da662 100644
> > > > --- a/lib/librte_net/rte_ip.h
> > > > +++ b/lib/librte_net/rte_ip.h
> > > > @@ -425,6 +425,55 @@ rte_ipv6_udptcp_cksum(const struct ipv6_hdr
> > > > *ipv6_hdr, const void *l4_hdr)
> > > >  	return (uint16_t)cksum;
> > > >  }
> > > >
> > > > +/* IPv6 fragmentation header size */ #define
> > > > +RTE_IPV6_FRAG_HDR_SIZE 8
> > > > +
> > > > +/**
> > > > + * Parse next IPv6 header extension
> > > > + *
> > > > + * This function checks if proto number is an IPv6 extensions and
> > > > +parses its
> > > > + * data if so, providing information on next header and extension
> length.
> > > > + *
> > > > + * @param p
> > > > + *   Pointer to an extension raw data.
> > > > + * @param proto
> > > > + *   Protocol number extracted from the "next header" field from
> > > > + *   the IPv6 header or the previous extension.
> > > > + * @param ext_len
> > > > + *   Extension data length.
> > > > + * @return
> > > > + *   next protocol number if proto is an IPv6 extension, -EINVAL
> otherwise
> > > > + */
> > > > +static inline int __rte_experimental
> > >
> > >
> > > Rte_experimental may not be required for inline functions.
> >
> > AFAIK we do need that tag for both inline and non-inline functions,
> > till API will be transferred to 'stable' state:
> >
> > $ find lib -type f | xargs grep __rte_experimental | grep inline | wc
> > -l
> > 57
> 
> My first impression was that this API was not exposed. Inline APIs are
> normally not exposed, So I thought rte_experimental is not required. I
> believe only exposed APIs Need to be tagged with rte_experimental.
> 
> Now since this API is exposed to the application as well, it should have an
> entry in the .map file.
> Otherwise shared build will fail I think for application.
> 
> >
> > >
> > >
> > > > +rte_ipv6_get_next_ext(uint8_t *p, int proto, size_t *ext_len) {
> > > > +	int next_proto;
> > > > +
> > > > +	switch (proto) {
> > > > +	case IPPROTO_AH:
> > > > +		next_proto = *p++;
> > > > +		*ext_len = (*p + 2) * sizeof(uint32_t);
> > > > +		break;
> > > > +
> > > > +	case IPPROTO_HOPOPTS:
> > > > +	case IPPROTO_ROUTING:
> > > > +	case IPPROTO_DSTOPTS:
> > > > +		next_proto = *p++;
> > > > +		*ext_len = (*p + 1) * sizeof(uint64_t);
> > > > +		break;
> > > > +
> > > > +	case IPPROTO_FRAGMENT:
> > > I see that there is some compilation issues with respect to IPPROTO_xxx
> fields.
> > These are reported in patchworks if you need details.
> > >
> > > Could you please fix these and send next rev.
> > >
> > > Thanks,
> > > Akhil
Akhil Goyal June 25, 2019, 12:57 p.m. UTC | #6 
Hi Marcin,

> 
> Hi Akhil,
> I've double checked and there is no problem with linking application against
> shared libraries, because inline functions are being inlined during compilation
> and before linking is done. To be more specific: there is not such symbol as
> rte_ipv6_get_next_ext in ipsec-secgw.o (which uses this function in
> prepare_one_packet function).
> 
> There are a number of inline experimental functions defined today  and they are
> mostly (43 out of 59) not listed in maps, e.g. rte_ticketlock functions family
> defined here:
> lib/librte_eal/common/include/generic/rte_ticketlock.h
> 
> Is there any particular reason we should put inlines in maps? Maps are used by
> linker to control versions of shared libraries and inlines are not processed by
> linker.
> 
> I am aware this patchset causes build failures on BSDs - this is a project-wide
> issue which I have addressed here:
> http://patchwork.dpdk.org/patch/53406/
> I'll put a note in the next version of this patchset's cover that it depends on
> abovementioned patchset.
> 
> Marcin

Thanks for correcting me.
diff mbox series

Patch

diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h
index f9b909090..be64da662 100644
--- a/lib/librte_net/rte_ip.h
+++ b/lib/librte_net/rte_ip.h
@@ -425,6 +425,55 @@  rte_ipv6_udptcp_cksum(const struct ipv6_hdr *ipv6_hdr, const void *l4_hdr)
 	return (uint16_t)cksum;
 }
 
+/* IPv6 fragmentation header size */
+#define RTE_IPV6_FRAG_HDR_SIZE 8
+
+/**
+ * Parse next IPv6 header extension
+ *
+ * This function checks if proto number is an IPv6 extensions and parses its
+ * data if so, providing information on next header and extension length.
+ *
+ * @param p
+ *   Pointer to an extension raw data.
+ * @param proto
+ *   Protocol number extracted from the "next header" field from
+ *   the IPv6 header or the previous extension.
+ * @param ext_len
+ *   Extension data length.
+ * @return
+ *   next protocol number if proto is an IPv6 extension, -EINVAL otherwise
+ */
+static inline int __rte_experimental
+rte_ipv6_get_next_ext(uint8_t *p, int proto, size_t *ext_len)
+{
+	int next_proto;
+
+	switch (proto) {
+	case IPPROTO_AH:
+		next_proto = *p++;
+		*ext_len = (*p + 2) * sizeof(uint32_t);
+		break;
+
+	case IPPROTO_HOPOPTS:
+	case IPPROTO_ROUTING:
+	case IPPROTO_DSTOPTS:
+		next_proto = *p++;
+		*ext_len = (*p + 1) * sizeof(uint64_t);
+		break;
+
+	case IPPROTO_FRAGMENT:
+		next_proto = *p;
+		*ext_len = RTE_IPV6_FRAG_HDR_SIZE;
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	return next_proto;
+}
+
 #ifdef __cplusplus
 }
 #endif