| Message ID | 1427392347-22709-1-git-send-email-stuart.haslam@linaro.org |
|---|---|
| State | New |
| Headers | show |
Sounds like we should hold v1.0.2 for this bug fix? Maxim: I think this review is properly yours as you did the jumbo frame add for v1.0.1. On Thu, Mar 26, 2015 at 12:52 PM, Stuart Haslam <stuart.haslam@linaro.org> wrote: > A magic number, used to ensure jumbo frames are transmitted and received > in full, is written to the end of a packet buffer at a fixed offset of > 9170 bytes. However for non-jumbo tests this is way beyond the end of > the allocated buffer. > > Signed-off-by: Stuart Haslam <stuart.haslam@linaro.org> > --- > test/validation/odp_pktio.c | 84 > ++++++++++++++++++++++++++++----------------- > 1 file changed, 52 insertions(+), 32 deletions(-) > > diff --git a/test/validation/odp_pktio.c b/test/validation/odp_pktio.c > index e022c33..d653da4 100644 > --- a/test/validation/odp_pktio.c > +++ b/test/validation/odp_pktio.c > @@ -44,7 +44,6 @@ typedef struct ODP_PACKED { > > /** structure of test packet UDP payload */ > typedef struct ODP_PACKED { > - pkt_head_t head; > char data[PKT_BUF_JUMBO_MAX_PAYLOAD - sizeof(pkt_head_t) - > sizeof(uint32be_t)]; > uint32be_t magic2; > @@ -74,33 +73,47 @@ static void pktio_pkt_set_macs(odp_packet_t pkt, > > static uint32_t pkt_payload_len(void) > { > - return test_jumbo ? sizeof(pkt_test_data_t) : sizeof(pkt_head_t); > + uint32_t len; > + > + len = sizeof(pkt_head_t); > + > + if (test_jumbo) > + len += sizeof(pkt_test_data_t); > + > + return len; > } > > static int pktio_pkt_set_seq(odp_packet_t pkt) > { > static uint32_t tstseq; > - size_t l4_off; > - pkt_test_data_t *data; > - uint32_t len = pkt_payload_len(); > - > + size_t off; > + pkt_head_t head; > > - l4_off = odp_packet_l4_offset(pkt); > - if (!l4_off) { > + off = odp_packet_l4_offset(pkt); > + if (!off) { > CU_FAIL("packet L4 offset not set"); > return -1; > } > > - data = calloc(1, len); > - CU_ASSERT_FATAL(data != NULL); > + head.magic = TEST_SEQ_MAGIC; > + head.seq = tstseq; > + > + off += ODPH_UDPHDR_LEN; > + odp_packet_copydata_in(pkt, off, sizeof(head), &head); > + > + if (test_jumbo) { > + pkt_test_data_t *data; > + > + data = calloc(1, sizeof(*data)); > + CU_ASSERT_FATAL(data != NULL); > > - data->head.magic = TEST_SEQ_MAGIC; > - data->magic2 = TEST_SEQ_MAGIC; > - data->head.seq = tstseq; > + data->magic2 = TEST_SEQ_MAGIC; > + > + off += sizeof(head); > + odp_packet_copydata_in(pkt, off, sizeof(*data), data); > + free(data); > + } > > - odp_packet_copydata_in(pkt, l4_off+ODPH_UDPHDR_LEN, > - len, data); > - free(data); > tstseq++; > > return 0; > @@ -108,30 +121,37 @@ static int pktio_pkt_set_seq(odp_packet_t pkt) > > static uint32_t pktio_pkt_seq(odp_packet_t pkt) > { > - size_t l4_off; > + size_t off; > uint32_t seq = TEST_SEQ_INVALID; > - pkt_test_data_t *data; > - uint32_t len = pkt_payload_len(); > + pkt_head_t head; > > - l4_off = odp_packet_l4_offset(pkt); > - if (l4_off == ODP_PACKET_OFFSET_INVALID) > + off = odp_packet_l4_offset(pkt); > + if (off == ODP_PACKET_OFFSET_INVALID) > return TEST_SEQ_INVALID; > > - data = calloc(1, len); > - CU_ASSERT_FATAL(data != NULL); > + off += ODPH_UDPHDR_LEN; > + odp_packet_copydata_out(pkt, off, sizeof(head), &head); > > - odp_packet_copydata_out(pkt, l4_off+ODPH_UDPHDR_LEN, > - len, data); > + if (head.magic != TEST_SEQ_MAGIC) > + return TEST_SEQ_INVALID; > > - if (data->head.magic == TEST_SEQ_MAGIC) { > - if (test_jumbo && data->magic2 != TEST_SEQ_MAGIC) { > - free(data); > - return TEST_SEQ_INVALID; > - } > - seq = data->head.seq; > + seq = head.seq; > + > + if (test_jumbo) { > + pkt_test_data_t *data; > + > + data = calloc(1, sizeof(*data)); > + CU_ASSERT_FATAL(data != NULL); > + > + off += sizeof(head); > + odp_packet_copydata_out(pkt, off, sizeof(*data), data); > + > + if (data->magic2 != TEST_SEQ_MAGIC) > + seq = TEST_SEQ_INVALID; > + > + free(data); > } > > - free(data); > return seq; > } > > -- > 2.1.1 > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp >
On 26 March 2015 at 17:14, Bill Fischofer <bill.fischofer@linaro.org> wrote: > Sounds like we should hold v1.0.2 for this bug fix? Maxim: I think this > review is properly yours as you did the jumbo frame add for v1.0.1. > Bill can you describe the use case than for holding, if we have a strong case for it then we can hold. Normal procedure is that whatever makes the cut is there and the release goes out unless there is a regression introduced. > > On Thu, Mar 26, 2015 at 12:52 PM, Stuart Haslam <stuart.haslam@linaro.org> > wrote: > >> A magic number, used to ensure jumbo frames are transmitted and received >> in full, is written to the end of a packet buffer at a fixed offset of >> 9170 bytes. However for non-jumbo tests this is way beyond the end of >> the allocated buffer. >> >> Signed-off-by: Stuart Haslam <stuart.haslam@linaro.org> >> --- >> test/validation/odp_pktio.c | 84 >> ++++++++++++++++++++++++++++----------------- >> 1 file changed, 52 insertions(+), 32 deletions(-) >> >> diff --git a/test/validation/odp_pktio.c b/test/validation/odp_pktio.c >> index e022c33..d653da4 100644 >> --- a/test/validation/odp_pktio.c >> +++ b/test/validation/odp_pktio.c >> @@ -44,7 +44,6 @@ typedef struct ODP_PACKED { >> >> /** structure of test packet UDP payload */ >> typedef struct ODP_PACKED { >> - pkt_head_t head; >> char data[PKT_BUF_JUMBO_MAX_PAYLOAD - sizeof(pkt_head_t) - >> sizeof(uint32be_t)]; >> uint32be_t magic2; >> @@ -74,33 +73,47 @@ static void pktio_pkt_set_macs(odp_packet_t pkt, >> >> static uint32_t pkt_payload_len(void) >> { >> - return test_jumbo ? sizeof(pkt_test_data_t) : sizeof(pkt_head_t); >> + uint32_t len; >> + >> + len = sizeof(pkt_head_t); >> + >> + if (test_jumbo) >> + len += sizeof(pkt_test_data_t); >> + >> + return len; >> } >> >> static int pktio_pkt_set_seq(odp_packet_t pkt) >> { >> static uint32_t tstseq; >> - size_t l4_off; >> - pkt_test_data_t *data; >> - uint32_t len = pkt_payload_len(); >> - >> + size_t off; >> + pkt_head_t head; >> >> - l4_off = odp_packet_l4_offset(pkt); >> - if (!l4_off) { >> + off = odp_packet_l4_offset(pkt); >> + if (!off) { >> CU_FAIL("packet L4 offset not set"); >> return -1; >> } >> >> - data = calloc(1, len); >> - CU_ASSERT_FATAL(data != NULL); >> + head.magic = TEST_SEQ_MAGIC; >> + head.seq = tstseq; >> + >> + off += ODPH_UDPHDR_LEN; >> + odp_packet_copydata_in(pkt, off, sizeof(head), &head); >> + >> + if (test_jumbo) { >> + pkt_test_data_t *data; >> + >> + data = calloc(1, sizeof(*data)); >> + CU_ASSERT_FATAL(data != NULL); >> >> - data->head.magic = TEST_SEQ_MAGIC; >> - data->magic2 = TEST_SEQ_MAGIC; >> - data->head.seq = tstseq; >> + data->magic2 = TEST_SEQ_MAGIC; >> + >> + off += sizeof(head); >> + odp_packet_copydata_in(pkt, off, sizeof(*data), data); >> + free(data); >> + } >> >> - odp_packet_copydata_in(pkt, l4_off+ODPH_UDPHDR_LEN, >> - len, data); >> - free(data); >> tstseq++; >> >> return 0; >> @@ -108,30 +121,37 @@ static int pktio_pkt_set_seq(odp_packet_t pkt) >> >> static uint32_t pktio_pkt_seq(odp_packet_t pkt) >> { >> - size_t l4_off; >> + size_t off; >> uint32_t seq = TEST_SEQ_INVALID; >> - pkt_test_data_t *data; >> - uint32_t len = pkt_payload_len(); >> + pkt_head_t head; >> >> - l4_off = odp_packet_l4_offset(pkt); >> - if (l4_off == ODP_PACKET_OFFSET_INVALID) >> + off = odp_packet_l4_offset(pkt); >> + if (off == ODP_PACKET_OFFSET_INVALID) >> return TEST_SEQ_INVALID; >> >> - data = calloc(1, len); >> - CU_ASSERT_FATAL(data != NULL); >> + off += ODPH_UDPHDR_LEN; >> + odp_packet_copydata_out(pkt, off, sizeof(head), &head); >> >> - odp_packet_copydata_out(pkt, l4_off+ODPH_UDPHDR_LEN, >> - len, data); >> + if (head.magic != TEST_SEQ_MAGIC) >> + return TEST_SEQ_INVALID; >> >> - if (data->head.magic == TEST_SEQ_MAGIC) { >> - if (test_jumbo && data->magic2 != TEST_SEQ_MAGIC) { >> - free(data); >> - return TEST_SEQ_INVALID; >> - } >> - seq = data->head.seq; >> + seq = head.seq; >> + >> + if (test_jumbo) { >> + pkt_test_data_t *data; >> + >> + data = calloc(1, sizeof(*data)); >> + CU_ASSERT_FATAL(data != NULL); >> + >> + off += sizeof(head); >> + odp_packet_copydata_out(pkt, off, sizeof(*data), data); >> + >> + if (data->magic2 != TEST_SEQ_MAGIC) >> + seq = TEST_SEQ_INVALID; >> + >> + free(data); >> } >> >> - free(data); >> return seq; >> } >> >> -- >> 2.1.1 >> >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org >> http://lists.linaro.org/mailman/listinfo/lng-odp >> > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp > >
If I understand Stuart's patch correctly, he's saying that the introduction of jumbo frame support in v1.0.1 broke non-jumbo processing. Since that's a critical function I'd think we'd want to fix that bug ASAP in v1.0.2. On Fri, Mar 27, 2015 at 7:04 AM, Mike Holmes <mike.holmes@linaro.org> wrote: > > > On 26 March 2015 at 17:14, Bill Fischofer <bill.fischofer@linaro.org> > wrote: > >> Sounds like we should hold v1.0.2 for this bug fix? Maxim: I think this >> review is properly yours as you did the jumbo frame add for v1.0.1. >> > > Bill can you describe the use case than for holding, if we have a strong > case for it then we can hold. > > Normal procedure is that whatever makes the cut is there and the release > goes out unless there is a regression introduced. > > >> >> On Thu, Mar 26, 2015 at 12:52 PM, Stuart Haslam <stuart.haslam@linaro.org >> > wrote: >> >>> A magic number, used to ensure jumbo frames are transmitted and received >>> in full, is written to the end of a packet buffer at a fixed offset of >>> 9170 bytes. However for non-jumbo tests this is way beyond the end of >>> the allocated buffer. >>> >>> Signed-off-by: Stuart Haslam <stuart.haslam@linaro.org> >>> --- >>> test/validation/odp_pktio.c | 84 >>> ++++++++++++++++++++++++++++----------------- >>> 1 file changed, 52 insertions(+), 32 deletions(-) >>> >>> diff --git a/test/validation/odp_pktio.c b/test/validation/odp_pktio.c >>> index e022c33..d653da4 100644 >>> --- a/test/validation/odp_pktio.c >>> +++ b/test/validation/odp_pktio.c >>> @@ -44,7 +44,6 @@ typedef struct ODP_PACKED { >>> >>> /** structure of test packet UDP payload */ >>> typedef struct ODP_PACKED { >>> - pkt_head_t head; >>> char data[PKT_BUF_JUMBO_MAX_PAYLOAD - sizeof(pkt_head_t) - >>> sizeof(uint32be_t)]; >>> uint32be_t magic2; >>> @@ -74,33 +73,47 @@ static void pktio_pkt_set_macs(odp_packet_t pkt, >>> >>> static uint32_t pkt_payload_len(void) >>> { >>> - return test_jumbo ? sizeof(pkt_test_data_t) : sizeof(pkt_head_t); >>> + uint32_t len; >>> + >>> + len = sizeof(pkt_head_t); >>> + >>> + if (test_jumbo) >>> + len += sizeof(pkt_test_data_t); >>> + >>> + return len; >>> } >>> >>> static int pktio_pkt_set_seq(odp_packet_t pkt) >>> { >>> static uint32_t tstseq; >>> - size_t l4_off; >>> - pkt_test_data_t *data; >>> - uint32_t len = pkt_payload_len(); >>> - >>> + size_t off; >>> + pkt_head_t head; >>> >>> - l4_off = odp_packet_l4_offset(pkt); >>> - if (!l4_off) { >>> + off = odp_packet_l4_offset(pkt); >>> + if (!off) { >>> CU_FAIL("packet L4 offset not set"); >>> return -1; >>> } >>> >>> - data = calloc(1, len); >>> - CU_ASSERT_FATAL(data != NULL); >>> + head.magic = TEST_SEQ_MAGIC; >>> + head.seq = tstseq; >>> + >>> + off += ODPH_UDPHDR_LEN; >>> + odp_packet_copydata_in(pkt, off, sizeof(head), &head); >>> + >>> + if (test_jumbo) { >>> + pkt_test_data_t *data; >>> + >>> + data = calloc(1, sizeof(*data)); >>> + CU_ASSERT_FATAL(data != NULL); >>> >>> - data->head.magic = TEST_SEQ_MAGIC; >>> - data->magic2 = TEST_SEQ_MAGIC; >>> - data->head.seq = tstseq; >>> + data->magic2 = TEST_SEQ_MAGIC; >>> + >>> + off += sizeof(head); >>> + odp_packet_copydata_in(pkt, off, sizeof(*data), data); >>> + free(data); >>> + } >>> >>> - odp_packet_copydata_in(pkt, l4_off+ODPH_UDPHDR_LEN, >>> - len, data); >>> - free(data); >>> tstseq++; >>> >>> return 0; >>> @@ -108,30 +121,37 @@ static int pktio_pkt_set_seq(odp_packet_t pkt) >>> >>> static uint32_t pktio_pkt_seq(odp_packet_t pkt) >>> { >>> - size_t l4_off; >>> + size_t off; >>> uint32_t seq = TEST_SEQ_INVALID; >>> - pkt_test_data_t *data; >>> - uint32_t len = pkt_payload_len(); >>> + pkt_head_t head; >>> >>> - l4_off = odp_packet_l4_offset(pkt); >>> - if (l4_off == ODP_PACKET_OFFSET_INVALID) >>> + off = odp_packet_l4_offset(pkt); >>> + if (off == ODP_PACKET_OFFSET_INVALID) >>> return TEST_SEQ_INVALID; >>> >>> - data = calloc(1, len); >>> - CU_ASSERT_FATAL(data != NULL); >>> + off += ODPH_UDPHDR_LEN; >>> + odp_packet_copydata_out(pkt, off, sizeof(head), &head); >>> >>> - odp_packet_copydata_out(pkt, l4_off+ODPH_UDPHDR_LEN, >>> - len, data); >>> + if (head.magic != TEST_SEQ_MAGIC) >>> + return TEST_SEQ_INVALID; >>> >>> - if (data->head.magic == TEST_SEQ_MAGIC) { >>> - if (test_jumbo && data->magic2 != TEST_SEQ_MAGIC) { >>> - free(data); >>> - return TEST_SEQ_INVALID; >>> - } >>> - seq = data->head.seq; >>> + seq = head.seq; >>> + >>> + if (test_jumbo) { >>> + pkt_test_data_t *data; >>> + >>> + data = calloc(1, sizeof(*data)); >>> + CU_ASSERT_FATAL(data != NULL); >>> + >>> + off += sizeof(head); >>> + odp_packet_copydata_out(pkt, off, sizeof(*data), data); >>> + >>> + if (data->magic2 != TEST_SEQ_MAGIC) >>> + seq = TEST_SEQ_INVALID; >>> + >>> + free(data); >>> } >>> >>> - free(data); >>> return seq; >>> } >>> >>> -- >>> 2.1.1 >>> >>> >>> _______________________________________________ >>> lng-odp mailing list >>> lng-odp@lists.linaro.org >>> http://lists.linaro.org/mailman/listinfo/lng-odp >>> >> >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org >> http://lists.linaro.org/mailman/listinfo/lng-odp >> >> > > > -- > Mike Holmes > Technical Manager - Linaro Networking Group > Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM SoCs > > >
diff --git a/test/validation/odp_pktio.c b/test/validation/odp_pktio.c index e022c33..d653da4 100644 --- a/test/validation/odp_pktio.c +++ b/test/validation/odp_pktio.c @@ -44,7 +44,6 @@ typedef struct ODP_PACKED { /** structure of test packet UDP payload */ typedef struct ODP_PACKED { - pkt_head_t head; char data[PKT_BUF_JUMBO_MAX_PAYLOAD - sizeof(pkt_head_t) - sizeof(uint32be_t)]; uint32be_t magic2; @@ -74,33 +73,47 @@ static void pktio_pkt_set_macs(odp_packet_t pkt, static uint32_t pkt_payload_len(void) { - return test_jumbo ? sizeof(pkt_test_data_t) : sizeof(pkt_head_t); + uint32_t len; + + len = sizeof(pkt_head_t); + + if (test_jumbo) + len += sizeof(pkt_test_data_t); + + return len; } static int pktio_pkt_set_seq(odp_packet_t pkt) { static uint32_t tstseq; - size_t l4_off; - pkt_test_data_t *data; - uint32_t len = pkt_payload_len(); - + size_t off; + pkt_head_t head; - l4_off = odp_packet_l4_offset(pkt); - if (!l4_off) { + off = odp_packet_l4_offset(pkt); + if (!off) { CU_FAIL("packet L4 offset not set"); return -1; } - data = calloc(1, len); - CU_ASSERT_FATAL(data != NULL); + head.magic = TEST_SEQ_MAGIC; + head.seq = tstseq; + + off += ODPH_UDPHDR_LEN; + odp_packet_copydata_in(pkt, off, sizeof(head), &head); + + if (test_jumbo) { + pkt_test_data_t *data; + + data = calloc(1, sizeof(*data)); + CU_ASSERT_FATAL(data != NULL); - data->head.magic = TEST_SEQ_MAGIC; - data->magic2 = TEST_SEQ_MAGIC; - data->head.seq = tstseq; + data->magic2 = TEST_SEQ_MAGIC; + + off += sizeof(head); + odp_packet_copydata_in(pkt, off, sizeof(*data), data); + free(data); + } - odp_packet_copydata_in(pkt, l4_off+ODPH_UDPHDR_LEN, - len, data); - free(data); tstseq++; return 0; @@ -108,30 +121,37 @@ static int pktio_pkt_set_seq(odp_packet_t pkt) static uint32_t pktio_pkt_seq(odp_packet_t pkt) { - size_t l4_off; + size_t off; uint32_t seq = TEST_SEQ_INVALID; - pkt_test_data_t *data; - uint32_t len = pkt_payload_len(); + pkt_head_t head; - l4_off = odp_packet_l4_offset(pkt); - if (l4_off == ODP_PACKET_OFFSET_INVALID) + off = odp_packet_l4_offset(pkt); + if (off == ODP_PACKET_OFFSET_INVALID) return TEST_SEQ_INVALID; - data = calloc(1, len); - CU_ASSERT_FATAL(data != NULL); + off += ODPH_UDPHDR_LEN; + odp_packet_copydata_out(pkt, off, sizeof(head), &head); - odp_packet_copydata_out(pkt, l4_off+ODPH_UDPHDR_LEN, - len, data); + if (head.magic != TEST_SEQ_MAGIC) + return TEST_SEQ_INVALID; - if (data->head.magic == TEST_SEQ_MAGIC) { - if (test_jumbo && data->magic2 != TEST_SEQ_MAGIC) { - free(data); - return TEST_SEQ_INVALID; - } - seq = data->head.seq; + seq = head.seq; + + if (test_jumbo) { + pkt_test_data_t *data; + + data = calloc(1, sizeof(*data)); + CU_ASSERT_FATAL(data != NULL); + + off += sizeof(head); + odp_packet_copydata_out(pkt, off, sizeof(*data), data); + + if (data->magic2 != TEST_SEQ_MAGIC) + seq = TEST_SEQ_INVALID; + + free(data); } - free(data); return seq; }
A magic number, used to ensure jumbo frames are transmitted and received in full, is written to the end of a packet buffer at a fixed offset of 9170 bytes. However for non-jumbo tests this is way beyond the end of the allocated buffer. Signed-off-by: Stuart Haslam <stuart.haslam@linaro.org> --- test/validation/odp_pktio.c | 84 ++++++++++++++++++++++++++++----------------- 1 file changed, 52 insertions(+), 32 deletions(-)