[dpdk-dev,04/12] mbuf: add function to calculate a checksum

Message ID 1469088510-7552-5-git-send-email-olivier.matz@6wind.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers

Commit Message

Olivier Matz July 21, 2016, 8:08 a.m. UTC
  This function can be used to calculate the checksum of data embedded in
mbuf, that can be composed of several segments.

This function will be used by the virtio pmd in next commits to calculate
the checksum in software in case the protocol is not recognized.

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 doc/guides/rel_notes/release_16_11.rst |  5 ++++
 lib/librte_mbuf/rte_mbuf.c             | 55 ++++++++++++++++++++++++++++++++--
 lib/librte_mbuf/rte_mbuf.h             | 13 ++++++++
 lib/librte_mbuf/rte_mbuf_version.map   |  1 +
 4 files changed, 72 insertions(+), 2 deletions(-)
  

Comments

Ananyev, Konstantin July 21, 2016, 10:51 a.m. UTC | #1
Hi Olivier,

> 
> This function can be used to calculate the checksum of data embedded in mbuf, that can be composed of several segments.
> 
> This function will be used by the virtio pmd in next commits to calculate the checksum in software in case the protocol is not recognized.
> 
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> ---
>  doc/guides/rel_notes/release_16_11.rst |  5 ++++
>  lib/librte_mbuf/rte_mbuf.c             | 55 ++++++++++++++++++++++++++++++++--
>  lib/librte_mbuf/rte_mbuf.h             | 13 ++++++++
>  lib/librte_mbuf/rte_mbuf_version.map   |  1 +
>  4 files changed, 72 insertions(+), 2 deletions(-)
> 
> diff --git a/doc/guides/rel_notes/release_16_11.rst b/doc/guides/rel_notes/release_16_11.rst
> index 6a591e2..da70f3b 100644
> --- a/doc/guides/rel_notes/release_16_11.rst
> +++ b/doc/guides/rel_notes/release_16_11.rst
> @@ -53,6 +53,11 @@ New Features
>    Added two new functions ``rte_get_rx_ol_flag_list()`` and
>    ``rte_get_tx_ol_flag_list()`` to dump offload flags as a string.
> 
> +* **Added a functions to calculate the checksum of data in a mbuf.**
> +
> +  Added a new function ``rte_pktmbuf_cksum()`` to process the checksum
> + of  data embedded in an mbuf chain.
> +
>  Resolved Issues
>  ---------------
> 
> diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c index 56f37e6..0304245 100644
> --- a/lib/librte_mbuf/rte_mbuf.c
> +++ b/lib/librte_mbuf/rte_mbuf.c
> @@ -60,6 +60,7 @@
>  #include <rte_hexdump.h>
>  #include <rte_errno.h>
>  #include <rte_memcpy.h>
> +#include <rte_ip.h>

As a nit, do we need to introduce a dependency for librte_mbuf on librte_net?
Might be better to put this functionality into librte_net?
Konstantin
  
don provan July 21, 2016, 4:26 p.m. UTC | #2
> -----Original Message-----
> From: Ananyev, Konstantin [mailto:konstantin.ananyev@intel.com]
> Sent: Thursday, July 21, 2016 3:51 AM
> Subject: Re: [dpdk-dev] [PATCH 04/12] mbuf: add function to calculate a
> checksum
> 
>...
> > +  Added a new function ``rte_pktmbuf_cksum()`` to process the checksum
> > + of  data embedded in an mbuf chain.
> >...
> > +#include <rte_ip.h>
>
> As a nit, do we need to introduce a dependency for librte_mbuf on librte_net?
> Might be better to put this functionality into librte_net?

That's not a nit at all. This is clearly a net function that has no place in the mbuf code.
That should be obvious even before we notice this circular dependency.
-don
dprovan@bivio.net
  
Olivier Matz July 21, 2016, 4:46 p.m. UTC | #3
Dear Don,

On 07/21/2016 06:26 PM, Don Provan wrote:
>> -----Original Message-----
>> From: Ananyev, Konstantin [mailto:konstantin.ananyev@intel.com]
>> Sent: Thursday, July 21, 2016 3:51 AM
>> Subject: Re: [dpdk-dev] [PATCH 04/12] mbuf: add function to calculate a
>> checksum
>>
>> ...
>>> +  Added a new function ``rte_pktmbuf_cksum()`` to process the checksum
>>> + of  data embedded in an mbuf chain.
>>> ...
>>> +#include <rte_ip.h>
>>
>> As a nit, do we need to introduce a dependency for librte_mbuf on librte_net?
>> Might be better to put this functionality into librte_net?
> 
> That's not a nit at all. This is clearly a net function that has no place in the mbuf code.
> That should be obvious even before we notice this circular dependency.


The function is called rte_pktmbuf_cksum(), and takes a mbuf as a
parameter. You cannot haughtily say "it no place in the mbuf code".

