netdev - Re: [PATCH net-next 10/16] net/mlx5e: Add devlink based SRIOV mode changes (legacy --> offloads)

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 28 Jun 2016 10:49:29 -0400
From:	Andy Gospodarek <gospo@...ulusnetworks.com>
To:	Or Gerlitz <ogerlitz@...lanox.com>
Cc:	Saeed Mahameed <saeedm@...lanox.com>,
	"David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
	Hadar Hen-Zion <hadarh@...lanox.com>,
	Jiri Pirko <jiri@...lanox.com>,
	Jesse Brandeburg <jesse.brandeburg@...el.com>,
	John Fastabend <john.r.fastabend@...el.com>
Subject: Re: [PATCH net-next 10/16] net/mlx5e: Add devlink based SRIOV mode
 changes (legacy --> offloads)

On Tue, Jun 28, 2016 at 05:25:11PM +0300, Or Gerlitz wrote:
> On 6/28/2016 4:42 PM, Andy Gospodarek wrote:
> >On Mon, Jun 27, 2016 at 07:07:23PM +0300, Saeed Mahameed wrote:
> >>From: Or Gerlitz <ogerlitz@...lanox.com>
> >>
> >>Implement handlers for the devlink commands to get and set the SRIOV
> >>E-Switch mode.
> >>
> >>When turning to the offloads mode, we disable the e-switch and enable
> >>it again in the new mode, create the NIC offloads table and create VF reps.
> >>
> >>When turning to legacy mode, we remove the VF reps and the offloads
> >>table, and re-initiate the e-switch in it's legacy mode.
> >>
> >>The actual creation/removal of the VF reps is done in downstream patches.
> >>
> >>Signed-off-by: Or Gerlitz <ogerlitz@...lanox.com>
> >>Signed-off-by: Saeed Mahameed <saeedm@...lanox.com>
> >>---
> >>  drivers/net/ethernet/mellanox/mlx5/core/eswitch.c  |  12 ++-
> >>  .../ethernet/mellanox/mlx5/core/eswitch_offloads.c | 102 ++++++++++++++++++++-
> >>  2 files changed, 105 insertions(+), 9 deletions(-)
> >>
> >[...]
> >>diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> >>index 3b3afbd..a39af6b 100644
> >>--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> >>+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> >[...]
> >>  int mlx5_devlink_eswitch_mode_set(struct devlink *devlink, u16 mode)
> >>  {
> >>-	return -EOPNOTSUPP;
> >>+	struct mlx5_core_dev *dev;
> >>+	u16 cur_mode;
> >>+
> >>+	dev = devlink_priv(devlink);
> >>+
> >>+	if (!MLX5_CAP_GEN(dev, vport_group_manager))
> >>+		return -EOPNOTSUPP;
> >>+
> >>+	cur_mode = dev->priv.eswitch->mode;
> >>+
> >>+	if (cur_mode == SRIOV_NONE || mode == SRIOV_NONE)
> >>+		return -EOPNOTSUPP;
> >>+
> >>+	if (cur_mode == mode)
> >>+		return 0;
> >>+
> >>+	if (mode == SRIOV_OFFLOADS) /* current mode is legacy */
> >>+		return esw_offloads_start(dev->priv.eswitch);
> >>+	else if (mode == SRIOV_LEGACY) /* curreny mode is offloads */
> >>+		return esw_offloads_stop(dev->priv.eswitch);
> >>+	else
> >>+		return -EINVAL;
> >>  }
> >>
> >>This is an _extremely_ minor nit, but I only bring it up since you are
> >>leading the way here and your model may be one that other people
> >>follow...
> >>
> >>Internally you have a enum to track the SRIOV modes:
> >>
> >>enum {
> >>        SRIOV_NONE,
> >>        SRIOV_LEGACY,
> >>        SRIOV_OFFLOADS
> >>};
> >>
> >>But patch 8 adds a new enum for devlink to track this as well.
> >>
> >>enum devlink_eswitch_mode {
> >>        DEVLINK_ESWITCH_MODE_NONE,
> >>        DEVLINK_ESWITCH_MODE_LEGACY,
> >>        DEVLINK_ESWITCH_MODE_OFFLOADS,
> >>};
> >>
> 
> 
> Andy,
> 
> In mlx5 we're having an eswitch driver instance also when not in sriov mode
> where on that case the mlx5 eswitch mode is called sriov_none, which is
> maybe not a very successful name, I'll look on that.
> 
> On the devlink/system level, the eswitch modes are relevant only for SRIOV,
> you can see in the mlx5 set function that we return error when in the none
> mode or asked to go there.
> 
> So... with your comment,  I realize now that I forgot to remove
> DEVLINK_ESWITCH_MODE_NONE value from the submission.
> 
> >Would it make sense at some point to use the devlink modes in the driver
> >so it's less to track?
> 
> This makes it a bit problematic for mlx5 to use the DEVLINK_ESWITCH_MODE_YYY
> values internally.

If you planned to remove DEVLINK_ESWITCH_MODE_NONE then I could see how
using these in mlx5 would be problematic.  Thinking about it for just a
minute, I can see the value dropping DEVLINK_ESWITCH_MODE_NONE.  If the
driver supports the ability to set the eswitch mode, then it should
report an actual mode other than none.

If you remove DEVLINK_ESWITCH_MODE_NONE, then obviously
mlx5_devlink_eswitch_mode_set/get will need to change a bit as well as
there will need to be a mapping between the two values since the enums
would no longer be the same.  Easy fix, though.  :-)

> >Again, this is an extremely _minor_ concern.  The rest of the set looks
> >great and I like the architectural decisions made here.  Awesome work
> >all around!