On Wed, Apr 2, 2014 at 1:00 PM, Jarno Rajahalme <jrajaha...@nicira.com> wrote:
>
> On Mar 31, 2014, at 9:43 PM, Pravin <pshe...@nicira.com> wrote:
>
>> From: Pravin Shelar <pshe...@nicira.com>
>>
>> On DPDK packet recv, ovs is given pointer to mbuf which has
>> information about a packet, for example pointer to data and size.
>> By moving mbuf to ofpbuf we can let dpdk allocate ofpbuf and
>> pass that to ovs for processing the packet.
>>
>> Signed-off-by: Pravin B Shelar <pshe...@nicira.com>
>> ---
>> lib/netdev-dpdk.c | 99
>> +++++++++++++++++++++++++++--------------------------
>> lib/ofpbuf.c | 4 +--
>> lib/ofpbuf.h | 6 +++-
>> 3 files changed, 57 insertions(+), 52 deletions(-)
>>
>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>> index 5db4b49..facf220 100644
>> --- a/lib/netdev-dpdk.c
>> +++ b/lib/netdev-dpdk.c
>> @@ -209,16 +209,53 @@ dpdk_rte_mzalloc(size_t sz)
>> void
>> free_dpdk_buf(struct ofpbuf *b)
>> {
>> - struct rte_mbuf *pkt;
>> -
>> - pkt = b->private_p;
>> - if (!pkt) {
>> - return;
>> - }
>> + struct rte_mbuf *pkt = (struct rte_mbuf *) b;
>>
>> rte_mempool_put(pkt->pool, pkt);
>> }
>>
>> +static void
>> +__rte_pktmbuf_init(struct rte_mempool *mp,
>> + __attribute__((unused)) void *opaque_arg,
>> + void *_m,
>> + __attribute__((unused)) unsigned i)
>
> Can use OVS_UNUSED here.
>
ok.
>> +{
>> + struct rte_mbuf *m = _m;
>> + uint32_t buf_len = mp->elt_size - sizeof(struct ofpbuf);
>> +
>> + RTE_MBUF_ASSERT(mp->elt_size >= sizeof(struct ofpbuf));
>> +
>> + memset(m, 0, mp->elt_size);
>> +
>> + /* start of buffer is just after mbuf structure */
>> + m->buf_addr = (char *)m + sizeof(struct ofpbuf);
>> + m->buf_physaddr = rte_mempool_virt2phy(mp, m) +
>> + sizeof(struct ofpbuf);
>> + m->buf_len = (uint16_t)buf_len;
>> +
>> + /* keep some headroom between start of buffer and data */
>> + m->pkt.data = (char*) m->buf_addr + RTE_MIN(RTE_PKTMBUF_HEADROOM,
>> m->buf_len);
>> +
>> + /* init some constant fields */
>
> Are these fields constant though the lifetime of 'm'?
>
Yes, they are constant for dpdk packet.
>> + m->type = RTE_MBUF_PKT;
>> + m->pool = mp;
>> + m->pkt.nb_segs = 1;
>> + m->pkt.in_port = 0xff;
>> +}
>> +
>> +static void
>> +ovs_rte_pktmbuf_init(struct rte_mempool *mp,
>> + __attribute__((unused)) void *opaque_arg,
>> + void *_m,
>> + __attribute__((unused)) unsigned i)
>> +{
>> + struct rte_mbuf *m = _m;
>> +
>> + __rte_pktmbuf_init(mp, opaque_arg, _m, i);
>> +
>> + ofpbuf_init_dpdk((struct ofpbuf *) m, m->buf_len);
>> +}
>> +
>> static struct dpdk_mp *
>> dpdk_mp_get(int socket_id, int mtu) OVS_REQUIRES(dpdk_mutex)
>> {
>> @@ -242,7 +279,7 @@ dpdk_mp_get(int socket_id, int mtu)
>> OVS_REQUIRES(dpdk_mutex)
>> MP_CACHE_SZ,
>> sizeof(struct rte_pktmbuf_pool_private),
>> rte_pktmbuf_pool_init, NULL,
>> - rte_pktmbuf_init, NULL,
>> + ovs_rte_pktmbuf_init, NULL,
>> socket_id, 0);
>>
>> if (dmp->mp == NULL) {
>> @@ -550,47 +587,22 @@ dpdk_queue_flush(struct netdev_dpdk *dev, int qid)
>> rte_spinlock_unlock(&txq->tx_lock);
>> }
>>
>> -inline static struct ofpbuf *
>> -build_ofpbuf(struct rte_mbuf *pkt)
>> -{
>> - struct ofpbuf *b;
>> -
>> - b = ofpbuf_new(0);
>> - b->private_p = pkt;
>> -
>> - ofpbuf_set_data(b, pkt->pkt.data);
>> - ofpbuf_set_base(b, (char *)ofpbuf_get_data(b) - DP_NETDEV_HEADROOM -
>> VLAN_ETH_HEADER_LEN);
>> - b->allocated = pkt->buf_len;
>> - ofpbuf_set_size(b, rte_pktmbuf_data_len(pkt));
>> - b->source = OFPBUF_DPDK;
>> -
>> - dp_packet_pad(b);
>> -
>> - return b;
>> -}
>> -
>> static int
>> netdev_dpdk_rxq_recv(struct netdev_rxq *rxq_, struct ofpbuf **packet, int *c)
>> {
>> struct netdev_rxq_dpdk *rx = netdev_rxq_dpdk_cast(rxq_);
>> struct netdev *netdev = rx->up.netdev;
>> struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
>> - struct rte_mbuf *burst_pkts[MAX_RX_QUEUE_LEN];
>> int nb_rx;
>> - int i;
>>
>> dpdk_queue_flush(dev, rxq_->queue_id);
>>
>> nb_rx = rte_eth_rx_burst(rx->port_id, rxq_->queue_id,
>> - burst_pkts, MAX_RX_QUEUE_LEN);
>> + (struct rte_mbuf **) packet, MAX_RX_QUEUE_LEN);
>
>
> The parameter 'packet' should be called 'packets' instead.
>
ok.
>> if (!nb_rx) {
>> return EAGAIN;
>> }
>>
>> - for (i = 0; i < nb_rx; i++) {
>> - packet[i] = build_ofpbuf(burst_pkts[i]);
>> - }
>> -
>> *c = nb_rx;
>>
>> return 0;
>> @@ -677,32 +689,23 @@ netdev_dpdk_send(struct netdev *netdev,
>> goto out;
>> }
>>
>> - rte_prefetch0(&ofpbuf->private_p);
>> - if (!may_steal ||
>> - !ofpbuf->private_p || ofpbuf->source != OFPBUF_DPDK) {
>> + if (!may_steal || ofpbuf->source != OFPBUF_DPDK) {
>> dpdk_do_tx_copy(netdev, (char *) ofpbuf_get_data(ofpbuf),
>> ofpbuf_get_size(ofpbuf));
>> +
>> + if (may_steal) {
>> + ofpbuf_delete(ofpbuf);
>> + }
>> } else {
>> - struct rte_mbuf *pkt;
>> int qid;
>>
>> - pkt = ofpbuf->private_p;
>> - ofpbuf->private_p = NULL;
>> - rte_pktmbuf_data_len(pkt) = ofpbuf_get_size(ofpbuf);
>> - rte_pktmbuf_pkt_len(pkt) = ofpbuf_get_size(ofpbuf);
>> -
>> qid = rte_lcore_id() % NR_QUEUE;
>>
>> - dpdk_queue_pkt(dev, qid, pkt);
>> + dpdk_queue_pkt(dev, qid, (struct rte_mbuf *)ofpbuf);
>>
>> - ofpbuf->private_p = NULL;
>> }
>> ret = 0;
>>
>> out:
>> - if (may_steal) {
>> - ofpbuf_delete(ofpbuf);
>> - }
>> -
>> return ret;
>> }
>>
>> diff --git a/lib/ofpbuf.c b/lib/ofpbuf.c
>> index f616b1c..749f7ff 100644
>> --- a/lib/ofpbuf.c
>> +++ b/lib/ofpbuf.c
>> @@ -134,9 +134,7 @@ ofpbuf_uninit(struct ofpbuf *b)
>> if (b->source == OFPBUF_MALLOC) {
>> free(ofpbuf_get_base(b));
>> }
>> - if (b->source == OFPBUF_DPDK) {
>> - free_dpdk_buf(b);
>> - }
>> + ovs_assert(b->source != OFPBUF_DPDK);
>> }
>> }
>>
>> diff --git a/lib/ofpbuf.h b/lib/ofpbuf.h
>> index 94651e4..ec3429a 100644
>> --- a/lib/ofpbuf.h
>> +++ b/lib/ofpbuf.h
>> @@ -41,7 +41,6 @@ enum OVS_PACKED_ENUM ofpbuf_source {
>> struct ofpbuf {
>> #ifdef DPDK_NETDEV
>> struct rte_mbuf mbuf; /* DPDK mbuf */
>> - void *private_p; /* private pointer for use by dpdk */
>> #else
>> void *base; /* First byte of allocated space. */
>> void *data; /* First byte actually in use. */
>> @@ -155,6 +154,11 @@ static inline void *ofpbuf_get_uninit_pointer(struct
>> ofpbuf *b)
>> static inline void ofpbuf_delete(struct ofpbuf *b)
>> {
>> if (b) {
>> + if (b->source == OFPBUF_DPDK) {
>> + free_dpdk_buf(b);
>> + return;
>> + }
>> +
>
> There is a comment in ofpbuf_get_uninit_pointer() that you may want to
> address in context of this patch.
>
> Otherwise:
>
> Acked-by: Jarno Rajahalme <jrajaha...@nicira.com>
>
>
Thanks for review. I pushed patches to master.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
