| Message ID | 1477548924-26376-6-git-send-email-qiudayu@chinac.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Daniele Di Proietto |
| Headers | show |
> >Currently, one packet is only copied to one segment >in function dpdk_do_tx_copy(), this could be an issue >when a jumboframe comes, especially for multipile segments. Typos - 'jumboframe', 'multipile' This patch doesn't apply cleanly - change 'mbufs' to 'pkts' as previously described to address this. I haven't tested this patchset yet - will try to get to it next week. Thanks again, Mark > >This patch calculate the segment number needed by the packet and >copy the data to different segments. > >Signed-off-by: Michael Qiu <qiudayu@chinac.com> >Signed-off-by: Jijiang Liu <liujijiang@chinac.com> >--- > lib/netdev-dpdk.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++------- > 1 file changed, 48 insertions(+), 7 deletions(-) > >diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >index ad92504..abb3b53 100644 >--- a/lib/netdev-dpdk.c >+++ b/lib/netdev-dpdk.c >@@ -1554,9 +1554,10 @@ dpdk_do_tx_copy(struct netdev *netdev, int qid, struct dp_packet_batch >*batch) > #endif > struct netdev_dpdk *dev = netdev_dpdk_cast(netdev); > struct rte_mbuf *mbufs[PKT_ARRAY_SIZE]; >+ struct rte_mbuf *temp, *head = NULL; > int dropped = 0; > int newcnt = 0; >- int i; >+ int i, j, nb_segs; > > /* If we are on a non pmd thread we have to use the mempool mutex, because > * every non pmd thread shares the same mempool cache */ >@@ -1569,6 +1570,7 @@ dpdk_do_tx_copy(struct netdev *netdev, int qid, struct dp_packet_batch >*batch) > > for (i = 0; i < batch->count; i++) { > int size = dp_packet_size(batch->packets[i]); >+ int max_data_len, tmp_len; > > if (OVS_UNLIKELY(size > dev->max_packet_len)) { > VLOG_WARN_RL(&rl, "Too big size %d max_packet_len %d", >@@ -1578,7 +1580,24 @@ dpdk_do_tx_copy(struct netdev *netdev, int qid, struct dp_packet_batch >*batch) > continue; > } > >- mbufs[newcnt] = rte_pktmbuf_alloc(dev->dpdk_mp->mp); >+ temp = mbufs[newcnt] = rte_pktmbuf_alloc(dev->dpdk_mp->mp); >+ >+ /* all new allocated mbuf's max data len is the same */ >+ max_data_len = temp->buf_len - temp->data_off; >+ >+ nb_segs = size/max_data_len; >+ if (size % max_data_len) >+ nb_segs = nb_segs + 1; >+ >+ for (j = 1; j < nb_segs; j++) { >+ temp->next = rte_pktmbuf_alloc(dev->dpdk_mp->mp); >+ if (!temp->next) { >+ rte_pktmbuf_free(mbufs[newcnt]); >+ mbufs[newcnt] = NULL; >+ break; >+ } >+ temp = temp->next; >+ } Need to terminate the mbuf chain here: + temp->next = NULL; > > if (!mbufs[newcnt]) { > dropped += batch->count - i; >@@ -1586,15 +1605,37 @@ dpdk_do_tx_copy(struct netdev *netdev, int qid, struct >dp_packet_batch *batch) > } > > /* We have to do a copy for now */ >- memcpy(rte_pktmbuf_mtod(mbufs[newcnt], void *), >- dp_packet_data(batch->packets[i]), size); >+ rte_pktmbuf_pkt_len(mbufs[newcnt]) = size; >+ temp = mbufs[newcnt]; >+ tmp_len = size < max_data_len ? size: max_data_len; >+ if (batch->packets[i]->source == DPBUF_DPDK) { >+ head = &(batch->packets[i]->mbuf); >+ while (temp && head && size > 0) { >+ rte_memcpy(rte_pktmbuf_mtod(temp, void*), dp_packet_data((struct dp_packet >*)head),tmp_len); >+ rte_pktmbuf_data_len(temp) = tmp_len; >+ head = head->next; >+ size = size - tmp_len; >+ tmp_len = size < max_data_len ? size: max_data_len; >+ temp = temp->next; >+ } >+ } else { This section could use a comment or two. >+ int offset = 0; >+ while (temp && size > 0) { >+ memcpy(rte_pktmbuf_mtod(temp, void *), >+ dp_packet_at(batch->packets[i], offset,tmp_len), tmp_len); >+ rte_pktmbuf_data_len(temp) = tmp_len; >+ temp = temp->next; >+ size = size - tmp_len; >+ offset +=tmp_len; >+ tmp_len = size < max_data_len ? size: max_data_len; >+ } >+ } >+ > >- mbufs[newcnt]->nb_segs = batch->packets[i]->mbuf.nb_segs; >+ mbufs[newcnt]->nb_segs = nb_segs; > mbufs[newcnt]->ol_flags = batch->packets[i]->mbuf.ol_flags; > mbufs[newcnt]->packet_type = batch->packets[i]->mbuf.packet_type; > mbufs[newcnt]->tx_offload = batch->packets[i]->mbuf.tx_offload; >- rte_pktmbuf_data_len(mbufs[newcnt]) = size; >- rte_pktmbuf_pkt_len(mbufs[newcnt]) = size; > > newcnt++; > } >-- >1.8.3.1
2016/10/28 19:17, Kavanagh, Mark B : >> Currently, one packet is only copied to one segment >> in function dpdk_do_tx_copy(), this could be an issue >> when a jumboframe comes, especially for multipile segments. > Typos - 'jumboframe', 'multipile' > > This patch doesn't apply cleanly - change 'mbufs' to 'pkts' as previously described to address this. I will fix the issues, and rebase to the latest code. > > I haven't tested this patchset yet - will try to get to it next week. OK, thanks. > Thanks again, > Mark > >> This patch calculate the segment number needed by the packet and >> copy the data to different segments. >> >> Signed-off-by: Michael Qiu <qiudayu@chinac.com> >> Signed-off-by: Jijiang Liu <liujijiang@chinac.com> >> --- >> lib/netdev-dpdk.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++------- >> 1 file changed, 48 insertions(+), 7 deletions(-) >> >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >> index ad92504..abb3b53 100644 >> --- a/lib/netdev-dpdk.c >> +++ b/lib/netdev-dpdk.c >> @@ -1554,9 +1554,10 @@ dpdk_do_tx_copy(struct netdev *netdev, int qid, struct dp_packet_batch >> *batch) >> #endif >> struct netdev_dpdk *dev = netdev_dpdk_cast(netdev); >> struct rte_mbuf *mbufs[PKT_ARRAY_SIZE]; >> + struct rte_mbuf *temp, *head = NULL; >> int dropped = 0; >> int newcnt = 0; >> - int i; >> + int i, j, nb_segs; >> >> /* If we are on a non pmd thread we have to use the mempool mutex, because >> * every non pmd thread shares the same mempool cache */ >> @@ -1569,6 +1570,7 @@ dpdk_do_tx_copy(struct netdev *netdev, int qid, struct dp_packet_batch >> *batch) >> >> for (i = 0; i < batch->count; i++) { >> int size = dp_packet_size(batch->packets[i]); >> + int max_data_len, tmp_len; >> >> if (OVS_UNLIKELY(size > dev->max_packet_len)) { >> VLOG_WARN_RL(&rl, "Too big size %d max_packet_len %d", >> @@ -1578,7 +1580,24 @@ dpdk_do_tx_copy(struct netdev *netdev, int qid, struct dp_packet_batch >> *batch) >> continue; >> } >> >> - mbufs[newcnt] = rte_pktmbuf_alloc(dev->dpdk_mp->mp); >> + temp = mbufs[newcnt] = rte_pktmbuf_alloc(dev->dpdk_mp->mp); >> + >> + /* all new allocated mbuf's max data len is the same */ >> + max_data_len = temp->buf_len - temp->data_off; >> + >> + nb_segs = size/max_data_len; >> + if (size % max_data_len) >> + nb_segs = nb_segs + 1; >> + >> + for (j = 1; j < nb_segs; j++) { >> + temp->next = rte_pktmbuf_alloc(dev->dpdk_mp->mp); >> + if (!temp->next) { >> + rte_pktmbuf_free(mbufs[newcnt]); >> + mbufs[newcnt] = NULL; >> + break; >> + } >> + temp = temp->next; >> + } > Need to terminate the mbuf chain here: > + temp->next = NULL; No need, all mbufs' next field have been set to NULL when allocated. > >> if (!mbufs[newcnt]) { >> dropped += batch->count - i; >> @@ -1586,15 +1605,37 @@ dpdk_do_tx_copy(struct netdev *netdev, int qid, struct >> dp_packet_batch *batch) >> } >> >> /* We have to do a copy for now */ >> - memcpy(rte_pktmbuf_mtod(mbufs[newcnt], void *), >> - dp_packet_data(batch->packets[i]), size); >> + rte_pktmbuf_pkt_len(mbufs[newcnt]) = size; >> + temp = mbufs[newcnt]; >> + tmp_len = size < max_data_len ? size: max_data_len; >> + if (batch->packets[i]->source == DPBUF_DPDK) { >> + head = &(batch->packets[i]->mbuf); >> + while (temp && head && size > 0) { >> + rte_memcpy(rte_pktmbuf_mtod(temp, void*), dp_packet_data((struct dp_packet >> *)head),tmp_len); >> + rte_pktmbuf_data_len(temp) = tmp_len; >> + head = head->next; >> + size = size - tmp_len; >> + tmp_len = size < max_data_len ? size: max_data_len; >> + temp = temp->next; >> + } >> + } else { > This section could use a comment or two. OK, I will add a comment. > >> + int offset = 0; >> + while (temp && size > 0) { >> + memcpy(rte_pktmbuf_mtod(temp, void *), >> + dp_packet_at(batch->packets[i], offset,tmp_len), tmp_len); >> + rte_pktmbuf_data_len(temp) = tmp_len; >> + temp = temp->next; >> + size = size - tmp_len; >> + offset +=tmp_len; >> + tmp_len = size < max_data_len ? size: max_data_len; >> + } >> + } >> + >> >> - mbufs[newcnt]->nb_segs = batch->packets[i]->mbuf.nb_segs; >> + mbufs[newcnt]->nb_segs = nb_segs; >> mbufs[newcnt]->ol_flags = batch->packets[i]->mbuf.ol_flags; >> mbufs[newcnt]->packet_type = batch->packets[i]->mbuf.packet_type; >> mbufs[newcnt]->tx_offload = batch->packets[i]->mbuf.tx_offload; >> - rte_pktmbuf_data_len(mbufs[newcnt]) = size; >> - rte_pktmbuf_pkt_len(mbufs[newcnt]) = size; >> >> newcnt++; >> } >> -- >> 1.8.3.1 >
2016/10/28 19:17, Kavanagh, Mark B : >> Currently, one packet is only copied to one segment >> in function dpdk_do_tx_copy(), this could be an issue >> when a jumboframe comes, especially for multipile segments. > Typos - 'jumboframe', 'multipile' > > This patch doesn't apply cleanly - change 'mbufs' to 'pkts' as previously described to address this. I will fix the issues, and rebase to the latest code. > > I haven't tested this patchset yet - will try to get to it next week. OK, thanks. > Thanks again, > Mark > >> This patch calculate the segment number needed by the packet and >> copy the data to different segments. >> >> Signed-off-by: Michael Qiu <qiudayu@chinac.com> >> Signed-off-by: Jijiang Liu <liujijiang@chinac.com> >> --- >> lib/netdev-dpdk.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++------- >> 1 file changed, 48 insertions(+), 7 deletions(-) >> >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >> index ad92504..abb3b53 100644 >> --- a/lib/netdev-dpdk.c >> +++ b/lib/netdev-dpdk.c >> @@ -1554,9 +1554,10 @@ dpdk_do_tx_copy(struct netdev *netdev, int qid, struct dp_packet_batch >> *batch) >> #endif >> struct netdev_dpdk *dev = netdev_dpdk_cast(netdev); >> struct rte_mbuf *mbufs[PKT_ARRAY_SIZE]; >> + struct rte_mbuf *temp, *head = NULL; >> int dropped = 0; >> int newcnt = 0; >> - int i; >> + int i, j, nb_segs; >> >> /* If we are on a non pmd thread we have to use the mempool mutex, because >> * every non pmd thread shares the same mempool cache */ >> @@ -1569,6 +1570,7 @@ dpdk_do_tx_copy(struct netdev *netdev, int qid, struct dp_packet_batch >> *batch) >> >> for (i = 0; i < batch->count; i++) { >> int size = dp_packet_size(batch->packets[i]); >> + int max_data_len, tmp_len; >> >> if (OVS_UNLIKELY(size > dev->max_packet_len)) { >> VLOG_WARN_RL(&rl, "Too big size %d max_packet_len %d", >> @@ -1578,7 +1580,24 @@ dpdk_do_tx_copy(struct netdev *netdev, int qid, struct dp_packet_batch >> *batch) >> continue; >> } >> >> - mbufs[newcnt] = rte_pktmbuf_alloc(dev->dpdk_mp->mp); >> + temp = mbufs[newcnt] = rte_pktmbuf_alloc(dev->dpdk_mp->mp); >> + >> + /* all new allocated mbuf's max data len is the same */ >> + max_data_len = temp->buf_len - temp->data_off; >> + >> + nb_segs = size/max_data_len; >> + if (size % max_data_len) >> + nb_segs = nb_segs + 1; >> + >> + for (j = 1; j < nb_segs; j++) { >> + temp->next = rte_pktmbuf_alloc(dev->dpdk_mp->mp); >> + if (!temp->next) { >> + rte_pktmbuf_free(mbufs[newcnt]); >> + mbufs[newcnt] = NULL; >> + break; >> + } >> + temp = temp->next; >> + } > Need to terminate the mbuf chain here: > + temp->next = NULL; No need, all mbufs' next field have been set to NULL when allocated. > >> if (!mbufs[newcnt]) { >> dropped += batch->count - i; >> @@ -1586,15 +1605,37 @@ dpdk_do_tx_copy(struct netdev *netdev, int qid, struct >> dp_packet_batch *batch) >> } >> >> /* We have to do a copy for now */ >> - memcpy(rte_pktmbuf_mtod(mbufs[newcnt], void *), >> - dp_packet_data(batch->packets[i]), size); >> + rte_pktmbuf_pkt_len(mbufs[newcnt]) = size; >> + temp = mbufs[newcnt]; >> + tmp_len = size < max_data_len ? size: max_data_len; >> + if (batch->packets[i]->source == DPBUF_DPDK) { >> + head = &(batch->packets[i]->mbuf); >> + while (temp && head && size > 0) { >> + rte_memcpy(rte_pktmbuf_mtod(temp, void*), dp_packet_data((struct dp_packet >> *)head),tmp_len); >> + rte_pktmbuf_data_len(temp) = tmp_len; >> + head = head->next; >> + size = size - tmp_len; >> + tmp_len = size < max_data_len ? size: max_data_len; >> + temp = temp->next; >> + } >> + } else { > This section could use a comment or two. OK, I will add a comment. > >> + int offset = 0; >> + while (temp && size > 0) { >> + memcpy(rte_pktmbuf_mtod(temp, void *), >> + dp_packet_at(batch->packets[i], offset,tmp_len), tmp_len); >> + rte_pktmbuf_data_len(temp) = tmp_len; >> + temp = temp->next; >> + size = size - tmp_len; >> + offset +=tmp_len; >> + tmp_len = size < max_data_len ? size: max_data_len; >> + } >> + } >> + >> >> - mbufs[newcnt]->nb_segs = batch->packets[i]->mbuf.nb_segs; >> + mbufs[newcnt]->nb_segs = nb_segs; >> mbufs[newcnt]->ol_flags = batch->packets[i]->mbuf.ol_flags; >> mbufs[newcnt]->packet_type = batch->packets[i]->mbuf.packet_type; >> mbufs[newcnt]->tx_offload = batch->packets[i]->mbuf.tx_offload; >> - rte_pktmbuf_data_len(mbufs[newcnt]) = size; >> - rte_pktmbuf_pkt_len(mbufs[newcnt]) = size; >> >> newcnt++; >> } >> -- >> 1.8.3.1 >
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index ad92504..abb3b53 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -1554,9 +1554,10 @@ dpdk_do_tx_copy(struct netdev *netdev, int qid, struct dp_packet_batch *batch) #endif struct netdev_dpdk *dev = netdev_dpdk_cast(netdev); struct rte_mbuf *mbufs[PKT_ARRAY_SIZE]; + struct rte_mbuf *temp, *head = NULL; int dropped = 0; int newcnt = 0; - int i; + int i, j, nb_segs; /* If we are on a non pmd thread we have to use the mempool mutex, because * every non pmd thread shares the same mempool cache */ @@ -1569,6 +1570,7 @@ dpdk_do_tx_copy(struct netdev *netdev, int qid, struct dp_packet_batch *batch) for (i = 0; i < batch->count; i++) { int size = dp_packet_size(batch->packets[i]); + int max_data_len, tmp_len; if (OVS_UNLIKELY(size > dev->max_packet_len)) { VLOG_WARN_RL(&rl, "Too big size %d max_packet_len %d", @@ -1578,7 +1580,24 @@ dpdk_do_tx_copy(struct netdev *netdev, int qid, struct dp_packet_batch *batch) continue; } - mbufs[newcnt] = rte_pktmbuf_alloc(dev->dpdk_mp->mp); + temp = mbufs[newcnt] = rte_pktmbuf_alloc(dev->dpdk_mp->mp); + + /* all new allocated mbuf's max data len is the same */ + max_data_len = temp->buf_len - temp->data_off; + + nb_segs = size/max_data_len; + if (size % max_data_len) + nb_segs = nb_segs + 1; + + for (j = 1; j < nb_segs; j++) { + temp->next = rte_pktmbuf_alloc(dev->dpdk_mp->mp); + if (!temp->next) { + rte_pktmbuf_free(mbufs[newcnt]); + mbufs[newcnt] = NULL; + break; + } + temp = temp->next; + } if (!mbufs[newcnt]) { dropped += batch->count - i; @@ -1586,15 +1605,37 @@ dpdk_do_tx_copy(struct netdev *netdev, int qid, struct dp_packet_batch *batch) } /* We have to do a copy for now */ - memcpy(rte_pktmbuf_mtod(mbufs[newcnt], void *), - dp_packet_data(batch->packets[i]), size); + rte_pktmbuf_pkt_len(mbufs[newcnt]) = size; + temp = mbufs[newcnt]; + tmp_len = size < max_data_len ? size: max_data_len; + if (batch->packets[i]->source == DPBUF_DPDK) { + head = &(batch->packets[i]->mbuf); + while (temp && head && size > 0) { + rte_memcpy(rte_pktmbuf_mtod(temp, void*), dp_packet_data((struct dp_packet *)head),tmp_len); + rte_pktmbuf_data_len(temp) = tmp_len; + head = head->next; + size = size - tmp_len; + tmp_len = size < max_data_len ? size: max_data_len; + temp = temp->next; + } + } else { + int offset = 0; + while (temp && size > 0) { + memcpy(rte_pktmbuf_mtod(temp, void *), + dp_packet_at(batch->packets[i], offset,tmp_len), tmp_len); + rte_pktmbuf_data_len(temp) = tmp_len; + temp = temp->next; + size = size - tmp_len; + offset +=tmp_len; + tmp_len = size < max_data_len ? size: max_data_len; + } + } + - mbufs[newcnt]->nb_segs = batch->packets[i]->mbuf.nb_segs; + mbufs[newcnt]->nb_segs = nb_segs; mbufs[newcnt]->ol_flags = batch->packets[i]->mbuf.ol_flags; mbufs[newcnt]->packet_type = batch->packets[i]->mbuf.packet_type; mbufs[newcnt]->tx_offload = batch->packets[i]->mbuf.tx_offload; - rte_pktmbuf_data_len(mbufs[newcnt]) = size; - rte_pktmbuf_pkt_len(mbufs[newcnt]) = size; newcnt++; }