Hi,
as far as I can see the patch introduces regressions.
CC Kevin and Luca to be careful with stable branches patches.
See details below.
Andrew.
On 9/23/19 5:05 PM, Marvin Liu wrote:
If reserve virtio header room by function rte_pktmbuf_prepend, both
segment data length and packet length of mbuf will be increased.
Data length will be equal to descriptor length, while packet length
should be decreased as virtio-net header won't be taken into packet.
Thus will cause mismatch in mbuf structure. Fix this issue by access
mbuf data directly and increase descriptor length if it is needed.
Fixes: 58169a9c8153 ("net/virtio: support Tx checksum offload")
Fixes: 892dc798fa9c ("net/virtio: implement Tx path for packed queues")
Fixes: 4905ed3a523f ("net/virtio: optimize Tx enqueue for packed ring")
Fixes: e5f456a98d3c ("net/virtio: support in-order Rx and Tx")
Cc: sta...@dpdk.org
Reported-by: Stephen Hemminger <step...@networkplumber.org>
Signed-off-by: Marvin Liu <yong....@intel.com>
diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index 27ead19fb..822cce06d 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -597,9 +597,8 @@ virtqueue_enqueue_xmit_inorder(struct virtnet_tx *txvq,
dxp->cookie = (void *)cookies[i];
dxp->ndescs = 1;
- hdr = (struct virtio_net_hdr *)
- rte_pktmbuf_prepend(cookies[i], head_size);
- cookies[i]->pkt_len -= head_size;
+ hdr = (struct virtio_net_hdr *)(char *)cookies[i]->buf_addr +
+ cookies[i]->data_off - head_size;
/* if offload disabled, hdr is not zeroed yet, do it now */
if (!vq->hw->has_tx_offload)
@@ -608,9 +607,10 @@ virtqueue_enqueue_xmit_inorder(struct virtnet_tx *txvq,
virtqueue_xmit_offload(hdr, cookies[i], true);
start_dp[idx].addr = VIRTIO_MBUF_DATA_DMA_ADDR(cookies[i], vq);
As I understand the problem is here. It points to start of the packet
(Ethernet header) since data_off is not changed above now, but
should point to virtio_net_hdr before the packet.
I think the patch fixes the bug in a wrong direction. It looks better
to simply remove
cookies[i]->pkt_len -= head_size;
above and care about real packet length difference in
virtio_update_packet_stats() or when it is called from Tx path.
If it is OK for maintainers I'm ready to send patches to rollback back
this one and fix it as described above.
- start_dp[idx].len = cookies[i]->data_len;
+ start_dp[idx].len = cookies[i]->data_len + head_size;
start_dp[idx].flags = 0;
+
vq_update_avail_ring(vq, idx);
idx++;
@@ -644,9 +644,8 @@ virtqueue_enqueue_xmit_packed_fast(struct virtnet_tx *txvq,
flags = vq->vq_packed.cached_flags;
/* prepend cannot fail, checked by caller */
- hdr = (struct virtio_net_hdr *)
- rte_pktmbuf_prepend(cookie, head_size);
- cookie->pkt_len -= head_size;
+ hdr = (struct virtio_net_hdr *)(char *)cookie->buf_addr +
+ cookie->data_off - head_size;
/* if offload disabled, hdr is not zeroed yet, do it now */
if (!vq->hw->has_tx_offload)
@@ -655,7 +654,7 @@ virtqueue_enqueue_xmit_packed_fast(struct virtnet_tx *txvq,
virtqueue_xmit_offload(hdr, cookie, true);
dp->addr = VIRTIO_MBUF_DATA_DMA_ADDR(cookie, vq);
- dp->len = cookie->data_len;
+ dp->len = cookie->data_len + head_size;
dp->id = id;
if (++vq->vq_avail_idx >= vq->vq_nentries) {
@@ -687,6 +686,7 @@ virtqueue_enqueue_xmit_packed(struct virtnet_tx *txvq,
struct rte_mbuf *cookie,
uint16_t head_size = vq->hw->vtnet_hdr_size;
struct virtio_net_hdr *hdr;
uint16_t prev;
+ bool prepend_header = false;
id = in_order ? vq->vq_avail_idx : vq->vq_desc_head_idx;
@@ -705,12 +705,9 @@ virtqueue_enqueue_xmit_packed(struct virtnet_tx *txvq, struct rte_mbuf *cookie,
if (can_push) {
/* prepend cannot fail, checked by caller */
- hdr = (struct virtio_net_hdr *)
- rte_pktmbuf_prepend(cookie, head_size);
- /* rte_pktmbuf_prepend() counts the hdr size to the pkt length,
- * which is wrong. Below subtract restores correct pkt size.
- */
- cookie->pkt_len -= head_size;
+ hdr = (struct virtio_net_hdr *)(char *)cookie->buf_addr +
+ cookie->data_off - head_size;
+ prepend_header = true;
/* if offload disabled, it is not zeroed below, do it now */
if (!vq->hw->has_tx_offload)
@@ -738,6 +735,11 @@ virtqueue_enqueue_xmit_packed(struct virtnet_tx *txvq,
struct rte_mbuf *cookie,
start_dp[idx].addr = VIRTIO_MBUF_DATA_DMA_ADDR(cookie, vq);
start_dp[idx].len = cookie->data_len;
+ if (prepend_header) {
+ start_dp[idx].len += head_size;
+ prepend_header = false;
+ }
+
if (likely(idx != head_idx)) {
flags = cookie->next ? VRING_DESC_F_NEXT : 0;
flags |= vq->vq_packed.cached_flags;
@@ -779,6 +781,7 @@ virtqueue_enqueue_xmit(struct virtnet_tx *txvq, struct
rte_mbuf *cookie,
uint16_t seg_num = cookie->nb_segs;
uint16_t head_idx, idx;
uint16_t head_size = vq->hw->vtnet_hdr_size;
+ bool prepend_header = false;
struct virtio_net_hdr *hdr;
head_idx = vq->vq_desc_head_idx;
@@ -794,12 +797,9 @@ virtqueue_enqueue_xmit(struct virtnet_tx *txvq, struct
rte_mbuf *cookie,
if (can_push) {
/* prepend cannot fail, checked by caller */
- hdr = (struct virtio_net_hdr *)
- rte_pktmbuf_prepend(cookie, head_size);
- /* rte_pktmbuf_prepend() counts the hdr size to the pkt length,
- * which is wrong. Below subtract restores correct pkt size.
- */
- cookie->pkt_len -= head_size;
+ hdr = (struct virtio_net_hdr *)(char *)cookie->buf_addr +
+ cookie->data_off - head_size;
+ prepend_header = true;
/* if offload disabled, it is not zeroed below, do it now */
if (!vq->hw->has_tx_offload)
@@ -838,6 +838,10 @@ virtqueue_enqueue_xmit(struct virtnet_tx *txvq, struct
rte_mbuf *cookie,
do {
start_dp[idx].addr = VIRTIO_MBUF_DATA_DMA_ADDR(cookie, vq);
start_dp[idx].len = cookie->data_len;
+ if (prepend_header) {
+ start_dp[idx].len += head_size;
+ prepend_header = false;
+ }
start_dp[idx].flags = cookie->next ? VRING_DESC_F_NEXT : 0;
idx = start_dp[idx].next;
} while ((cookie = cookie->next) != NULL);