As you can see, librte_net only contains headers files. The initial goal
of librte_net was to contain network headers and nothing more.
See:
http://dpdk.org/browse/dpdk/commit/lib/librte_net?id=af75078fece3615088e561357c1e97603e43a5fe

To me, the question of having a dependency in one direction or another
(librte_net needs librte_mbuf, or librte_mbuf needs librte_net) is an
open debate.

I've asked myself the same question when software packet type parsing,
that needs definitions of network headers. As packet_type is a pure mbuf
notion, my choice was to have this parse in mbuf library, and using
network headers definitions provided by librte_net.
  
Olivier Matz July 22, 2016, 8:24 a.m. UTC | #4
Hi Konstantin,

On 07/21/2016 12:51 PM, Ananyev, Konstantin wrote:
> Hi Olivier,
> 
>>
>> This function can be used to calculate the checksum of data embedded in mbuf, that can be composed of several segments.
>>
>> This function will be used by the virtio pmd in next commits to calculate the checksum in software in case the protocol is not recognized.
>>
>> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
>> ---
>>  doc/guides/rel_notes/release_16_11.rst |  5 ++++
>>  lib/librte_mbuf/rte_mbuf.c             | 55 ++++++++++++++++++++++++++++++++--
>>  lib/librte_mbuf/rte_mbuf.h             | 13 ++++++++
>>  lib/librte_mbuf/rte_mbuf_version.map   |  1 +
>>  4 files changed, 72 insertions(+), 2 deletions(-)
>>
>> diff --git a/doc/guides/rel_notes/release_16_11.rst b/doc/guides/rel_notes/release_16_11.rst
>> index 6a591e2..da70f3b 100644
>> --- a/doc/guides/rel_notes/release_16_11.rst
>> +++ b/doc/guides/rel_notes/release_16_11.rst
>> @@ -53,6 +53,11 @@ New Features
>>    Added two new functions ``rte_get_rx_ol_flag_list()`` and
>>    ``rte_get_tx_ol_flag_list()`` to dump offload flags as a string.
>>
>> +* **Added a functions to calculate the checksum of data in a mbuf.**
>> +
>> +  Added a new function ``rte_pktmbuf_cksum()`` to process the checksum
>> + of  data embedded in an mbuf chain.
>> +
>>  Resolved Issues
>>  ---------------
>>
>> diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c index 56f37e6..0304245 100644
>> --- a/lib/librte_mbuf/rte_mbuf.c
>> +++ b/lib/librte_mbuf/rte_mbuf.c
>> @@ -60,6 +60,7 @@
>>  #include <rte_hexdump.h>
>>  #include <rte_errno.h>
>>  #include <rte_memcpy.h>
>> +#include <rte_ip.h>
> 
> As a nit, do we need to introduce a dependency for librte_mbuf on librte_net?
> Might be better to put this functionality into librte_net?

I tried to have this code in librte_net, also when working on the
software packet type parser, and it did not really convince me, mainly
because librte_net is just header files as of today (it's not a real
library). But I can give it a try and post a patch so we can compare,
probably not in the coming days, but I keep a note on it.

Also, as I answered to Don, it would make less sense to move software
packet type parser in librte_net, since it's not a network feature but
more a dpdk mbuf feature. But software packet type needs network headers
definitions... so the cat is eating its tail ;)

Regards,
Olivier
  
Olivier Matz Aug. 29, 2016, 2:52 p.m. UTC | #5
Hi guys,

On 07/22/2016 10:24 AM, Olivier Matz wrote:
>>> diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c index 56f37e6..0304245 100644
>>> --- a/lib/librte_mbuf/rte_mbuf.c
>>> +++ b/lib/librte_mbuf/rte_mbuf.c
>>> @@ -60,6 +60,7 @@
>>>  #include <rte_hexdump.h>
>>>  #include <rte_errno.h>
>>>  #include <rte_memcpy.h>
>>> +#include <rte_ip.h>
>>
>> As a nit, do we need to introduce a dependency for librte_mbuf on librte_net?
>> Might be better to put this functionality into librte_net?
> 
> I tried to have this code in librte_net, also when working on the
> software packet type parser, and it did not really convince me, mainly
> because librte_net is just header files as of today (it's not a real
> library). But I can give it a try and post a patch so we can compare,
> probably not in the coming days, but I keep a note on it.

Back on this. I've just submitted a v2 for the software packet type
patchset:
http://dpdk.org/ml/archives/dev/2016-August/045876.html

As promised, I did the exercice of moving the the software packet type
parser in librte_net. I did it on the sw ptype patchset, but it would be
almost the same for the mbuf checksum function calculation of this patchset.

The most notable differences between v1 and v2 are:

v1:
- rte_pktmbuf_get_ptype() is in librte_mbuf
- only headers in librte_net
- librte_mbuf depends on headers in librte_net

v2:
- rte_net_get_ptype() is in librte_net
- librte_net is now a real library (.so or .a)
- librte_net depends on librte_mbuf

