Possible bug in dpdk_virtio_from_vm_rx

[PerSundstr8m]

Hi,

Looking at the function 'dpdk_virtio_from_vm_rx' there is an obvious bug that may cause the code to look for completed packages up to 'max_pkts' ahead of the 'avail_pkts' index.
Obviously this seem to work [in most cases], but it is still a bug.

I do not really know how to contribute this information, so I opened up this "issue".

I assume that the comment
" /* Unsigned subtraction gives the right result even with wrap around. */"
is from a time when the ring size was fixed at 256 and the type used for 'avail_pkts' was a 'char'.

Regards,
/Per
(Excuse the indentation, I guess this is not the proper place for code..)

static int
dpdk_virtio_from_vm_rx(void *port, struct rte_mbuf **pkts, uint32_t max_pkts)
{
struct dpdk_virtio_reader *p = (struct dpdk_virtio_reader *)port;
vr_dpdk_virtioq_t *vq = p->rx_virtioq;

/* Unsigned subtraction gives the right result even with wrap around. */
avail_pkts = vq_hard_avail_idx - vq->vdv_last_used_idx;
avail_pkts = RTE_MIN(avail_pkts, max_pkts);
if (unlikely(avail_pkts == 0)) {
DPDK_UDEBUG(VROUTER, &vq->vdv_hash, "%s: queue %p has no packets\n",
__func__, vq);
return 0;
}

For unsigned arithmetic to work with wrap around, the wrap around has to happen at the size of the unsigned variable.
That is not the case here.

When dealing with smaller numbers you need to "simulate" a shorter unsigned int by AND-ing away the unwanted bits.

So here a line:
avail_pkts = avail_pkts & (vq->vdv_size - 1);

is missing.

This small code example illustrates the problem:
(In the example, I have assumed that 'max_pkts' is the ring size.
This is not the case in the original code, but it is not important for
illustrating the problem.)

#include <stdio.h>
#include <stdlib.h>
#include <stdint.h>#define RTE_MIN(a,b) __extension__ ({ \
typeof (a) _a = (a); \
typeof (b) _b = (b); \
_a < _b ? _a : _b; \
})

main(int argc, char** argv)
{
uint16_t vq_hard_avail_idx = (argc > 1) ? atoi(argv[1]) : 0;
uint16_t vdv_last_used_idx = (argc > 2) ? atoi(argv[2]) : 0;
uint16_t max_pkts = (argc > 3) ? atoi(argv[3]) : 0;
void* fix = getenv("FIX");

uint16_t avail_pkts = vq_hard_avail_idx - vdv_last_used_idx;

if (fix) {
avail_pkts = avail_pkts & (max_pkts-1);
}

printf("%d - %d = %d\n", vq_hard_avail_idx, vdv_last_used_idx, avail_pkts);
printf("RTE_MIN(%d,%d) = ",avail_pkts,max_pkts);
avail_pkts = RTE_MIN(avail_pkts, max_pkts); printf("%d\n", avail_pkts);
}

# ./test 5 255 256
5 - 255 = 65286
RTE_MIN(65286,256) = 256 <==== INCORRECT
#
# FIX=1 ./x 5 255 256
5 - 255 = 6
RTE_MIN(6,256) = 6 <==== CORRECT

[kirankn80]

Hi there,

Thanks for the reporting and analyzing the issue in detail. We have raised a bug here - https://contrail-jws.atlassian.net/browse/CEM-12874 and will be address this soon.

Regards,
Kiran