Re: [dpdk-dev] [PATCH v3 2/4] net/mrvl: add mrvl net pmd driver

[Tomasz Duszynski <tdu@semihalf.com>]

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 <jck@semihalf.com>
> > Tomasz Duszynski <tdu@semihalf.com>
> >
> > Marvell:
> > Dmitri Epshtein <dima@marvell.com>
> > Natalie Samsonov <nsamsono@marvell.com>
> >
> > Signed-off-by: Jacek Siuda <jck@semihalf.com>
> > Signed-off-by: Tomasz Duszynski <tdu@semihalf.com>
>
> <...>
>
> > +++ 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.

>
> <...>
>
> > +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 = 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.

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.

> <...>
>
> > +
> > +	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.

> > +		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