| Message ID | 1416087158-6149-1-git-send-email-maxim.uvarov@linaro.org |
|---|---|
| State | Accepted |
| Commit | dc4bf89e36476b8cd31f3fdc1a2bb075af20f8ff |
| Headers | show |
On Sat, Nov 15, 2014 at 11:32 PM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > Keep original logic but make function more clear. > > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> > --- > platform/linux-generic/odp_packet_io.c | 62 +++++++++++++++------------------- > 1 file changed, 27 insertions(+), 35 deletions(-) > > diff --git a/platform/linux-generic/odp_packet_io.c b/platform/linux-generic/odp_packet_io.c > index f35193f..d4af13e 100644 > --- a/platform/linux-generic/odp_packet_io.c > +++ b/platform/linux-generic/odp_packet_io.c > @@ -416,31 +416,24 @@ int pktin_enqueue(queue_entry_t *qentry, odp_buffer_hdr_t *buf_hdr) > odp_buffer_hdr_t *pktin_dequeue(queue_entry_t *qentry) > { > odp_buffer_hdr_t *buf_hdr; > + odp_buffer_t buf; > + odp_packet_t pkt_tbl[QUEUE_MULTI_MAX]; > + odp_buffer_hdr_t *tmp_hdr_tbl[QUEUE_MULTI_MAX]; > + int pkts, i; > > buf_hdr = queue_deq(qentry); > + if (buf_hdr != NULL) > + return buf_hdr; > > - if (buf_hdr == NULL) { > - odp_packet_t pkt; > - odp_buffer_t buf; > - odp_packet_t pkt_tbl[QUEUE_MULTI_MAX]; > - odp_buffer_hdr_t *tmp_hdr_tbl[QUEUE_MULTI_MAX]; > - int pkts, i, j; > - > - pkts = odp_pktio_recv(qentry->s.pktin, pkt_tbl, > - QUEUE_MULTI_MAX); > - > - if (pkts > 0) { > - pkt = pkt_tbl[0]; > - buf = odp_packet_to_buffer(pkt); > - buf_hdr = odp_buf_to_hdr(buf); > + pkts = odp_pktio_recv(qentry->s.pktin, pkt_tbl, QUEUE_MULTI_MAX); > + if (pkts <= 0) > + return buf_hdr; Might as well return NULL, it makes things clearer. > > - for (i = 1, j = 0; i < pkts; ++i) { > - buf = odp_packet_to_buffer(pkt_tbl[i]); > - tmp_hdr_tbl[j++] = odp_buf_to_hdr(buf); > - } > - queue_enq_multi(qentry, tmp_hdr_tbl, j); > - } > + for (i = 0; i < pkts; ++i) { > + buf = odp_packet_to_buffer(pkt_tbl[i]); > + tmp_hdr_tbl[i] = odp_buf_to_hdr(buf); > } > + queue_enq_multi(qentry, tmp_hdr_tbl, i); You need to set buf_hdr to the first entry in pkt_tbl before returning, on this path it remains NULL. > > return buf_hdr; > } > @@ -454,25 +447,24 @@ int pktin_enq_multi(queue_entry_t *qentry, odp_buffer_hdr_t *buf_hdr[], int num) > int pktin_deq_multi(queue_entry_t *qentry, odp_buffer_hdr_t *buf_hdr[], int num) > { > int nbr; > + odp_packet_t pkt_tbl[QUEUE_MULTI_MAX]; > + odp_buffer_hdr_t *tmp_hdr_tbl[QUEUE_MULTI_MAX]; > + odp_buffer_t buf; > + int pkts, i; > > nbr = queue_deq_multi(qentry, buf_hdr, num); > + if (nbr >= num) The geq comparison here is scary, queue_deq_multi should return at most num buffer. Perhaps an assert should be added? > + return nbr; > > - if (nbr < num) { > - odp_packet_t pkt_tbl[QUEUE_MULTI_MAX]; > - odp_buffer_hdr_t *tmp_hdr_tbl[QUEUE_MULTI_MAX]; > - odp_buffer_t buf; > - int pkts, i; > - > - pkts = odp_pktio_recv(qentry->s.pktin, pkt_tbl, > - QUEUE_MULTI_MAX); > - if (pkts > 0) { > - for (i = 0; i < pkts; ++i) { > - buf = odp_packet_to_buffer(pkt_tbl[i]); > - tmp_hdr_tbl[i] = odp_buf_to_hdr(buf); > - } > - queue_enq_multi(qentry, tmp_hdr_tbl, pkts); > - } > + pkts = odp_pktio_recv(qentry->s.pktin, pkt_tbl, QUEUE_MULTI_MAX); > + if (pkts <= 0) > + return nbr; > + > + for (i = 0; i < pkts; ++i) { > + buf = odp_packet_to_buffer(pkt_tbl[i]); > + tmp_hdr_tbl[i] = odp_buf_to_hdr(buf); > } > + queue_enq_multi(qentry, tmp_hdr_tbl, pkts); > > return nbr; > } > -- > 1.8.5.1.163.gd7aced9 > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp
On 11/17/2014 05:07 PM, Ciprian Barbu wrote: >> nbr = queue_deq_multi(qentry, buf_hdr, num); >> >+ if (nbr >= num) > The geq comparison here is scary, queue_deq_multi should return at > most num buffer. Perhaps an assert should be added? > Ciprian, thanks for review. I just kept original code logic and looks like the was bug. I.e. nbr == num is valid case. I.e. queue return as many packets as requested. That can be reproducible with ping -f. Will send new version with all comments updated. Maxim.
diff --git a/platform/linux-generic/odp_packet_io.c b/platform/linux-generic/odp_packet_io.c index f35193f..d4af13e 100644 --- a/platform/linux-generic/odp_packet_io.c +++ b/platform/linux-generic/odp_packet_io.c @@ -416,31 +416,24 @@ int pktin_enqueue(queue_entry_t *qentry, odp_buffer_hdr_t *buf_hdr) odp_buffer_hdr_t *pktin_dequeue(queue_entry_t *qentry) { odp_buffer_hdr_t *buf_hdr; + odp_buffer_t buf; + odp_packet_t pkt_tbl[QUEUE_MULTI_MAX]; + odp_buffer_hdr_t *tmp_hdr_tbl[QUEUE_MULTI_MAX]; + int pkts, i; buf_hdr = queue_deq(qentry); + if (buf_hdr != NULL) + return buf_hdr; - if (buf_hdr == NULL) { - odp_packet_t pkt; - odp_buffer_t buf; - odp_packet_t pkt_tbl[QUEUE_MULTI_MAX]; - odp_buffer_hdr_t *tmp_hdr_tbl[QUEUE_MULTI_MAX]; - int pkts, i, j; - - pkts = odp_pktio_recv(qentry->s.pktin, pkt_tbl, - QUEUE_MULTI_MAX); - - if (pkts > 0) { - pkt = pkt_tbl[0]; - buf = odp_packet_to_buffer(pkt); - buf_hdr = odp_buf_to_hdr(buf); + pkts = odp_pktio_recv(qentry->s.pktin, pkt_tbl, QUEUE_MULTI_MAX); + if (pkts <= 0) + return buf_hdr; - for (i = 1, j = 0; i < pkts; ++i) { - buf = odp_packet_to_buffer(pkt_tbl[i]); - tmp_hdr_tbl[j++] = odp_buf_to_hdr(buf); - } - queue_enq_multi(qentry, tmp_hdr_tbl, j); - } + for (i = 0; i < pkts; ++i) { + buf = odp_packet_to_buffer(pkt_tbl[i]); + tmp_hdr_tbl[i] = odp_buf_to_hdr(buf); } + queue_enq_multi(qentry, tmp_hdr_tbl, i); return buf_hdr; } @@ -454,25 +447,24 @@ int pktin_enq_multi(queue_entry_t *qentry, odp_buffer_hdr_t *buf_hdr[], int num) int pktin_deq_multi(queue_entry_t *qentry, odp_buffer_hdr_t *buf_hdr[], int num) { int nbr; + odp_packet_t pkt_tbl[QUEUE_MULTI_MAX]; + odp_buffer_hdr_t *tmp_hdr_tbl[QUEUE_MULTI_MAX]; + odp_buffer_t buf; + int pkts, i; nbr = queue_deq_multi(qentry, buf_hdr, num); + if (nbr >= num) + return nbr; - if (nbr < num) { - odp_packet_t pkt_tbl[QUEUE_MULTI_MAX]; - odp_buffer_hdr_t *tmp_hdr_tbl[QUEUE_MULTI_MAX]; - odp_buffer_t buf; - int pkts, i; - - pkts = odp_pktio_recv(qentry->s.pktin, pkt_tbl, - QUEUE_MULTI_MAX); - if (pkts > 0) { - for (i = 0; i < pkts; ++i) { - buf = odp_packet_to_buffer(pkt_tbl[i]); - tmp_hdr_tbl[i] = odp_buf_to_hdr(buf); - } - queue_enq_multi(qentry, tmp_hdr_tbl, pkts); - } + pkts = odp_pktio_recv(qentry->s.pktin, pkt_tbl, QUEUE_MULTI_MAX); + if (pkts <= 0) + return nbr; + + for (i = 0; i < pkts; ++i) { + buf = odp_packet_to_buffer(pkt_tbl[i]); + tmp_hdr_tbl[i] = odp_buf_to_hdr(buf); } + queue_enq_multi(qentry, tmp_hdr_tbl, pkts); return nbr; }
Keep original logic but make function more clear. Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> --- platform/linux-generic/odp_packet_io.c | 62 +++++++++++++++------------------- 1 file changed, 27 insertions(+), 35 deletions(-)