On Wed, Jun 03, 2015 at 06:56:00PM +0100, Zoltan Kiss wrote: > > > On 03/06/15 18:00, Stuart Haslam wrote: > >On Wed, Jun 03, 2015 at 05:44:19PM +0100, Zoltan Kiss wrote: > >>And change the behaviour in linux-generic implementation, where it's > >>releasing > >>it during the call. > >> > >>Signed-off-by: Zoltan Kiss <[email protected]> > >>--- > >> example/packet/odp_pktio.c | 9 ++++++++- > >> platform/linux-generic/odp_packet_io.c | 10 +++++++++- > >> platform/linux-generic/odp_packet_socket.c | 6 ------ > >> test/performance/odp_l2fwd.c | 11 +++++++++-- > >> 4 files changed, 26 insertions(+), 10 deletions(-) > >> > >>diff --git a/example/packet/odp_pktio.c b/example/packet/odp_pktio.c > >>index f08d9f4..4242caa 100644 > >>--- a/example/packet/odp_pktio.c > >>+++ b/example/packet/odp_pktio.c > >>@@ -282,9 +282,16 @@ static void *pktio_ifburst_thread(void *arg) > >> /* Drop packets with errors */ > >> pkts_ok = drop_err_pkts(pkt_tbl, pkts); > >> if (pkts_ok > 0) { > >>+ int sent; > >> /* Swap Eth MACs and IP-addrs */ > >> swap_pkt_addrs(pkt_tbl, pkts_ok); > >>- odp_pktio_send(pktio, pkt_tbl, pkts_ok); > >>+ sent = odp_pktio_send(pktio, pkt_tbl, pkts_ok); > >>+ if (odp_unlikely(sent < pkts_ok)) { > >>+ err_cnt += pkts_ok - sent; > >>+ do > >>+ > >>odp_packet_free(pkt_tbl[sent++]); > >>+ while (++sent < pkts_ok); > > > >You're incrementing sent twice. > > Indeed, I'll fix that. > > > > >>+ } > >> } > >> > >> if (odp_unlikely(pkts_ok != pkts)) > >>diff --git a/platform/linux-generic/odp_packet_io.c > >>b/platform/linux-generic/odp_packet_io.c > >>index 5ae24b9..6e7db41 100644 > >>--- a/platform/linux-generic/odp_packet_io.c > >>+++ b/platform/linux-generic/odp_packet_io.c > >>@@ -564,6 +564,9 @@ int pktout_enqueue(queue_entry_t *qentry, > >>odp_buffer_hdr_t *buf_hdr) > >> int nbr; > >> > >> nbr = odp_pktio_send(qentry->s.pktout, &pkt, len); > >>+ if (odp_unlikely(nbr == 0)) > >>+ odp_packet_free(pkt); > >>+ > > > >Why do this here rather than letting the caller of enqueue decide how > >to deal with the failure? > > This is bringing up an another issue: we should define the same > behaviour for odp_queue_enq() and odp_queue_enq_multi(), shouldn't > we? >
Yes, Leo mentioned that in the egress API document. > > > >> return (nbr == len ? 0 : -1); > >> } > >> > >>@@ -583,7 +586,12 @@ int pktout_enq_multi(queue_entry_t *qentry, > >>odp_buffer_hdr_t *buf_hdr[], > >> for (i = 0; i < num; ++i) > >> pkt_tbl[i] = _odp_packet_from_buffer(buf_hdr[i]->handle.handle); > >> > >>- nbr = odp_pktio_send(qentry->s.pktout, pkt_tbl, num); > >>+ i = nbr = odp_pktio_send(qentry->s.pktout, pkt_tbl, num); > >>+ if (odp_unlikely(i < num)) > >>+ do > >>+ odp_packet_free(pkt_tbl[i]); > >>+ while (++i < num); > >>+ > > > >Same comment as above. > > > >> return nbr; > >> } > >> > >>diff --git a/platform/linux-generic/odp_packet_socket.c > >>b/platform/linux-generic/odp_packet_socket.c > >>index 9272146..309980d 100644 > >>--- a/platform/linux-generic/odp_packet_socket.c > >>+++ b/platform/linux-generic/odp_packet_socket.c > >>@@ -301,9 +301,6 @@ int send_pkt_sock_basic(pkt_sock_t *const pkt_sock, > >> } /* end while */ > >> nb_tx = i; > >> > >>- for (i = 0; i < len; i++) > >>- odp_packet_free(pkt_table[i]); > >>- > > > >Now packets that were actually sent are being leaked. > I'll fix these too > By the way I submitted a patch (https://patches.linaro.org/46723/) a while back to tidy up the handling of transmit errors in odp_packet_socket.c. There's an outstanding comment I need to address, I'll send an updated version later today or tomorrow. -- Stuart. _______________________________________________ lng-odp mailing list [email protected] https://lists.linaro.org/mailman/listinfo/lng-odp
