From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by dpdk.org (Postfix) with ESMTP id AFDE41B387 for ; Wed, 4 Oct 2017 02:28:49 +0200 (CEST) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga105.fm.intel.com with ESMTP; 03 Oct 2017 17:28:48 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.42,475,1500966000"; d="scan'208";a="906430620" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.241.224.245]) ([10.241.224.245]) by FMSMGA003.fm.intel.com with ESMTP; 03 Oct 2017 17:28:47 -0700 To: Tomasz Duszynski , dev@dpdk.org Cc: mw@semihalf.com, dima@marvell.com, nsamsono@marvell.com, Jianbo.liu@linaro.org, Jacek Siuda References: <1506594158-15721-2-git-send-email-tdu@semihalf.com> <1507031500-11473-1-git-send-email-tdu@semihalf.com> <1507031500-11473-3-git-send-email-tdu@semihalf.com> From: Ferruh Yigit Message-ID: Date: Wed, 4 Oct 2017 01:28:47 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <1507031500-11473-3-git-send-email-tdu@semihalf.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v3 2/4] net/mrvl: add mrvl net pmd driver X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 04 Oct 2017 00:28:50 -0000 On 10/3/2017 12:51 PM, Tomasz Duszynski wrote: > Add support for the Marvell PPv2 (Packet Processor v2) 1/10 Gbps adapter. > Driver is based on external, publicly available, light-weight Marvell > MUSDK library that provides access to network packet processor. > > Driver comes with support for the following features: > > * Speed capabilities > * Link status > * Queue start/stop > * MTU update > * Jumbo frame > * Promiscuous mode > * Allmulticast mode > * Unicast MAC filter > * Multicast MAC filter > * RSS hash > * VLAN filter > * CRC offload > * L3 checksum offload > * L4 checksum offload > * Packet type parsing > * Basic stats > * Stats per queue > > Driver was engineered cooperatively by Semihalf and Marvell teams. > > Semihalf: > Jacek Siuda > Tomasz Duszynski > > Marvell: > Dmitri Epshtein > Natalie Samsonov > > Signed-off-by: Jacek Siuda > Signed-off-by: Tomasz Duszynski <...> > +++ b/config/common_base > @@ -262,6 +262,13 @@ CONFIG_RTE_LIBRTE_NFP_PMD=n > CONFIG_RTE_LIBRTE_NFP_DEBUG=n > > # > +# Compile Marvell PMD driver > +# > +CONFIG_RTE_LIBRTE_MRVL_PMD=n > +CONFIG_RTE_LIBRTE_MRVL_DEBUG=n > +CONFIG_RTE_MRVL_MUSDK_DMA_MEMSIZE=41943040 Is dma memsize needs to be a configuration option? <...> > +include $(RTE_SDK)/mk/rte.vars.mk > + > +ifneq ($(MAKECMDGOALS),clean) > +ifneq ($(MAKECMDGOALS),config) > +ifeq ($(LIBMUSDK_PATH),) > +$(error "Please define LIBMUSDK_PATH environment variable") Not sure how to resolve this dependency. What do you think adding this as configuration option? Or DPDK just adds the -lmusdk external dependency and while compiling for marvel EXTRA_LDFLAGS parameter should be pass with "-L$(LIBMUSDK_PATH)" and this can be documented in marvel doc. What do you think? > +endif > +ifeq ($(CONFIG_RTE_LIBRTE_CFGFILE),n) > +$(error "RTE_LIBRTE_CFGFILE must be enabled in configuration!") This can be also handled in drivers/net/Makefile, it can be possible to add check there for LIBRTE_CFGFILE dependency. > +endif > +endif > +endif > + > +# library name > +LIB = librte_pmd_mrvl.a > + > +# library version > +LIBABIVER := 1 > + > +# versioning export map > +EXPORT_MAP := rte_pmd_mrvl_version.map > + > +# external library dependencies > +CFLAGS += -I$(LIBMUSDK_PATH)/include > +CFLAGS += -DMVCONF_ARCH_DMA_ADDR_T_64BIT > +CFLAGS += -DCONF_PP2_BPOOL_COOKIE_SIZE=32 > +CFLAGS += $(WERROR_FLAGS) > +CFLAGS += -O3 > +LDLIBS += -L$(LIBMUSDK_PATH)/lib This can be LDFLAGS instead of LDLIBS > +LDLIBS += -lmusdk > + > +# library source files > +SRCS-$(CONFIG_RTE_LIBRTE_MRVL_PMD) += mrvl_ethdev.c > +SRCS-$(CONFIG_RTE_LIBRTE_MRVL_PMD) += mrvl_qos.c > + > +# library dependencies > +DEPDIRS-$(CONFIG_RTE_LIBRTE_MRVL_PMD) += lib/librte_cfgfile These variables no more used, you can drop this. drivers/net/Makefile used for this, you are already updating that file, librte_cfgfile needs to be added there. > + > +include $(RTE_SDK)/mk/rte.lib.mk <...> > +/* > + * To use buffer harvesting based on loopback port shadow queue structure > + * was introduced for buffers information bookkeeping. > + * > + * Before sending the packet, related buffer information (pp2_buff_inf) is > + * stored in shadow queue. After packet is transmitted no longer used > + * packet buffer is released back to it's original hardware pool, > + * on condition it originated from interface. > + * In case it was generated by application itself i.e: mbuf->port field is > + * 0xff then its released to software mempool. You already explained here but can you please give more details why shadow queue needed? > + */ > +struct mrvl_shadow_txq { > + int head; /* write index - used when sending buffers */ > + int tail; /* read index - used when releasing buffers */ > + u16 size; /* queue occupied size */ > + u16 num_to_release; /* number of buffers sent, that can be released */ > + struct buff_release_entry ent[MRVL_PP2_TX_SHADOWQ_SIZE]; /* q entries */ > +}; > + > +struct mrvl_rxq { > + struct mrvl_priv *priv; > + struct rte_mempool *mp; > + int queue_id; > + int port_id; > + int cksum_enabled; > + uint64_t bytes_recv; > + uint64_t drop_mac; > +}; > + > +struct mrvl_txq { > + struct mrvl_priv *priv; > + int queue_id; > + int port_id; > + uint64_t bytes_sent; > +}; > + <...> > +static int > +mrvl_dev_start(struct rte_eth_dev *dev) > +{ > + struct mrvl_priv *priv = dev->data->dev_private; > + char match[MRVL_MATCH_LEN]; > + int ret; > + > + snprintf(match, sizeof(match), "ppio-%d:%d", > + priv->pp_id, priv->ppio_id); > + priv->ppio_params.match = match; Why this match is used, just a reminder that match is only valid for the scope of this function, after this function it will be invalid. <...> > + > + if (rte_spinlock_trylock(&q->priv->lock) == 1) { Why getting lock in Rx data path? > + num = mrvl_get_bpool_size(bpool->pp2_id, bpool->id); > + > + if (unlikely(num <= q->priv->bpool_min_size || > + (!rx_done && num < q->priv->bpool_init_size))) { > + ret = mrvl_fill_bpool(q, MRVL_BURST_SIZE); > + if (ret) > + RTE_LOG(ERR, PMD, "Failed to fill bpool\n"); > + } else if (unlikely(num > q->priv->bpool_max_size)) { > + int i; > + int pkt_to_remove = num - q->priv->bpool_init_size; > + struct rte_mbuf *mbuf; > + struct pp2_buff_inf buff; > + > + RTE_LOG(DEBUG, PMD, > + "\nport-%d:%d: bpool %d oversize - remove %d buffers (pool size: %d -> %d)\n", > + bpool->pp2_id, q->priv->ppio->port_id, > + bpool->id, pkt_to_remove, num, > + q->priv->bpool_init_size); > + > + for (i = 0; i < pkt_to_remove; i++) { > + pp2_bpool_get_buff(hifs[core_id], bpool, &buff); > + mbuf = (struct rte_mbuf *) > + (cookie_addr_high | buff.cookie); > + rte_pktmbuf_free(mbuf); > + } > + mrvl_port_bpool_size > + [bpool->pp2_id][bpool->id][core_id] -= > + pkt_to_remove; > + } > + rte_spinlock_unlock(&q->priv->lock); > + } > + > + return rx_done; > +} <...> > + cfgnum = rte_kvargs_count(kvlist, MRVL_CFG_ARG); > + if (cfgnum > 1) { > + RTE_LOG(ERR, PMD, "Cannot handle more than one config file!\n"); > + goto out_free_kvlist; > + } else if (cfgnum == 1) { > + rte_kvargs_process(kvlist, MRVL_CFG_ARG, > + mrvl_get_qoscfg, &mrvl_qos_cfg); Is the expected format/contect of the config file documented? How one can know how to create a config file? > + } > + > + /* > + * ret == -EEXIST is correct, it means DMA > + * has been already initialized (by another PMD). > + */ > + ret = mv_sys_dma_mem_init(RTE_MRVL_MUSDK_DMA_MEMSIZE); > + if (ret < 0 && ret != -EEXIST) > + goto out_free_kvlist; > + > + ret = mrvl_init_pp2(); > + if (ret) { > + RTE_LOG(ERR, PMD, "Failed to init PP!\n"); > + goto out_deinit_dma; > + } > + > + ret = mrvl_init_hifs(); > + if (ret) > + goto out_deinit_hifs; > + > + for (i = 0; i < ifnum; i++) { > + RTE_LOG(INFO, PMD, "Creating %s\n", ifnames[i]); > + ret = mrvl_eth_dev_create(vdev, ifnames[i]); So you are supporting multiple ethdev devices created by single vdev device, by providing multiple "iface" argument in device args. This will cause eal create single virtual device but driver create multiple ethdev devices. I don't see direct problem with this but lets think about it. This can be problem if you want to provide ethdev specific device arguments. Perhaps that is why you need to provide a config file ? It can be an option to define each ethdev with: "--vdev net_mrvl0,iface=xx0,config=yy0 --vdev net_mrlv1,iface=xx1,config=yy1 ..." This may remove your dependecy to librte_cfgfile. > + if (ret) > + goto out_cleanup; > + } > + > + rte_kvargs_free(kvlist); > + > + memset(mrvl_port_bpool_size, 0, sizeof(mrvl_port_bpool_size)); > + > + mrvl_lcore_first = RTE_MAX_LCORE; > + mrvl_lcore_last = 0; > + > + RTE_LCORE_FOREACH(core_id) { > + mrvl_set_first_last_cores(core_id); This sets limits of core_id. Why you need to know this in PMD level? <...>