Netdev Archive on lore.kernel.org
 help / color / Atom feed
From: Jiri Pirko <jiri@resnulli.us>
To: Jacob Keller <jacob.e.keller@intel.com>
Cc: netdev@vger.kernel.org, Jakub Kicinski <kuba@kernel.org>
Subject: Re: [net-next v2 00/11] implement DEVLINK_CMD_REGION_NEW
Date: Thu, 26 Mar 2020 08:23:07 +0100
Message-ID: <20200326072307.GG11304@nanopsycho.orion> (raw)
In-Reply-To: <20200326035157.2211090-1-jacob.e.keller@intel.com>

Thu, Mar 26, 2020 at 04:51:46AM CET, jacob.e.keller@intel.com wrote:
>This is a second revision of the previous series to implement the
>DEVLINK_CMD_REGION_NEW. The series can be viewed on lore.kernel.org at
>
>https://lore.kernel.org/netdev/20200324223445.2077900-1-jacob.e.keller@intel.com/
>
>This version includes the suggested cleanups from Jakub and Jiri on the
>list, including the following changes, broken out by the v1 patch title.

Jakob. You are missing the cover letter.

You need to include the cover letter in the same way as for v1. The only
thing that changes is that you add a changelog. Or you can just have
changelog per patch.

Please, keep the changelog complete. For v1 you had changelog from RFC.
Keep it.


I suggest to repost to include the cover letter.


>
>Changes to patches since v1:
>
> * devlink: prepare to support region operations
>
>   No changes
>
> * devlink: convert snapshot destructor callback to region op
>
>   No changes
>
> * devlink: trivial: fix tab in function documentation
>
>   No changes
>
> * devlink: add function to take snapshot while locked
>
>   Added Jakub's Reviewed-by tag.
>
> * <NEW> devlink: use -ENOSPC to indicate no more room for snapshots
>
>   New patch added to convert confusing -ENOMEM to -ENOSPC, as suggested by
>   Jiri.
>
> * devlink: extract snapshot id allocation to helper function
>
>   No changes
>
> * devlink: convert snapshot id getter to return an error
>
>   Changed title to "devlink: report error once U32_MAX snapshot ids have
>   been used".
>
>   Refactored this patch to make devlink_region_snapshot_id_get take a
>   pointer to u32, so that the error value and id value are separated. This
>   means that we can remove the INT_MAX limitation on id values.
>
> * devlink: track snapshot id usage count using an xarray
>
>   Fixed the xa_init to use xa_init_flags with XA_FLAGS_ALLOC, so that
>   xa_alloc can properly be used.
>
>   Changed devlink_region_snapshot_id_get to use an initial count of 1
>   instead of 0. Added a new devlink_region_snapshot_id_put function, used
>   to release this initial count. This closes the race condition and issues
>   caused if the driver either doesn't create a snapshot, or if userspace
>   deletes the first snapshot before others are created.
>
>   Used WARN_ON in a few more checks that should not occur, such as if the
>   xarray entry is not a value, or when the id isn't yet in the xarray.
>
>   Removed an unnecessary if (err) { return err; } construction.
>
>   Use xa_limit_32b instead of xa_limit_31b now that we don't return the
>   snapshot id directly.
>
>   Cleanup the label used in __devlink_region_snapshot_create to indicate the
>   failure cause, rather than the cleanup step.
>
>   Removed the unnecessary locking around xa_destroy
>
> * devlink: implement DEVLINK_CMD_REGION_NEW
>
>   Added a WARN_ON to the check in snapshot_id_insert in case the id already
>   exists.
>
>   Removed an unnecessary "if (err) { return err; }" construction
>
>   Use -ENOSPC instead of -ENOMEM when max_snapshots is reached.
>
>   Cleanup label names to match style of the other labels in the file,
>   naming after the failure cause rather than the cleanup step. Also fix a
>   bug in the label ordering.
>
>   Call the new devlink_region_snapshot_id_put function in the mlx4 and
>   netdevsim drivers.
>
> * netdevsim: support taking immediate snapshot via devlink
>
>   Create a local devlink pointer instead of calling priv_to_devlink
>   multiple times.
>
>   Removed previous selftest for devlink region new without a snapshot id,
>   as this is no longer supported. Adjusted and verified that the tests pass
>   now.
>
> * ice: add a devlink region for dumping NVM contents
>
>   Use "dev_err" instead of "dev_warn" for a message about failure to create
>   the devlink region.
>
>Jacob Keller (11):
>  devlink: prepare to support region operations
>  devlink: convert snapshot destructor callback to region op
>  devlink: trivial: fix tab in function documentation
>  devlink: add function to take snapshot while locked
>  devlink: use -ENOSPC to indicate no more room for snapshots
>  devlink: extract snapshot id allocation to helper function
>  devlink: report error once U32_MAX snapshot ids have been used
>  devlink: track snapshot id usage count using an xarray
>  devlink: implement DEVLINK_CMD_REGION_NEW
>  netdevsim: support taking immediate snapshot via devlink
>  ice: add a devlink region for dumping NVM contents
>
> .../networking/devlink/devlink-region.rst     |   8 +
> Documentation/networking/devlink/ice.rst      |  26 ++
> drivers/net/ethernet/intel/ice/ice.h          |   2 +
> drivers/net/ethernet/intel/ice/ice_devlink.c  |  99 +++++
> drivers/net/ethernet/intel/ice/ice_devlink.h  |   3 +
> drivers/net/ethernet/intel/ice/ice_main.c     |   4 +
> drivers/net/ethernet/mellanox/mlx4/crdump.c   |  36 +-
> drivers/net/netdevsim/dev.c                   |  46 ++-
> include/net/devlink.h                         |  33 +-
> net/core/devlink.c                            | 363 +++++++++++++++---
> .../drivers/net/netdevsim/devlink.sh          |  10 +
> 11 files changed, 550 insertions(+), 80 deletions(-)
>
>-- 
>2.24.1
>

  parent reply index

