DPDK patches and discussions
 help / color / mirror / Atom feed
From: Shahaf Shuler <shahafs@mellanox.com>
To: Slava Ovsiienko <viacheslavo@mellanox.com>,
	"dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH 1/1] net/mlx5: add support for PF representor
Date: Tue, 16 Apr 2019 05:43:21 +0000	[thread overview]
Message-ID: <AM0PR0502MB3795793E582BD033A12F447CC3240@AM0PR0502MB3795.eurprd05.prod.outlook.com> (raw)
Message-ID: <20190416054321.DwnAKuJsh6pRnEa5B_AOdBHGnRXnpFbUQ8HO5Pm5igA@z> (raw)
In-Reply-To: <AM4PR05MB3265B7723D2B36325AE5A852D22B0@AM4PR05MB3265.eurprd05.prod.outlook.com>

Monday, April 15, 2019 12:12 PM, Slava Ovsiienko:
> Subject: RE: [dpdk-dev] [PATCH 1/1] net/mlx5: add support for PF
> representor
> 
> Hi, Shahaf
> 
> > -----Original Message-----
> > From: Shahaf Shuler
> > Sent: Sunday, April 14, 2019 10:43
> > To: Slava Ovsiienko <viacheslavo@mellanox.com>; dev@dpdk.org
> > Subject: RE: [dpdk-dev] [PATCH 1/1] net/mlx5: add support for PF
> > representor
> >
> > Hi Slava,
> >
> > Friday, April 12, 2019 6:48 PM, Viacheslav Ovsiienko:
> > > Subject: [dpdk-dev] [PATCH 1/1] net/mlx5: add support for PF
> > > representor
> > >
> > > On BlueField platform we have the new entity - PF representor.
> > > This one represents the PCI PF attached to external host on the side
> > > of
> > ARM.
> > > The traffic sent by the external host to the NIC via PF will be seem
> > > by ARM on this PF representor.
> > >
> > > This patch extends port recognizing capability on the base of
> > > physical port name. The following naming formats are supported:
> > >
> > >   - missing physical port name (no sysfs/netlink key) at all,
> > >     this is old style (before kernel 5.0) format, master assumed
> > >   - 1 (decimal digits) - old style (before kernel 5.0) format,
> > >     exists only for representors, master does not have physical
> > >     port name at all (see above)
> > >   - p0 ("p" followed by decimal digits), new style (kernel version
> > >     is 5.0 or higher, Mellanox OFED 4.6 or higher) name format
> > >     for uplink representor, plays the role of master
> > >   - pf0vf0 ("pf" followed by PF index concatenated with "vf"
> > >     followed by VF index),  new style (kernel version  is 5.0
> > >     or higher, Mellanox OFED 4.6 or higher) name format for
> > >     VF representor. If index of VF is "-1" it is a special case
> > >     of host PF representor, this representor must be indexed in
> > >     devargs as 65535, for example representor=[0-3,65535] will
> > >     allow representors for VF0, VF1, VF2, VF3 and host PF.
> > >     Note: do not specify representor=[0-65535] it causes devargs
> > >     processing error, because number of ports (rte_eth_dev) is
> > >     limited.
> > >
> >
> > The above is a bit complex to understand and in fact we have 2 modes:
> > 1. legacy - phys_port_name are numbers. Master doesn't have
> > phys_port_name 2. modern - phys_port_name are strings.
> > uplink representor is p%d
> > representors are (including PF representor) pf%dvf%d. the vf index for
> > the PF representor is 65535.
> 
> OK, I'll try to update the commit message to make it more clear.
> >
> > > Applications should distinguish representors and master devices
> > > exclusively by device flag RTE_ETH_DEV_REPRESENTOR and do not rely
> > > on switch port_id (mlx5 PMD deduces ones from representor_id) values
> > > returned by
> > > dev_infos_get() API.
> > >
> >
> > Please also reference the kernel commit which introduce the name
> change.
> OK.
> 
> >
> > > Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> > > ---
> > >  drivers/net/mlx5/mlx5.h        | 11 ++++++-
> > >  drivers/net/mlx5/mlx5_ethdev.c | 68
> +++++++++++++++++++++++++++----
> > > -----------
> > >  drivers/net/mlx5/mlx5_nl.c     | 42 +++++++++++++++++---------
> > >  3 files changed, 82 insertions(+), 39 deletions(-)
> > >
> > > diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h index
> > > 8eb1019..81c02ce 100644
> > > --- a/drivers/net/mlx5/mlx5.h
> > > +++ b/drivers/net/mlx5/mlx5.h
> > > @@ -80,11 +80,20 @@ struct mlx5_mp_param {
> > >  /** Key string for IPC. */
> > >  #define MLX5_MP_NAME "net_mlx5_mp"
> > >
> > > +/* Recognized Infiniband device physical port name types. */ enum
> > > +mlx5_phys_port_name_type {
> > > +	MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN = 0, /* Unrecognized.
> > > */
> > > +	MLX5_PHYS_PORT_NAME_TYPE_LEGACY, /* before kernel ver < 5.0
> > > */
> > > +	MLX5_PHYS_PORT_NAME_TYPE_UPLINK, /* p0, kernel ver >= 5.0 */
> > > +	MLX5_PHYS_PORT_NAME_TYPE_PFVF, /* pf0vf0, kernel ver >= 5.0
> > > */ };
> > > +
> > >  /** Switch information returned by mlx5_nl_switch_info(). */
> > > struct mlx5_switch_info {
> > >  	uint32_t master:1; /**< Master device. */
> > >  	uint32_t representor:1; /**< Representor device. */
> > > -	uint32_t port_name_new:1; /**< Rep. port name is in new format.
> > > */
> > > +	enum mlx5_phys_port_name_type name_type; /** < Port name
> > > type. */
> > > +	int32_t pf_num; /**< PF number (valid for pfxvfx format only). */
> > >  	int32_t port_name; /**< Representor port name. */
> > >  	uint64_t switch_id; /**< Switch identifier. */  }; diff --git
> > > a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
> > > index 3992918..371989f 100644
> > > --- a/drivers/net/mlx5/mlx5_ethdev.c
> > > +++ b/drivers/net/mlx5/mlx5_ethdev.c
> > > @@ -1395,12 +1395,11 @@ int mlx5_fw_version_get(struct rte_eth_dev
> > > *dev, char *fw_ver, size_t fw_size)
> > >  	struct mlx5_switch_info data = {
> > >  		.master = 0,
> > >  		.representor = 0,
> > > -		.port_name_new = 0,
> > > +		.name_type = MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN,
> > >  		.port_name = 0,
> > >  		.switch_id = 0,
> > >  	};
> > >  	DIR *dir;
> > > -	bool port_name_set = false;
> > >  	bool port_switch_id_set = false;
> > >  	bool device_dir = false;
> > >  	char c;
> > > @@ -1423,8 +1422,7 @@ int mlx5_fw_version_get(struct rte_eth_dev
> > *dev,
> > > char *fw_ver, size_t fw_size)
> > >  		ret = fscanf(file, "%s", port_name);
> > >  		fclose(file);
> > >  		if (ret == 1)
> > > -			port_name_set =
> > > mlx5_translate_port_name(port_name,
> > > -								 &data);
> > > +			mlx5_translate_port_name(port_name, &data);
> > >  	}
> > >  	file = fopen(phys_switch_id, "rb");
> > >  	if (file == NULL) {
> > > @@ -1440,8 +1438,15 @@ int mlx5_fw_version_get(struct rte_eth_dev
> > > *dev, char *fw_ver, size_t fw_size)
> > >  		closedir(dir);
> > >  		device_dir = true;
> > >  	}
> > > -	data.master = port_switch_id_set && (!port_name_set ||
> > > device_dir);
> > > -	data.representor = port_switch_id_set && port_name_set &&
> > > !device_dir;
> > > +	if (port_switch_id_set) {
> > > +		data.master =
> > > +			device_dir ||
> > > +			data.name_type ==
> > > MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN ||
> > > +			data.name_type ==
> > > MLX5_PHYS_PORT_NAME_TYPE_UPLINK;
> > > +		data.representor = !device_dir &&
> > > +			(data.name_type ==
> > > MLX5_PHYS_PORT_NAME_TYPE_LEGACY ||
> > > +			 data.name_type ==
> > > MLX5_PHYS_PORT_NAME_TYPE_PFVF);
> >
> >
> > Why we need to split the logic of the master/representor detection
> > between the mlx5_translate_port_name and the caller function?
> 
> We have two stages:
> - gathering the port attributes (name, vf_num, switchdev_id, presence of
> device directory, etc.)  in callbacks
> - analyzing the parameters on gathering completion. We need all the
> parameters to make a reliable master/representor  recognition.
> 
> Translate routine is called on gathering stage and just recognizes the name
> format, extracts useful values (pf/vf num) and stores the results in compact
> form. It is easier than storing the entire port name.
> 
> >
> > The way I envision it is mlx5_tranlate_port_name receives the
> > phys_port_name and maybe more metadata and return the port
> This "metadata" may be not gathered yet at the moment of port name
> processing.
> 
> > classification (master/representor) and the representor/pf number.
> > No need for data.master = some_logic(translate_port_name_info).
> >
> > Inside the translate function I would expect to have 2 smaller function:
> > 1. to handle the modern format (strings) 2. to handle the legacy
> > format
> > (integers)
> 
> Actually this patch is also some kind of preparation for coming subfuctions.
> If we have set of mutual exclusive formats the switch/case seems to be the
> most native way to treat these ones. I'd prefer to keep switch/case.
> Comments are clear and it is easy to understand where legacy/new format is
> treated. And it is easy to extend for coming subfunctions.
> 
> I think we should isolate master/representor recognition logic into separate
> routine, "translate" routine is for parameter gathering stage, "recognize" for
> analyzing.

If you insist to split it then we should have 2 separate routines, unlike the current approach where switch/case are spread in different places.

  parent reply	other threads:[~2019-04-16  5:43 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-04-12 15:48 Viacheslav Ovsiienko
2019-04-12 15:48 ` Viacheslav Ovsiienko
2019-04-14  7:42 ` Shahaf Shuler
2019-04-14  7:42   ` Shahaf Shuler
2019-04-15  9:11   ` Slava Ovsiienko
2019-04-15  9:11     ` Slava Ovsiienko
2019-04-16  5:43     ` Shahaf Shuler [this message]
2019-04-16  5:43       ` Shahaf Shuler
2019-04-16 14:10 ` [dpdk-dev] [PATCH v2] " Viacheslav Ovsiienko
2019-04-16 14:10   ` Viacheslav Ovsiienko
2019-04-18 14:26   ` Dekel Peled
2019-04-18 14:26     ` Dekel Peled
2019-04-18 18:54   ` Shahaf Shuler
2019-04-18 18:54     ` Shahaf Shuler

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=AM0PR0502MB3795793E582BD033A12F447CC3240@AM0PR0502MB3795.eurprd05.prod.outlook.com \
    --to=shahafs@mellanox.com \
    --cc=dev@dpdk.org \
    --cc=viacheslavo@mellanox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).