On Mon, Nov 17, 2014 at 6:12 PM, Maxim Uvarov <[email protected]> wrote:
> Keep original logic but make function more clear.
>
> Signed-off-by: Maxim Uvarov <[email protected]>
> ---
> v2: fixes for checks that Ciprian found.
>
> Tested this patch in lxc with ping -f for both burst and queue modes.
>
> Maxim.
>
> platform/linux-generic/odp_packet_io.c | 66
> ++++++++++++++++------------------
> 1 file changed, 30 insertions(+), 36 deletions(-)
>
> diff --git a/platform/linux-generic/odp_packet_io.c
> b/platform/linux-generic/odp_packet_io.c
> index f35193f..73de80e 100644
> --- a/platform/linux-generic/odp_packet_io.c
> +++ b/platform/linux-generic/odp_packet_io.c
> @@ -416,32 +416,26 @@ 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 NULL;
>
> - 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);
I don't generally rely on the value of a for counter, even if it's
correct (there are exceptions where you need to though). The peril is
with people that write in C++, or some C11 extension, where you can
hide the counter inside the loop. I.e. for (int i=0; i<max; i++). And
you know you received pkts buffers, I think it's more clear to use
pkts instead.
> + buf_hdr = tmp_hdr_tbl[0];
> return buf_hdr;
> }
>
> @@ -454,25 +448,25 @@ 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)
> + ODP_ABORT("queue_deq_multi req: %d, returned %d\n",
> + num, 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);
> - }
> - }
I think you still need to handle the '==' case separately. If
queue_deq_multi returned exactly num (i.e. nbr == num) it may do that
in two stituations:
1. there were exactly nbr buffers in the queue - in this case you need
to fill it up again
2. there were more than nbr buffers in the queue - in this case you
might want to wait for the next dequeue, to have a guarantee that the
queue will *always* have at most QUEUE_MULTI_MAX. That's how the
original code worked like.
But you don't know which of the 2 cases was true, so the correct thing
is to return, without receiving anything anymore. It's the only way to
make sure you don't have more than QUEUE_MULTI_MAX buffers in queue.
So you just need:
if (nbr == num)
return nbr;
> + 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);
> + buf_hdr = &tmp_hdr_tbl[0];
> return nbr;
> }
> --
> 1.8.5.1.163.gd7aced9
>
>
> _______________________________________________
> lng-odp mailing list
> [email protected]
> http://lists.linaro.org/mailman/listinfo/lng-odp
_______________________________________________
lng-odp mailing list
[email protected]
http://lists.linaro.org/mailman/listinfo/lng-odp