Thread overview: 33+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-26  3:51 Jacob Keller
2020-03-26  3:51 ` [net-next v2 01/11] devlink: prepare to support region operations Jacob Keller
2020-03-26  3:51 ` [net-next v2 02/11] devlink: convert snapshot destructor callback to region op Jacob Keller
2020-03-26  3:51 ` [net-next v2 03/11] devlink: trivial: fix tab in function documentation Jacob Keller
2020-03-26  3:51 ` [net-next v2 04/11] devlink: add function to take snapshot while locked Jacob Keller
2020-03-26  3:51 ` [net-next v2 05/11] devlink: use -ENOSPC to indicate no more room for snapshots Jacob Keller
2020-03-26  7:24   ` Jiri Pirko
2020-03-26  3:51 ` [net-next v2 06/11] devlink: extract snapshot id allocation to helper function Jacob Keller
2020-03-26  7:25   ` Jiri Pirko
2020-03-26  3:51 ` [net-next v2 07/11] devlink: report error once U32_MAX snapshot ids have been used Jacob Keller
2020-03-26  7:30   ` Jiri Pirko
2020-03-26 16:09     ` Keller, Jacob E
2020-03-26  3:51 ` [net-next v2 08/11] devlink: track snapshot id usage count using an xarray Jacob Keller
2020-03-26  8:35   ` Jiri Pirko
2020-03-26  3:51 ` [net-next v2 09/11] devlink: implement DEVLINK_CMD_REGION_NEW Jacob Keller
2020-03-26  8:51   ` Jiri Pirko
2020-03-26  8:52   ` Jiri Pirko
2020-03-26 16:19     ` Jacob Keller
2020-03-26 21:10       ` Jiri Pirko
2020-03-26  3:51 ` [net-next v2 10/11] netdevsim: support taking immediate snapshot via devlink Jacob Keller
2020-03-26  9:00   ` Jiri Pirko
2020-03-26  3:51 ` [net-next v2 11/11] ice: add a devlink region for dumping NVM contents Jacob Keller
2020-03-26  9:02   ` Jiri Pirko
2020-03-26 16:23     ` Jacob Keller
2020-03-26 21:11       ` Jiri Pirko
2020-03-26  7:23 ` Jiri Pirko [this message]
2020-03-26  7:51 ` [net-next v2 00/11] implement DEVLINK_CMD_REGION_NEW Jiri Pirko
2020-03-26 16:15   ` Jacob Keller
2020-03-26 21:12     ` Jiri Pirko
2020-03-26  8:36 ` Jiri Pirko
2020-03-26  8:54 ` Jiri Pirko
2020-03-26 18:27 ` David Miller
2020-03-26 18:29   ` Jacob Keller

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=20200326072307.GG11304@nanopsycho.orion \
    --to=jiri@resnulli.us \
    --cc=jacob.e.keller@intel.com \
    --cc=kuba@kernel.org \
    --cc=netdev@vger.kernel.org \
    /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

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org
	public-inbox-index netdev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git