From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 76C937D4E for ; Thu, 5 Oct 2017 19:37:35 +0200 (CEST) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 05 Oct 2017 10:37:34 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.42,481,1500966000"; d="scan'208";a="907125202" Received: from unknown (HELO [10.241.225.33]) ([10.241.225.33]) by FMSMGA003.fm.intel.com with ESMTP; 05 Oct 2017 10:37:29 -0700 To: Tomasz Duszynski Cc: dev@dpdk.org, 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> <20171004131944.GC5668@tdu> From: Ferruh Yigit Message-ID: Date: Thu, 5 Oct 2017 18:37:26 +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: <20171004131944.GC5668@tdu> 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: Thu, 05 Oct 2017 17:37:36 -0000 On 10/4/2017 2:19 PM, Tomasz Duszynski wrote: > On Wed, Oct 04, 2017 at 01:28:47AM +0100, Ferruh Yigit wrote: >> 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? > > That config option is used both by NET and CRYPTO drivers. In case NET > and CRYPTO are used together i.e ipsec-secgw then DMA_MEMSIZE must be > the set to the same size. Putting this configuration option in .config > makes sure DMA_MEMSIZE stays synchronized. OK. > >> >> <...> >> >>> +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? > > All other drivers with external dependencies follow the same approach. > >> >> 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? > > Both solutions are reasonable. The former was chosen because that's what the > other drivers do. OK. > >> >>> +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. >> > > ACK > >>> +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 > > Moving that to LDFLAGS will break compilation in case > CONFIG_RTE_BUILD_SHARED_LIB is set as -L... does not show up on command > line thus linker does not know where to look extra library up. > I may be wrong but it looks as if specifying LDFLAGS in driver's > Makefile is no-op. I would expect LDFLAGS will work, but if it is breaking the build, please keep as it is, we can check and fix this later. > > On the other hand, if we are building static libraries both > -lmusdk and -L$(LIBMUSDK_PATH)/lib are added to specific _LDLIBS which in turn > ends up in 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. >> > > ACK > >>> + >>> +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? > > It's used for mbuf harvesting in tx-path. Instead of releasing pushed > out mbuf to mempool and allocating it once again later on, mbuf is > stored in the shadow queue and returned back to hardware buffer manager > after being sent. > >> >>> + */ >>> +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. >> > > Keeping match locally is fine. That's used to tell MUSDK which physical > port to configure, i.e ppio-0:1 means to configure port 1 on packet > processor 0. Armada 8k has to such packet processor, while armada 7k > only one. Ok, thanks for clarification. > >> <...> >> >>> + >>> + if (rte_spinlock_trylock(&q->priv->lock) == 1) { >> >> Why getting lock in Rx data path? >> > > In multi-core and multi-queue case some kind of protection is > necessary so that several cores cannot modify bpool at > the same time. > >>> + 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? >> > > Right, documentation is missing for that. Will add in v4. > >>> + } >>> + >>> + /* >>> + * 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. >> > > Currently there's not need to passing separate options to each created > device. As for configuration file it handles all devices at once. Ok, that was an option... > >>> + 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? > > It's just to limit number of entries in mrvl_port_bpool_size we iterate > over every time we want to count the total number of buffers in the > hardware buffer pool. > >> >> <...> >> > > -- > - Tomasz Duszyński >