On Sat, Jun 16, 2018 at 08:57:51AM +0000, Xueming(Steven) Li wrote:
> Reviewed-by: Xueming Li <xuemi...@mellanox.com>
>
> Minor comments inside:
>
> > -----Original Message-----
> > From: dev <dev-boun...@dpdk.org> On Behalf Of Adrien Mazarguil
> > Sent: Thursday, June 14, 2018 4:35 PM
> > To: Shahaf Shuler <shah...@mellanox.com>
> > Cc: dev@dpdk.org
> > Subject: [dpdk-dev] [PATCH v2 6/7] net/mlx5: probe all port representors
> >
> > Probe existing port representors in addition to their master device and
> > associate them automatically.
> >
> > To avoid name collision between Ethernet devices, their names use the same
> > convention as ixgbe and
> > i40e PMDs, that is, instead of only a PCI address in DBDF notation:
> >
> > - "net_{DBDF}_0" for master/switch devices.
> > - "net_{DBDF}_representor_{rep}" with "rep" starting from 0 for port
> > representors.
> >
> > Both optionally suffixed with "_port_{num}" instead of " port {num}" for
> > devices that expose several
> > Verbs ports (note this is never the case on mlx5, but kept for historical
> > reasons for the time being).
> >
> > (Patch based on prior work from Yuanhan Liu)
> >
> > Signed-off-by: Adrien Mazarguil <adrien.mazarg...@6wind.com>
> > --
> > v2 changes:
> >
> > - Added representor information to dev_infos_get(). DPDK port ID of master
> > device is now stored in the private structure to retrieve it
> > conveniently.
> > - Master device is assigned dummy representor ID value -1 to better
> > distinguish from the the first actual representor reported by
> > dev_infos_get() as those are indexed from 0.
> > - Added RTE_ETH_DEV_REPRESENTOR device flag.
> > ---
> > drivers/net/mlx5/mlx5.c | 138 ++++++++++++++++++++++++--------
> > drivers/net/mlx5/mlx5.h | 9 ++-
> > drivers/net/mlx5/mlx5_ethdev.c | 151 ++++++++++++++++++++++++++++++++----
> > drivers/net/mlx5/mlx5_mac.c | 2 +-
> > drivers/net/mlx5/mlx5_stats.c | 6 +-
> > 5 files changed, 252 insertions(+), 54 deletions(-)
> >
> > diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c index
> > 498f80c89..716c9d9a5 100644
<snip>
> > --- a/drivers/net/mlx5/mlx5.c
> > +++ b/drivers/net/mlx5/mlx5.c
> > @@ -304,6 +304,9 @@ mlx5_dev_close(struct rte_eth_dev *dev)
> > if (ret)
> > DRV_LOG(WARNING, "port %u some flows still remain",
> > dev->data->port_id);
> > + if (!priv->representor &&
> > + priv->domain_id != RTE_ETH_DEV_SWITCH_DOMAIN_ID_INVALID)
> > + claim_zero(rte_eth_switch_domain_free(priv->domain_id));
> > memset(priv, 0, sizeof(*priv));
> > }
> >
> > @@ -648,6 +651,10 @@ mlx5_uar_init_secondary(struct rte_eth_dev *dev)
> > * Verbs device attributes.
> > * @param port
> > * Verbs port to use (indexed from 1).
> > + * @param master
> > + * Master device in case @p ibv_dev is a port representor.
> > + * @param rep_id
> > + * Representor identifier when @p master is non-NULL.
> > *
> > * @return
> > * A valid Ethernet device object on success, NULL otherwise and
> > rte_errno
> > @@ -658,7 +665,9 @@ mlx5_dev_spawn_one(struct rte_device *dpdk_dev,
> > struct ibv_device *ibv_dev,
> > int vf,
> > const struct ibv_device_attr_ex *attr,
> > - unsigned int port)
> > + unsigned int port,
> > + struct rte_eth_dev *master,
> > + unsigned int rep_id)
> > {
> > struct ibv_context *ctx;
> > struct ibv_port_attr port_attr;
> > @@ -802,11 +811,14 @@ mlx5_dev_spawn_one(struct rte_device *dpdk_dev,
> > " old OFED/rdma-core version or firmware configuration");
> > #endif
> > config.mpls_en = mpls_en;
> > - if (attr->orig_attr.phys_port_cnt > 1)
> > - snprintf(name, sizeof(name), "%s port %u",
> > - dpdk_dev->name, port);
> > + if (!master)
> > + snprintf(name, sizeof(name), "net_%s_0", dpdk_dev->name);
> > else
> > - snprintf(name, sizeof(name), "%s", dpdk_dev->name);
> > + snprintf(name, sizeof(name), "net_%s_representor_%u",
> > + dpdk_dev->name, rep_id);
> > + if (attr->orig_attr.phys_port_cnt > 1)
> > + snprintf(name, sizeof(name), "%s_port_%u", name, port);
> > + DRV_LOG(DEBUG, "naming Ethernet device \"%s\"", name);
> > if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
> > eth_dev = rte_eth_dev_attach_secondary(name);
> > if (eth_dev == NULL) {
> > @@ -883,6 +895,30 @@ mlx5_dev_spawn_one(struct rte_device *dpdk_dev,
> > priv->port = port;
> > priv->pd = pd;
> > priv->mtu = ETHER_MTU;
> > + /*
> > + * Allocate a switch domain for master devices and share it with
> > + * port representors.
> > + */
> > + if (!master) {
> > + priv->representor = 0;
> > + priv->master_id = -1; /* Updated once known. */
> > + priv->domain_id = RTE_ETH_DEV_SWITCH_DOMAIN_ID_INVALID;
>
> Domain_id will override below.
It's done as a safety measure. If rte_eth_switch_domain_alloc() happened to
fail, rte_eth_switch_domain_free() would otherwise be attempted on an
uninitialized value when cleaning up priv, possibly destroying an unrelated
domain. This is prevented thanks to RTE_ETH_DEV_SWITCH_DOMAIN_ID_INVALID.
> > + priv->rep_id = -1; /* Dummy unique value. */
> > + err = rte_eth_switch_domain_alloc(&priv->domain_id);
> > + if (err) {
> > + err = rte_errno;
> > + DRV_LOG(ERR, "unable to allocate switch domain: %s",
> > + strerror(rte_errno));
> > + goto error;
> > + }
> > + } else {
> > + priv->representor = 1;
> > + priv->master_id =
> > + ((struct priv *)master->data->dev_private)->master_id;
> > + priv->domain_id =
> > + ((struct priv *)master->data->dev_private)->domain_id;
> > + priv->rep_id = rep_id;
> > + }
>
> Do you think such information should be set as well in secondary process?
Unless I'm mistaken, it's implicitly the case as secondaries do not allocate
their own private structure, they inherit it from the primary.
Thanks for the review.
--
Adrien Mazarguil
6WIND