Please, let me know if you have any comment. Depending on them, I'll
adapt the v2 of this patchset too.


Olivier
  

Patch

diff --git a/doc/guides/rel_notes/release_16_11.rst b/doc/guides/rel_notes/release_16_11.rst
index 6a591e2..da70f3b 100644
--- a/doc/guides/rel_notes/release_16_11.rst
+++ b/doc/guides/rel_notes/release_16_11.rst
@@ -53,6 +53,11 @@  New Features
   Added two new functions ``rte_get_rx_ol_flag_list()`` and
   ``rte_get_tx_ol_flag_list()`` to dump offload flags as a string.
 
+* **Added a functions to calculate the checksum of data in a mbuf.**
+
+  Added a new function ``rte_pktmbuf_cksum()`` to process the checksum of
+  data embedded in an mbuf chain.
+
 Resolved Issues
 ---------------
 
diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index 56f37e6..0304245 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -60,6 +60,7 @@ 
 #include <rte_hexdump.h>
 #include <rte_errno.h>
 #include <rte_memcpy.h>
+#include <rte_ip.h>
 
 /*
  * ctrlmbuf constructor, given as a callback function to
@@ -273,8 +274,7 @@  const void *__rte_pktmbuf_read(const struct rte_mbuf *m, uint32_t off,
 	if (off + len > rte_pktmbuf_pkt_len(m))
 		return NULL;
 
-	while (off >= rte_pktmbuf_data_len(seg) &&
-			rte_pktmbuf_data_len(seg) != 0) {
+	while (off >= rte_pktmbuf_data_len(seg)) {
 		off -= rte_pktmbuf_data_len(seg);
 		seg = seg->next;
 	}
@@ -432,3 +432,54 @@  int rte_get_tx_ol_flag_list(uint64_t mask, char *buf, size_t buflen)
 
 	return 0;
 }
+
+/* compute the raw (non complemented) checksum of a packet */
+uint16_t
+rte_pktmbuf_cksum(const struct rte_mbuf *m, uint32_t off, uint32_t len)
+{
+	const struct rte_mbuf *seg;
+	const char *buf;
+	uint32_t sum, tmp;
+	uint32_t seglen, done;
+
+	/* easy case: all data in the first segment */
+	if (off + len <= rte_pktmbuf_data_len(m))
+		return rte_raw_cksum(rte_pktmbuf_mtod_offset(m,
+				const char *, off), len);
+
+	if (off + len > rte_pktmbuf_pkt_len(m))
+		return 0; /* invalid params, return a dummy value */
+
+	/* else browse the segment to find offset */
+	seglen = 0;
+	for (seg = m; seg != NULL; seg = seg->next) {
+		seglen = rte_pktmbuf_data_len(seg);
+		if (off < seglen)
+			break;
+		off -= seglen;
+	}
+	seglen -= off;
+	buf = rte_pktmbuf_mtod_offset(seg, const char *, off);
+	if (seglen >= len) /* all in one segment */
+		return rte_raw_cksum(buf, len);
+
+	/* hard case: process checksum of several segments */
+	sum = 0;
+	done = 0;
+	for (;;) {
+		tmp = __rte_raw_cksum(buf, seglen, 0);
+		if (done & 1)
+			tmp = rte_bswap16(tmp);
+		sum += tmp;
+		done += seglen;
+		if (done == len)
+			break;
+		seg = seg->next;
+		buf = rte_pktmbuf_mtod(seg, const char *);
+		seglen = rte_pktmbuf_data_len(seg);
+		if (seglen > len - done)
+			seglen = len - done;
+	}
+
+	return __rte_raw_cksum_reduce(sum);
+}
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 3c21c71..7bbe096 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -1581,6 +1581,19 @@  static inline int rte_pktmbuf_chain(struct rte_mbuf *head, struct rte_mbuf *tail
  */
 void rte_pktmbuf_dump(FILE *f, const struct rte_mbuf *m, unsigned dump_len);
 
+/**
+ * Compute the raw (non complemented) checksum of a packet.
+ *
+ * @param m
+ *   The pointer to the mbuf.
+ * @param off
+ *   The offset in bytes to start the checksum.
+ * @param len
+ *   The length in bytes of the data to ckecksum.
+ */
+uint16_t
+rte_pktmbuf_cksum(const struct rte_mbuf *m, uint32_t off, uint32_t len);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/lib/librte_mbuf/rte_mbuf_version.map b/lib/librte_mbuf/rte_mbuf_version.map
index 6f83745..7b85dad 100644
--- a/lib/librte_mbuf/rte_mbuf_version.map
+++ b/lib/librte_mbuf/rte_mbuf_version.map
@@ -33,6 +33,7 @@  DPDK_16.11 {
 	rte_get_ptype_tunnel_name;
 	rte_get_rx_ol_flag_list;
 	rte_get_tx_ol_flag_list;
+	rte_pktmbuf_cksum;
 	rte_pktmbuf_get_ptype;
 
 } DPDK_2.1;