From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf0-f42.google.com (mail-lf0-f42.google.com [209.85.215.42]) by dpdk.org (Postfix) with ESMTP id 5FA741B655 for ; Wed, 4 Oct 2017 15:19:47 +0200 (CEST) Received: by mail-lf0-f42.google.com with SMTP id p184so13119920lfe.12 for ; Wed, 04 Oct 2017 06:19:47 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=semihalf-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to :user-agent; bh=Ozu0HRljt7G4hnCUTUpToXszmdjdJApmoGEZG5enrKA=; b=WkZ56YJf3CvvommDcVTlkVzQBitDwAVM7Gshd55fIwejDDxVV29+ssJvzW0eBt13ot Bg8OxWp63pQs+0YsJbjNLVW0kSAZ6fia8IF/AfnSpj9qBr/TbFtTpKPUi7pD1uYy8MYR MnVAg3oK3kVL4bCB7Fl5ih3jVWyXN11c3hOgtzwXfuextREJIohoak2NNCA/f8qt6QRg yyloq0fiC5zieD737Qqz4QpTOa7FwT+fTMHSmIGvVde0meTriOxgl0pj3PJN5AdSjwIY /BP8LTXJmTnRjk4aWZdLZzJ4iG7iqfhDSrbUh/KZ7NVCFdVhHELc4NbsG38frUm8NTzY zskA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=Ozu0HRljt7G4hnCUTUpToXszmdjdJApmoGEZG5enrKA=; b=KrnVV4614FwCuOFseSk3pv5GJhbpNiFqy43C4daXFVN+eXll/LNXeLZl08+fNcPO5c CQBZ/Zmo3xC99M8avDVPsfglpXOtUwEEbd8//eeamAy6y2i44bvoq87z9FHW5lIH+4A6 tHFLpyhUPIwCeNXmcZ9VvVbeubinAUXIgYCE5N/OgiJuv2tWrsUX2YowF5c+r9h6rA3A cFvTcuFJFk1qti6il4TKB8kd9rf++SUp0ch/9RR4my3JPv5ATjz3zBDmsYc9p/l4cBRx GosCLp4p+hyK4RHtimvKU8FLd1M+LL+6Cyf7OsZT1r09pjQczhbaZjbxW7j+mHyZmnP0 M14w== X-Gm-Message-State: AHPjjUjbR4cY+sA4CakB8+nGhj3A0j7t6lkm/vwE3rHVXx7yjDtVr4/f l40Z7LsU+UXNWimhYBsMbZvrMswJDIg= X-Google-Smtp-Source: AOwi7QCoxt7iBZx87pruMAhm0A602c4T57+0useKAF15/Sl/z9oxJT/TQsfyP3wNaz+oOlxJfKfGbQ== X-Received: by 10.46.68.214 with SMTP id b83mr9658729ljf.86.1507123186631; Wed, 04 Oct 2017 06:19:46 -0700 (PDT) Received: from localhost (31-172-191-173.noc.fibertech.net.pl. [31.172.191.173]) by smtp.gmail.com with ESMTPSA id x25sm3425856lja.5.2017.10.04.06.19.44 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 04 Oct 2017 06:19:45 -0700 (PDT) Date: Wed, 4 Oct 2017 15:19:44 +0200 From: Tomasz Duszynski To: Ferruh Yigit Cc: Tomasz Duszynski , dev@dpdk.org, mw@semihalf.com, dima@marvell.com, nsamsono@marvell.com, Jianbo.liu@linaro.org, Jacek Siuda Message-ID: <20171004131944.GC5668@tdu> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable In-Reply-To: User-Agent: Mutt/1.5.23.1 (2014-03-12) 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 13:19:47 -0000 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 adapte= r. > > 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=3Dn > > CONFIG_RTE_LIBRTE_NFP_DEBUG=3Dn > > > > # > > +# Compile Marvell PMD driver > > +# > > +CONFIG_RTE_LIBRTE_MRVL_PMD=3Dn > > +CONFIG_RTE_LIBRTE_MRVL_DEBUG=3Dn > > +CONFIG_RTE_MRVL_MUSDK_DMA_MEMSIZE=3D41943040 > > 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. > > <...> > > > +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. > > > +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 =3D librte_pmd_mrvl.a > > + > > +# library version > > +LIBABIVER :=3D 1 > > + > > +# versioning export map > > +EXPORT_MAP :=3D rte_pmd_mrvl_version.map > > + > > +# external library dependencies > > +CFLAGS +=3D -I$(LIBMUSDK_PATH)/include > > +CFLAGS +=3D -DMVCONF_ARCH_DMA_ADDR_T_64BIT > > +CFLAGS +=3D -DCONF_PP2_BPOOL_COOKIE_SIZE=3D32 > > +CFLAGS +=3D $(WERROR_FLAGS) > > +CFLAGS +=3D -O3 > > +LDLIBS +=3D -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. On the other hand, if we are building static libraries both -lmusdk and -L$(LIBMUSDK_PATH)/lib are added to specific _LDLIBS which in t= urn ends up in LDLIBS. > > > +LDLIBS +=3D -lmusdk > > + > > +# library source files > > +SRCS-$(CONFIG_RTE_LIBRTE_MRVL_PMD) +=3D mrvl_ethdev.c > > +SRCS-$(CONFIG_RTE_LIBRTE_MRVL_PMD) +=3D mrvl_qos.c > > + > > +# library dependencies > > +DEPDIRS-$(CONFIG_RTE_LIBRTE_MRVL_PMD) +=3D 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 struct= ure > > + * 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 fie= ld 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 =3D 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 =3D 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. > <...> > > > + > > + if (rte_spinlock_trylock(&q->priv->lock) =3D=3D 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 =3D mrvl_get_bpool_size(bpool->pp2_id, bpool->id); > > + > > + if (unlikely(num <=3D q->priv->bpool_min_size || > > + (!rx_done && num < q->priv->bpool_init_size))) { > > + ret =3D 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 =3D 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 =3D 0; i < pkt_to_remove; i++) { > > + pp2_bpool_get_buff(hifs[core_id], bpool, &buff); > > + mbuf =3D (struct rte_mbuf *) > > + (cookie_addr_high | buff.cookie); > > + rte_pktmbuf_free(mbuf); > > + } > > + mrvl_port_bpool_size > > + [bpool->pp2_id][bpool->id][core_id] -=3D > > + pkt_to_remove; > > + } > > + rte_spinlock_unlock(&q->priv->lock); > > + } > > + > > + return rx_done; > > +} > > <...> > > > + cfgnum =3D 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 =3D=3D 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 =3D=3D -EEXIST is correct, it means DMA > > + * has been already initialized (by another PMD). > > + */ > > + ret =3D mv_sys_dma_mem_init(RTE_MRVL_MUSDK_DMA_MEMSIZE); > > + if (ret < 0 && ret !=3D -EEXIST) > > + goto out_free_kvlist; > > + > > + ret =3D mrvl_init_pp2(); > > + if (ret) { > > + RTE_LOG(ERR, PMD, "Failed to init PP!\n"); > > + goto out_deinit_dma; > > + } > > + > > + ret =3D mrvl_init_hifs(); > > + if (ret) > > + goto out_deinit_hifs; > > + > > + for (i =3D 0; i < ifnum; i++) { > > + RTE_LOG(INFO, PMD, "Creating %s\n", ifnames[i]); > > + ret =3D 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=3Dxx0,config=3Dyy0 --vdev > net_mrlv1,iface=3Dxx1,config=3Dyy1 ..." > > 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. > > + if (ret) > > + goto out_cleanup; > > + } > > + > > + rte_kvargs_free(kvlist); > > + > > + memset(mrvl_port_bpool_size, 0, sizeof(mrvl_port_bpool_size)); > > + > > + mrvl_lcore_first =3D RTE_MAX_LCORE; > > + mrvl_lcore_last =3D 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=C5=84ski