Hi Flavio,
Thanks for your comments.
SNIPPED
> pkts[i] = virtio_dev_pktmbuf_alloc(dev, mbuf_pool,
> buf_len);
> - if (unlikely(pkts[i] == NULL))
> + if (unlikely(pkts[i] == NULL)) {
> + /*
> + * mbuf allocation fails for jumbo packets when
> external
> + * buffer allocation is not allowed and linear
> buffer
> + * is required. Drop this packet.
> + */
> +#ifdef RTE_LIBRTE_VHOST_DEBUG
> + VHOST_LOG_DATA(ERR,
> + "Failed to allocate memory for
> mbuf. Packet dropped!\n"); #endif
That message is useful to spot that missing packets that happens once in a
while, so we should be able to see it even in production without debug enabled.
However, we can't let it flood the log.
Agreed, but VHOST_LOG wrapper does not have rate limit functionality.
I am not sure if librte eal has this functionality, but if not you could limit
by using a static bool:
static bool allocerr_warned = false;
if (allocerr_warned) {
VHOST_LOG_DATA(ERR,
"Failed to allocate memory for mbuf. Packet dropped!\n");
allocerr_warned = true;
}
This is good idea, but having a static variable makes it file scope making it
to entire VHOST devices. Hence if the intention is to implement device specific
log rate limit, should not we resort to `dev->allocerr_warn` counter mechanism,
which resets after n failures `#define LOG_ALLOCFAIL 32`.
SNIPPED
> static __rte_always_inline int
> @@ -1946,21 +1967,24 @@ virtio_dev_tx_single_packed(struct virtio_net *dev,
> struct rte_mbuf **pkts)
> {
>
> - uint16_t buf_id, desc_count;
> + uint16_t buf_id, desc_count = 0;
> + int ret;
>
> - if (vhost_dequeue_single_packed(dev, vq, mbuf_pool, pkts, &buf_id,
> - &desc_count))
> - return -1;
> + ret = vhost_dequeue_single_packed(dev, vq, mbuf_pool, pkts, &buf_id,
> + &desc_count);
>
> - if (virtio_net_is_inorder(dev))
> - vhost_shadow_dequeue_single_packed_inorder(vq, buf_id,
> -
> desc_count);
> - else
> - vhost_shadow_dequeue_single_packed(vq, buf_id,
> desc_count);
> + if (likely(desc_count > 0)) {
The vhost_dequeue_single_packed() could return -1 with desc_count > 0 and this
change doesn't handle that.
Yes, as per my current understanding in either success or failure we need to
flush the descriptors `desc_count` to handle this issue.
Is there an expectation for partial or incomplete packet where `num_desc` is
greater than 0, we need to preserve it.
SNIPPED
Thanks & Regards,
Sivaprasad
