On 22 Jun 2018, at 21:05, Lam, Tiago wrote:
> On 18/06/2018 14:15, Eelco Chaudron wrote: >> >> >> On 11 Jun 2018, at 18:21, Tiago Lam wrote: >> >>> From: Mark Kavanagh <[email protected]> >>> >>> Currently, packets are only copied to a single segment in the function >>> dpdk_do_tx_copy(). This could be an issue in the case of jumbo frames, >>> particularly when multi-segment mbufs are involved. >>> >>> This patch calculates the number of segments needed by a packet and >>> copies the data to each segment. >>> >>> A new function, dpdk_buf_alloc(), has also been introduced as a >>> wrapper >>> around the nonpmd_mp_mutex to serialise allocations from a non-pmd >>> context. >>> >>> Co-authored-by: Michael Qiu <[email protected]> >>> Co-authored-by: Tiago Lam <[email protected]> >>> >>> Signed-off-by: Mark Kavanagh <[email protected]> >>> Signed-off-by: Michael Qiu <[email protected]> >>> Signed-off-by: Tiago Lam <[email protected]> >>> --- >>> lib/netdev-dpdk.c | 94 >>> +++++++++++++++++++++++++++++++++++++++++++++++++------ >>> 1 file changed, 84 insertions(+), 10 deletions(-) >>> >>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >>> index 9b1fb9a..0079e28 100644 >>> --- a/lib/netdev-dpdk.c >>> +++ b/lib/netdev-dpdk.c >>> @@ -515,6 +515,22 @@ dpdk_rte_mzalloc(size_t sz) >>> return rte_zmalloc(OVS_VPORT_DPDK, sz, OVS_CACHE_LINE_SIZE); >>> } >>> >>> +static struct rte_mbuf * >>> +dpdk_buf_alloc(struct rte_mempool *mp) >>> +{ >>> + if (!dpdk_thread_is_pmd()) { >>> + ovs_mutex_lock(&nonpmd_mp_mutex); >> Can you explain why the lock is needed here for non PMD threads? >>> + } >>> + >>> + struct rte_mbuf *mbuf = rte_pktmbuf_alloc(mp); >>> + >>> + if (!dpdk_thread_is_pmd()) { >>> + ovs_mutex_unlock(&nonpmd_mp_mutex); >>> + } >>> + >>> + return mbuf; >>> +} >>> + >>> void >>> free_dpdk_buf(struct dp_packet *p) >>> { >>> @@ -2167,6 +2183,71 @@ out: >>> } >>> } >>> >>> +static int >> >> a bool, true for success might be more usefull here. > > I've updated this part to return better error codes. More specifically, > `ENOMEM` so the caller knows there's no memory. > Even better… >> >>> +dpdk_prep_tx_buf(struct dp_packet *packet, struct rte_mbuf **head, >>> + struct rte_mempool *mp) >> >> Maybe the function name should be more what it's doing, i.e >> dpdk_clone_dp_packet_to_mbuf(). >> > > Sounds clearer to me, aside from the clone since it's different types of > packets. What about `dpdk_copy_dp_packet_to_mbuf()`? > Sounds good to me. >>> +{ >>> + struct rte_mbuf *temp; >>> + uint32_t size = dp_packet_size(packet); >>> + uint16_t max_data_len, data_len; >>> + uint32_t nb_segs = 0; >>> + int i; >>> + >>> + temp = *head = dpdk_buf_alloc(mp); >>> + if (OVS_UNLIKELY(!temp)) { >>> + return 1; >>> + } >>> + >>> + /* All new allocated mbuf's max data len is the same */ >>> + max_data_len = temp->buf_len - temp->data_off; >>> + >>> + /* Calculate # of output mbufs. */ >>> + nb_segs = size / max_data_len; >>> + if (size % max_data_len) { >>> + nb_segs = nb_segs + 1; >>> + } >>> + >>> + /* Allocate additional mbufs when multiple output mbufs required. >>> */ >>> + for (i = 1; i < nb_segs; i++) { >>> + temp->next = dpdk_buf_alloc(mp); >>> + if (!temp->next) { >>> + free_dpdk_buf((struct dp_packet *) *head); >>> + *head = NULL; >>> + break; >>> + } >>> + temp = temp->next; >>> + } >>> + /* We have to do a copy for now */ >>> + rte_pktmbuf_pkt_len(*head) = size; >>> + temp = *head; >>> + >>> + data_len = size < max_data_len ? size: max_data_len; >> >> Can we use max_data_len to copy? It's only valid if rte_pktmbuf_mtod() >> returns the first byte, but after alloc it's >> rte_pktmbuf_reset_headroom(). So we will overwrite invalid memory. >> >>> + if (packet->source == DPBUF_DPDK) { >>> + *head = &(packet->mbuf); >>> + while (temp && head && size > 0) { >>> + rte_memcpy(rte_pktmbuf_mtod(temp, void *), >>> + dp_packet_data((struct dp_packet *)head), >>> data_len); >> >> Here you assume source and destination mbuf sizes are the same... >> Also use container_of for "(struct dp_packet *)head)" >> >>> + rte_pktmbuf_data_len(temp) = data_len; >>> + *head = (*head)->next; >>> + size = size - data_len; >>> + data_len = size < max_data_len ? size: max_data_len; >>> + temp = temp->next; >>> + } >>> + } else { >> >> Why not use dp_packet_mbuf_write() here? >> > > I missed this. I'll take this approach as well, thanks. I assume you also mean the other two inlines above this one. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
