| Message ID | 1447179842-2541-1-git-send-email-stuart.haslam@linaro.org |
|---|---|
| State | Accepted |
| Commit | fb98d4c479718301eaedee8d910297e221554b25 |
| Headers | show |
Hello Matias, can you please review and test that patches in your env? To be sure that bug is totally fixed. Thank you, Maxim. On 11/10/2015 21:24, Stuart Haslam wrote: > Fix the way transmit errors are handled to avoid getting out of sync > between kernel and user space, which causes transmission to hang. > > Fixes bug https://bugs.linaro.org/show_bug.cgi?id=1890 > > Signed-off-by: Stuart Haslam <stuart.haslam@linaro.org> > --- > platform/linux-generic/pktio/socket_mmap.c | 59 +++++++++++++++++++++--------- > 1 file changed, 41 insertions(+), 18 deletions(-) > > diff --git a/platform/linux-generic/pktio/socket_mmap.c b/platform/linux-generic/pktio/socket_mmap.c > index 79ff82d..2bdb72b 100644 > --- a/platform/linux-generic/pktio/socket_mmap.c > +++ b/platform/linux-generic/pktio/socket_mmap.c > @@ -182,6 +182,7 @@ static inline unsigned pkt_mmap_v2_tx(int sock, struct ring *ring, > unsigned n, i = 0; > unsigned nb_tx = 0; > int send_errno; > + int total_len = 0; > > first_frame_num = ring->frame_num; > frame_num = first_frame_num; > @@ -195,6 +196,7 @@ static inline unsigned pkt_mmap_v2_tx(int sock, struct ring *ring, > pkt_len = odp_packet_len(pkt_table[i]); > ppd.v2->tp_h.tp_snaplen = pkt_len; > ppd.v2->tp_h.tp_len = pkt_len; > + total_len += pkt_len; > > buf = (uint8_t *)ppd.raw + TPACKET2_HDRLEN - > sizeof(struct sockaddr_ll); > @@ -215,28 +217,49 @@ static inline unsigned pkt_mmap_v2_tx(int sock, struct ring *ring, > * failure a value of -1 is returned, even if the failure occurred > * after some of the packets in the ring have already been sent, so we > * need to inspect the packet status to determine which were sent. */ > - for (frame_num = first_frame_num, n = 0; n < i; ++n) { > - struct tpacket2_hdr *hdr = ring->rd[frame_num].iov_base; > + if (odp_likely(ret == total_len)) { > + nb_tx = i; > + ring->frame_num = frame_num; > + } else if (ret == -1) { > + for (frame_num = first_frame_num, n = 0; n < i; ++n) { > + struct tpacket2_hdr *hdr = ring->rd[frame_num].iov_base; > + > + if (odp_likely(hdr->tp_status == TP_STATUS_AVAILABLE || > + hdr->tp_status == TP_STATUS_SENDING)) { > + nb_tx++; > + } else { > + /* The remaining frames weren't sent, clear > + * their status to indicate we're not waiting > + * for the kernel to process them. */ > + hdr->tp_status = TP_STATUS_AVAILABLE; > + } > > - if (odp_likely(hdr->tp_status == TP_STATUS_AVAILABLE)) { > - nb_tx++; > - } else if (hdr->tp_status & TP_STATUS_WRONG_FORMAT) { > - /* status will be cleared on the next send request */ > - break; > + if (++frame_num >= frame_count) > + frame_num = 0; > } > > - if (++frame_num >= frame_count) > - frame_num = 0; > - } > - > - ring->frame_num = (ring->frame_num + nb_tx) % frame_count; > + ring->frame_num = (first_frame_num + nb_tx) % frame_count; > + > + if (nb_tx == 0 && SOCK_ERR_REPORT(send_errno)) { > + __odp_errno = send_errno; > + /* ENOBUFS indicates that the transmit queue is full, > + * which will happen regularly when overloaded so don't > + * print it */ > + if (errno != ENOBUFS) > + ODP_ERR("sendto(pkt mmap): %s\n", > + strerror(send_errno)); > + return -1; > + } > + } else { > + /* Short send, return value is number of bytes sent so use this > + * to determine number of complete frames sent. */ > + for (n = 0; n < i && ret > 0; ++n) { > + ret -= odp_packet_len(pkt_table[n]); > + if (ret >= 0) > + nb_tx++; > + } > > - if (odp_unlikely(ret == -1 && > - nb_tx == 0 && > - SOCK_ERR_REPORT(send_errno))) { > - __odp_errno = send_errno; > - ODP_ERR("sendto(pkt mmap): %s\n", strerror(send_errno)); > - return -1; > + ring->frame_num = (first_frame_num + nb_tx) % frame_count; > } > > for (i = 0; i < nb_tx; ++i)
Merged, thanks! Maxim. On 11/13/2015 11:37, Elo, Matias (Nokia - FI/Espoo) wrote: > The patch fixes socket mmap transmit bug. For both patches: > > Reviewed-and-Tested-by: Matias Elo <matias.elo@nokia.com> > > While testing the patch set I noticed that the pktio_test_send_failure() doesn't pass when using a directly attached cable between NIC ports. However, this seems to be unrelated to the patch set. > > $ sudo ODP_PKTIO_IF0=p1p1 ODP_PKTIO_IF1=p1p2 ./test/validation/pktio/pktio_main > ... > Test: pktio_test_send_failure ... Received 0 packets > FAILED > 1. pktio.c:394 - CU_FAIL("failed to receive transmitted packet") > 2. pktio.c:943 - CU_FAIL("failed to receive transmitted packets\n") > 3. pktio.c:394 - CU_FAIL("failed to receive transmitted packet") > 4. pktio.c:963 - i == TX_BATCH_LEN > > > -Matias > >> -----Original Message----- >> From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of EXT Stuart >> Haslam >> Sent: Tuesday, November 10, 2015 8:24 PM >> To: lng-odp@lists.linaro.org >> Subject: [lng-odp] [PATCH 1/2] linux-generic: pktio: recover from transmit errors >> >> Fix the way transmit errors are handled to avoid getting out of sync >> between kernel and user space, which causes transmission to hang. >> >> Fixes bug https://bugs.linaro.org/show_bug.cgi?id=1890 >> >> Signed-off-by: Stuart Haslam <stuart.haslam@linaro.org> >> --- >> platform/linux-generic/pktio/socket_mmap.c | 59 +++++++++++++++++++++--- >> ------ >> 1 file changed, 41 insertions(+), 18 deletions(-) >> >> diff --git a/platform/linux-generic/pktio/socket_mmap.c b/platform/linux- >> generic/pktio/socket_mmap.c >> index 79ff82d..2bdb72b 100644 >> --- a/platform/linux-generic/pktio/socket_mmap.c >> +++ b/platform/linux-generic/pktio/socket_mmap.c >> @@ -182,6 +182,7 @@ static inline unsigned pkt_mmap_v2_tx(int sock, struct >> ring *ring, >> unsigned n, i = 0; >> unsigned nb_tx = 0; >> int send_errno; >> + int total_len = 0; >> >> first_frame_num = ring->frame_num; >> frame_num = first_frame_num; >> @@ -195,6 +196,7 @@ static inline unsigned pkt_mmap_v2_tx(int sock, struct >> ring *ring, >> pkt_len = odp_packet_len(pkt_table[i]); >> ppd.v2->tp_h.tp_snaplen = pkt_len; >> ppd.v2->tp_h.tp_len = pkt_len; >> + total_len += pkt_len; >> >> buf = (uint8_t *)ppd.raw + TPACKET2_HDRLEN - >> sizeof(struct sockaddr_ll); >> @@ -215,28 +217,49 @@ static inline unsigned pkt_mmap_v2_tx(int sock, struct >> ring *ring, >> * failure a value of -1 is returned, even if the failure occurred >> * after some of the packets in the ring have already been sent, so we >> * need to inspect the packet status to determine which were sent. */ >> - for (frame_num = first_frame_num, n = 0; n < i; ++n) { >> - struct tpacket2_hdr *hdr = ring->rd[frame_num].iov_base; >> + if (odp_likely(ret == total_len)) { >> + nb_tx = i; >> + ring->frame_num = frame_num; >> + } else if (ret == -1) { >> + for (frame_num = first_frame_num, n = 0; n < i; ++n) { >> + struct tpacket2_hdr *hdr = ring- >>> rd[frame_num].iov_base; >> + >> + if (odp_likely(hdr->tp_status == TP_STATUS_AVAILABLE >> || >> + hdr->tp_status == TP_STATUS_SENDING)) { >> + nb_tx++; >> + } else { >> + /* The remaining frames weren't sent, clear >> + * their status to indicate we're not waiting >> + * for the kernel to process them. */ >> + hdr->tp_status = TP_STATUS_AVAILABLE; >> + } >> >> - if (odp_likely(hdr->tp_status == TP_STATUS_AVAILABLE)) { >> - nb_tx++; >> - } else if (hdr->tp_status & TP_STATUS_WRONG_FORMAT) { >> - /* status will be cleared on the next send request */ >> - break; >> + if (++frame_num >= frame_count) >> + frame_num = 0; >> } >> >> - if (++frame_num >= frame_count) >> - frame_num = 0; >> - } >> - >> - ring->frame_num = (ring->frame_num + nb_tx) % frame_count; >> + ring->frame_num = (first_frame_num + nb_tx) % frame_count; >> + >> + if (nb_tx == 0 && SOCK_ERR_REPORT(send_errno)) { >> + __odp_errno = send_errno; >> + /* ENOBUFS indicates that the transmit queue is full, >> + * which will happen regularly when overloaded so don't >> + * print it */ >> + if (errno != ENOBUFS) >> + ODP_ERR("sendto(pkt mmap): %s\n", >> + strerror(send_errno)); >> + return -1; >> + } >> + } else { >> + /* Short send, return value is number of bytes sent so use this >> + * to determine number of complete frames sent. */ >> + for (n = 0; n < i && ret > 0; ++n) { >> + ret -= odp_packet_len(pkt_table[n]); >> + if (ret >= 0) >> + nb_tx++; >> + } >> >> - if (odp_unlikely(ret == -1 && >> - nb_tx == 0 && >> - SOCK_ERR_REPORT(send_errno))) { >> - __odp_errno = send_errno; >> - ODP_ERR("sendto(pkt mmap): %s\n", strerror(send_errno)); >> - return -1; >> + ring->frame_num = (first_frame_num + nb_tx) % frame_count; >> } >> >> for (i = 0; i < nb_tx; ++i) >> -- >> 2.1.1 >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org >> https://lists.linaro.org/mailman/listinfo/lng-odp > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp
diff --git a/platform/linux-generic/pktio/socket_mmap.c b/platform/linux-generic/pktio/socket_mmap.c index 79ff82d..2bdb72b 100644 --- a/platform/linux-generic/pktio/socket_mmap.c +++ b/platform/linux-generic/pktio/socket_mmap.c @@ -182,6 +182,7 @@ static inline unsigned pkt_mmap_v2_tx(int sock, struct ring *ring, unsigned n, i = 0; unsigned nb_tx = 0; int send_errno; + int total_len = 0; first_frame_num = ring->frame_num; frame_num = first_frame_num; @@ -195,6 +196,7 @@ static inline unsigned pkt_mmap_v2_tx(int sock, struct ring *ring, pkt_len = odp_packet_len(pkt_table[i]); ppd.v2->tp_h.tp_snaplen = pkt_len; ppd.v2->tp_h.tp_len = pkt_len; + total_len += pkt_len; buf = (uint8_t *)ppd.raw + TPACKET2_HDRLEN - sizeof(struct sockaddr_ll); @@ -215,28 +217,49 @@ static inline unsigned pkt_mmap_v2_tx(int sock, struct ring *ring, * failure a value of -1 is returned, even if the failure occurred * after some of the packets in the ring have already been sent, so we * need to inspect the packet status to determine which were sent. */ - for (frame_num = first_frame_num, n = 0; n < i; ++n) { - struct tpacket2_hdr *hdr = ring->rd[frame_num].iov_base; + if (odp_likely(ret == total_len)) { + nb_tx = i; + ring->frame_num = frame_num; + } else if (ret == -1) { + for (frame_num = first_frame_num, n = 0; n < i; ++n) { + struct tpacket2_hdr *hdr = ring->rd[frame_num].iov_base; + + if (odp_likely(hdr->tp_status == TP_STATUS_AVAILABLE || + hdr->tp_status == TP_STATUS_SENDING)) { + nb_tx++; + } else { + /* The remaining frames weren't sent, clear + * their status to indicate we're not waiting + * for the kernel to process them. */ + hdr->tp_status = TP_STATUS_AVAILABLE; + } - if (odp_likely(hdr->tp_status == TP_STATUS_AVAILABLE)) { - nb_tx++; - } else if (hdr->tp_status & TP_STATUS_WRONG_FORMAT) { - /* status will be cleared on the next send request */ - break; + if (++frame_num >= frame_count) + frame_num = 0; } - if (++frame_num >= frame_count) - frame_num = 0; - } - - ring->frame_num = (ring->frame_num + nb_tx) % frame_count; + ring->frame_num = (first_frame_num + nb_tx) % frame_count; + + if (nb_tx == 0 && SOCK_ERR_REPORT(send_errno)) { + __odp_errno = send_errno; + /* ENOBUFS indicates that the transmit queue is full, + * which will happen regularly when overloaded so don't + * print it */ + if (errno != ENOBUFS) + ODP_ERR("sendto(pkt mmap): %s\n", + strerror(send_errno)); + return -1; + } + } else { + /* Short send, return value is number of bytes sent so use this + * to determine number of complete frames sent. */ + for (n = 0; n < i && ret > 0; ++n) { + ret -= odp_packet_len(pkt_table[n]); + if (ret >= 0) + nb_tx++; + } - if (odp_unlikely(ret == -1 && - nb_tx == 0 && - SOCK_ERR_REPORT(send_errno))) { - __odp_errno = send_errno; - ODP_ERR("sendto(pkt mmap): %s\n", strerror(send_errno)); - return -1; + ring->frame_num = (first_frame_num + nb_tx) % frame_count; } for (i = 0; i < nb_tx; ++i)
Fix the way transmit errors are handled to avoid getting out of sync between kernel and user space, which causes transmission to hang. Fixes bug https://bugs.linaro.org/show_bug.cgi?id=1890 Signed-off-by: Stuart Haslam <stuart.haslam@linaro.org> --- platform/linux-generic/pktio/socket_mmap.c | 59 +++++++++++++++++++++--------- 1 file changed, 41 insertions(+), 18 deletions(-)