| Message ID | 1477548924-26376-4-git-send-email-qiudayu@chinac.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Daniele Di Proietto |
| Headers | show |
> This patch didn't apply cleanly either; 'apply -3' resolved the issue, but I recommend rebasing to HEAD of master for next version. Other than that, just some minor cosmetic comments inline. >When a packet is from DPDK source, and it contains >multipule segments, data_len is not equal to the Typo in the previous line - 'multipule' >packet size. This patch fix this issue. > >Signed-off-by: Michael Qiu <qiudayu@chinac.com> >Signed-off-by: Marcin Ksiadz <marcinx.ksiadz@intel.com> >Signed-off-by: Mark Kavanagh <mark.b.kavanagh@intel.com> >Signed-off-by: Przemyslaw Lal <przemyslawx.lal@intel.com> >Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com> >--- > lib/dp-packet.h | 27 +++++++++++++++++---------- > 1 file changed, 17 insertions(+), 10 deletions(-) > >diff --git a/lib/dp-packet.h b/lib/dp-packet.h >index 7c1e637..b5a55bb 100644 >--- a/lib/dp-packet.h >+++ b/lib/dp-packet.h >@@ -24,6 +24,10 @@ > #include "util.h" > #include "netdev-dpdk.h" > >+#ifdef DPDK_NETDEV >+#include "rte_ether.h" >+#endif >+ > #ifdef __cplusplus > extern "C" { > #endif >@@ -409,17 +413,20 @@ dp_packet_size(const struct dp_packet *b) > static inline void > dp_packet_set_size(struct dp_packet *b, uint32_t v) > { >- /* netdev-dpdk does not currently support segmentation; consequently, for >- * all intents and purposes, 'data_len' (16 bit) and 'pkt_len' (32 bit) may >- * be used interchangably. >- * >- * On the datapath, it is expected that the size of packets >- * (and thus 'v') will always be <= UINT16_MAX; this means that there is no >- * loss of accuracy in assigning 'v' to 'data_len'. >+ /* >+ * Assign current segment length. If total length is greater than >+ * max data length in a segment, additional calculation is needed > */ >- b->mbuf.data_len = (uint16_t)v; /* Current seg length. */ >- b->mbuf.pkt_len = v; /* Total length of all segments linked to >- * this segment. */ >+ if (v > (b->mbuf.buf_len - b->mbuf.data_off)) { >+ b->mbuf.data_len = >+ (uint16_t)(b->mbuf.buf_len - b->mbuf.data_off); Coding style: there should be a space between cast type and attribute being cast (this mistake was present in the previous code, to be fair). >+ } else { >+ b->mbuf.data_len = (uint16_t)v; As above. >+ } >+ >+ /* Total length of all segments linked to >+ * this segment. */ It might be clearer to phrase it along the lines of 'Total length of all packet segments'; also, with its current length, this comment can fit on one line. >+ b->mbuf.pkt_len = v; > } > > static inline uint16_t >-- >1.8.3.1
2016/10/28 18:09, Kavanagh, Mark B : > This patch didn't apply cleanly either; 'apply -3' resolved the issue, but I recommend rebasing to HEAD of master for next version. > > Other than that, just some minor cosmetic comments inline. > >> When a packet is from DPDK source, and it contains >> multipule segments, data_len is not equal to the > Typo in the previous line - 'multipule' Thanks for point it out, I'll fix it. > >> packet size. This patch fix this issue. >> >> Signed-off-by: Michael Qiu <qiudayu@chinac.com> >> Signed-off-by: Marcin Ksiadz <marcinx.ksiadz@intel.com> >> Signed-off-by: Mark Kavanagh <mark.b.kavanagh@intel.com> >> Signed-off-by: Przemyslaw Lal <przemyslawx.lal@intel.com> >> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com> >> --- >> lib/dp-packet.h | 27 +++++++++++++++++---------- >> 1 file changed, 17 insertions(+), 10 deletions(-) >> >> diff --git a/lib/dp-packet.h b/lib/dp-packet.h >> index 7c1e637..b5a55bb 100644 >> --- a/lib/dp-packet.h >> +++ b/lib/dp-packet.h >> @@ -24,6 +24,10 @@ >> #include "util.h" >> #include "netdev-dpdk.h" >> >> +#ifdef DPDK_NETDEV >> +#include "rte_ether.h" >> +#endif >> + >> #ifdef __cplusplus >> extern "C" { >> #endif >> @@ -409,17 +413,20 @@ dp_packet_size(const struct dp_packet *b) >> static inline void >> dp_packet_set_size(struct dp_packet *b, uint32_t v) >> { >> - /* netdev-dpdk does not currently support segmentation; consequently, for >> - * all intents and purposes, 'data_len' (16 bit) and 'pkt_len' (32 bit) may >> - * be used interchangably. >> - * >> - * On the datapath, it is expected that the size of packets >> - * (and thus 'v') will always be <= UINT16_MAX; this means that there is no >> - * loss of accuracy in assigning 'v' to 'data_len'. >> + /* >> + * Assign current segment length. If total length is greater than >> + * max data length in a segment, additional calculation is needed >> */ >> - b->mbuf.data_len = (uint16_t)v; /* Current seg length. */ >> - b->mbuf.pkt_len = v; /* Total length of all segments linked to >> - * this segment. */ >> + if (v > (b->mbuf.buf_len - b->mbuf.data_off)) { >> + b->mbuf.data_len = >> + (uint16_t)(b->mbuf.buf_len - b->mbuf.data_off); > Coding style: there should be a space between cast type and attribute being cast (this mistake was present in the previous code, to be fair). OK, I will fix it :) > >> + } else { >> + b->mbuf.data_len = (uint16_t)v; > As above. > >> + } >> + >> + /* Total length of all segments linked to >> + * this segment. */ > It might be clearer to phrase it along the lines of 'Total length of all packet segments'; also, with its current length, this comment can fit on one line. Yes, you are right. I will fix. > >> + b->mbuf.pkt_len = v; >> } >> >> static inline uint16_t >> -- >> 1.8.3.1 >
2016/10/28 18:09, Kavanagh, Mark B : > This patch didn't apply cleanly either; 'apply -3' resolved the issue, but I recommend rebasing to HEAD of master for next version. > > Other than that, just some minor cosmetic comments inline. > >> When a packet is from DPDK source, and it contains >> multipule segments, data_len is not equal to the > Typo in the previous line - 'multipule' Thanks for point it out, I'll fix it. > >> packet size. This patch fix this issue. >> >> Signed-off-by: Michael Qiu <qiudayu@chinac.com> >> Signed-off-by: Marcin Ksiadz <marcinx.ksiadz@intel.com> >> Signed-off-by: Mark Kavanagh <mark.b.kavanagh@intel.com> >> Signed-off-by: Przemyslaw Lal <przemyslawx.lal@intel.com> >> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com> >> --- >> lib/dp-packet.h | 27 +++++++++++++++++---------- >> 1 file changed, 17 insertions(+), 10 deletions(-) >> >> diff --git a/lib/dp-packet.h b/lib/dp-packet.h >> index 7c1e637..b5a55bb 100644 >> --- a/lib/dp-packet.h >> +++ b/lib/dp-packet.h >> @@ -24,6 +24,10 @@ >> #include "util.h" >> #include "netdev-dpdk.h" >> >> +#ifdef DPDK_NETDEV >> +#include "rte_ether.h" >> +#endif >> + >> #ifdef __cplusplus >> extern "C" { >> #endif >> @@ -409,17 +413,20 @@ dp_packet_size(const struct dp_packet *b) >> static inline void >> dp_packet_set_size(struct dp_packet *b, uint32_t v) >> { >> - /* netdev-dpdk does not currently support segmentation; consequently, for >> - * all intents and purposes, 'data_len' (16 bit) and 'pkt_len' (32 bit) may >> - * be used interchangably. >> - * >> - * On the datapath, it is expected that the size of packets >> - * (and thus 'v') will always be <= UINT16_MAX; this means that there is no >> - * loss of accuracy in assigning 'v' to 'data_len'. >> + /* >> + * Assign current segment length. If total length is greater than >> + * max data length in a segment, additional calculation is needed >> */ >> - b->mbuf.data_len = (uint16_t)v; /* Current seg length. */ >> - b->mbuf.pkt_len = v; /* Total length of all segments linked to >> - * this segment. */ >> + if (v > (b->mbuf.buf_len - b->mbuf.data_off)) { >> + b->mbuf.data_len = >> + (uint16_t)(b->mbuf.buf_len - b->mbuf.data_off); > Coding style: there should be a space between cast type and attribute being cast (this mistake was present in the previous code, to be fair). OK, I will fix it :) > >> + } else { >> + b->mbuf.data_len = (uint16_t)v; > As above. > >> + } >> + >> + /* Total length of all segments linked to >> + * this segment. */ > It might be clearer to phrase it along the lines of 'Total length of all packet segments'; also, with its current length, this comment can fit on one line. Yes, you are right. I will fix. > >> + b->mbuf.pkt_len = v; >> } >> >> static inline uint16_t >> -- >> 1.8.3.1 >
diff --git a/lib/dp-packet.h b/lib/dp-packet.h index 7c1e637..b5a55bb 100644 --- a/lib/dp-packet.h +++ b/lib/dp-packet.h @@ -24,6 +24,10 @@ #include "util.h" #include "netdev-dpdk.h" +#ifdef DPDK_NETDEV +#include "rte_ether.h" +#endif + #ifdef __cplusplus extern "C" { #endif @@ -409,17 +413,20 @@ dp_packet_size(const struct dp_packet *b) static inline void dp_packet_set_size(struct dp_packet *b, uint32_t v) { - /* netdev-dpdk does not currently support segmentation; consequently, for - * all intents and purposes, 'data_len' (16 bit) and 'pkt_len' (32 bit) may - * be used interchangably. - * - * On the datapath, it is expected that the size of packets - * (and thus 'v') will always be <= UINT16_MAX; this means that there is no - * loss of accuracy in assigning 'v' to 'data_len'. + /* + * Assign current segment length. If total length is greater than + * max data length in a segment, additional calculation is needed */ - b->mbuf.data_len = (uint16_t)v; /* Current seg length. */ - b->mbuf.pkt_len = v; /* Total length of all segments linked to - * this segment. */ + if (v > (b->mbuf.buf_len - b->mbuf.data_off)) { + b->mbuf.data_len = + (uint16_t)(b->mbuf.buf_len - b->mbuf.data_off); + } else { + b->mbuf.data_len = (uint16_t)v; + } + + /* Total length of all segments linked to + * this segment. */ + b->mbuf.pkt_len = v; } static inline uint16_t