RE: [PATCH rdma-next 4/8] IB/core: Skip device which doesn't have necessary capabilities
> From: Jason Gunthorpe
> Sent: Thursday, April 8, 2021 5:46 PM
> On Wed, Apr 07, 2021 at 03:44:35PM +0000, Parav Pandit wrote:
>
> > > If it returns EOPNOTUPP then the remove is never called so if it
> > > allocated memory and left it allocated then it is leaking memory.
> > >
> > I probably confused you. There is no leak today because add_one
> > allocates memory, and later on when SA/CM etc per port cap is not
> > present, it is unused left there which is freed on remove_one().
> > Returning EOPNOTUPP is fine at start of add_one() before allocation.
>
> Most of ULPs are OK, eg umad does:
>
> umad_dev = kzalloc(struct_size(umad_dev, ports, e - s + 1),
> GFP_KERNEL);
> if (!umad_dev)
> return -ENOMEM;
> for (i = s; i <= e; ++i) {
> if (!rdma_cap_ib_mad(device, i))
> continue;
>
> if (!count) {
> ret = -EOPNOTSUPP;
> goto free;
> free:
> /* balances kref_init */
> ib_umad_dev_put(umad_dev);
>
> It looks like only cm.c and cma.c need fixing, just fix those two.
Only cma.c needs a fixing. cm.c also reports EOPNOTSUPP.
I will send the simplified fix through Leon.
RE: [PATCH rdma-next 4/8] IB/core: Skip device which doesn't have necessary capabilities
> From: Jason Gunthorpe
> Sent: Wednesday, April 7, 2021 8:44 PM
>
> On Wed, Apr 07, 2021 at 03:06:35PM +, Parav Pandit wrote:
> >
> >
> > > From: Jason Gunthorpe
> > > Sent: Tuesday, April 6, 2021 9:17 PM
> > >
> > > On Mon, Apr 05, 2021 at 08:49:56AM +0300, Leon Romanovsky wrote:
> > > > @@ -2293,6 +2295,17 @@ static void ib_sa_event(struct
> > > > ib_event_handler
> > > *handler,
> > > > }
> > > > }
> > > >
> > > > +static bool ib_sa_client_supported(struct ib_device *device) {
> > > > + unsigned int i;
> > > > +
> > > > + rdma_for_each_port(device, i) {
> > > > + if (rdma_cap_ib_sa(device, i))
> > > > + return true;
> > > > + }
> > > > + return false;
> > > > +}
> > >
> > > This is already done though:
>
> > It is but, ib_sa_device() allocates ib_sa_device worth of struct for
> > each port without checking the rdma_cap_ib_sa(). This results into
> > allocating 40 * 512 = 20480 rounded of to power of 2 to 32K bytes of
> > memory for the rdma device with 512 ports. Other modules are also
> > similarly wasting such memory.
>
> If it returns EOPNOTUPP then the remove is never called so if it allocated
> memory and left it allocated then it is leaking memory.
>
I probably confused you. There is no leak today because add_one allocates
memory, and later on when SA/CM etc per port cap is not present, it is unused
left there which is freed on remove_one().
Returning EOPNOTUPP is fine at start of add_one() before allocation.
> If you are saying 32k bytes of temporary allocation matters during device
> startup then it needs benchmarks and a use case.
>
Use case is clear and explained in commit logs, i.e. to not allocate the memory
which is never used.
> > > The add_one function should return -EOPNOTSUPP if it doesn't want to
> > > run on this device and any supported checks should just be at the
> > > front - this is how things work right now
>
> > I am ok to fold this check at the beginning of add callback. When
> > 512 to 1K RoCE devices are used, they do not have SA, CM, CMA etc caps
> > on and all the client needs to go through refcnt + xa + sem and unroll
> > them. Is_supported() routine helps to cut down all of it. I didn't
> > calculate the usec saved with it.
>
> If that is the reason then explain in the cover letter and provide benchmarks
I doubt it will be significant but I will do a benchmark.
RE: [PATCH rdma-next 4/8] IB/core: Skip device which doesn't have necessary capabilities
> From: Jason Gunthorpe
> Sent: Tuesday, April 6, 2021 9:17 PM
>
> On Mon, Apr 05, 2021 at 08:49:56AM +0300, Leon Romanovsky wrote:
> > @@ -2293,6 +2295,17 @@ static void ib_sa_event(struct ib_event_handler
> *handler,
> > }
> > }
> >
> > +static bool ib_sa_client_supported(struct ib_device *device) {
> > + unsigned int i;
> > +
> > + rdma_for_each_port(device, i) {
> > + if (rdma_cap_ib_sa(device, i))
> > + return true;
> > + }
> > + return false;
> > +}
>
> This is already done though:
It is but, ib_sa_device() allocates ib_sa_device worth of struct for each port
without checking the rdma_cap_ib_sa().
This results into allocating 40 * 512 = 20480 rounded of to power of 2 to 32K
bytes of memory for the rdma device with 512 ports.
Other modules are also similarly wasting such memory.
>
> for (i = 0; i <= e - s; ++i) {
> spin_lock_init(_dev->port[i].ah_lock);
> if (!rdma_cap_ib_sa(device, i + 1))
> continue;
> [..]
>
> if (!count) {
> ret = -EOPNOTSUPP;
> goto free;
>
> Why does it need to be duplicated? The other patches are all basically like
> that too.
>
> The add_one function should return -EOPNOTSUPP if it doesn't want to run
> on this device and any supported checks should just be at the front - this is
> how things work right now
>
I am ok to fold this check at the beginning of add callback.
When 512 to 1K RoCE devices are used, they do not have SA, CM, CMA etc caps on
and all the client needs to go through refcnt + xa + sem and unroll them.
Is_supported() routine helps to cut down all of it. I didn't calculate the usec
saved with it.
Please let me know.
RE: [PATCH] net/mlx5: remove unneeded semicolon
Hi Saeed, > From: Parav Pandit > Sent: Monday, February 22, 2021 3:32 PM > > > > From: Jiapeng Chong > > Sent: Monday, February 22, 2021 3:27 PM > > > > Fix the following coccicheck warnings: > > > > ./drivers/net/ethernet/mellanox/mlx5/core/sf/devlink.c:495:2-3: > > Unneeded semicolon. > > > > Reported-by: Abaci Robot > > Signed-off-by: Jiapeng Chong > > --- > > drivers/net/ethernet/mellanox/mlx5/core/sf/devlink.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/sf/devlink.c > > b/drivers/net/ethernet/mellanox/mlx5/core/sf/devlink.c > > index c2ba41b..60a6328 100644 > > --- a/drivers/net/ethernet/mellanox/mlx5/core/sf/devlink.c > > +++ b/drivers/net/ethernet/mellanox/mlx5/core/sf/devlink.c > > @@ -492,7 +492,7 @@ static int mlx5_sf_esw_event(struct notifier_block > > *nb, unsigned long event, voi > > break; > > default: > > break; > > - }; > > + } > > > > return 0; > > } > > -- > > 1.8.3.1 > > Reviewed-by: Parav Pandit Will you take this patch [1] to your queue? [1] https://lore.kernel.org/linux-rdma/1613987819-43161-1-git-send-email-jiapeng.ch...@linux.alibaba.com/
RE: [PATCH] net/mlx5: remove unneeded semicolon
> From: Jiapeng Chong > Sent: Monday, February 22, 2021 3:27 PM > > Fix the following coccicheck warnings: > > ./drivers/net/ethernet/mellanox/mlx5/core/sf/devlink.c:495:2-3: Unneeded > semicolon. > > Reported-by: Abaci Robot > Signed-off-by: Jiapeng Chong > --- > drivers/net/ethernet/mellanox/mlx5/core/sf/devlink.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/sf/devlink.c > b/drivers/net/ethernet/mellanox/mlx5/core/sf/devlink.c > index c2ba41b..60a6328 100644 > --- a/drivers/net/ethernet/mellanox/mlx5/core/sf/devlink.c > +++ b/drivers/net/ethernet/mellanox/mlx5/core/sf/devlink.c > @@ -492,7 +492,7 @@ static int mlx5_sf_esw_event(struct notifier_block > *nb, unsigned long event, voi > break; > default: > break; > - }; > + } > > return 0; > } > -- > 1.8.3.1 Reviewed-by: Parav Pandit
RE: [PATCH] net/mlx5: docs: correct section reference in table of contents
> From: Lukas Bulwahn
> Sent: Friday, February 5, 2021 3:25 PM
>
> Commit 142d93d12dc1 ("net/mlx5: Add devlink subfunction port
> documentation") refers to a section 'mlx5 port function' in the table of
> contents, but includes a section 'mlx5 function attributes' instead.
>
> Hence, make htmldocs warns:
>
> mlx5.rst:16: WARNING: Unknown target name: "mlx5 port function".
>
> Correct the section reference in table of contents to the actual name of
> section in the documentation.
>
> Also, tune another section underline while visiting this document.
>
> Signed-off-by: Lukas Bulwahn
> ---
> Saeed, please pick this patch for your -next tree on top of the commit above.
>
> .../networking/device_drivers/ethernet/mellanox/mlx5.rst | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git
> a/Documentation/networking/device_drivers/ethernet/mellanox/mlx5.rst
> b/Documentation/networking/device_drivers/ethernet/mellanox/mlx5.rst
> index a1b32fcd0d76..1b7e32d8a61b 100644
> --- a/Documentation/networking/device_drivers/ethernet/mellanox/mlx5.rst
> +++
> b/Documentation/networking/device_drivers/ethernet/mellanox/mlx5.rst
> @@ -13,12 +13,12 @@ Contents
> - `Devlink info`_
> - `Devlink parameters`_
> - `mlx5 subfunction`_
> -- `mlx5 port function`_
> +- `mlx5 function attributes`_
> - `Devlink health reporters`_
> - `mlx5 tracepoints`_
>
> Enabling the driver and kconfig options -
>
> +===
>
> | mlx5 core is modular and most of the major mlx5 core driver features can
> be selected (compiled in/out) | at build time via kernel Kconfig flags.
> --
> 2.17.1
Thanks.
Reviewed-by: Parav Pandit
RE: [resend/standalone PATCH v4] Add auxiliary bus support
> From: Leon Romanovsky > Sent: Friday, December 4, 2020 6:02 PM > > On Fri, Dec 04, 2020 at 12:42:46PM +0100, Greg KH wrote: > > On Wed, Dec 02, 2020 at 04:54:24PM -0800, Dan Williams wrote: > > > From: Dave Ertman > > > > > > Add support for the Auxiliary Bus, auxiliary_device and auxiliary_driver. > > > It enables drivers to create an auxiliary_device and bind an > > > auxiliary_driver to it. > > > > > > The bus supports probe/remove shutdown and suspend/resume > callbacks. > > > Each auxiliary_device has a unique string based id; driver binds to > > > an auxiliary_device based on this id through the bus. > > > > > > Co-developed-by: Kiran Patil > > > Co-developed-by: Ranjani Sridharan > > > > > > Co-developed-by: Fred Oh > > > Co-developed-by: Leon Romanovsky > > > Signed-off-by: Kiran Patil > > > Signed-off-by: Ranjani Sridharan > > > Signed-off-by: Fred Oh > > > Signed-off-by: Leon Romanovsky > > > Signed-off-by: Dave Ertman > > > Reviewed-by: Pierre-Louis Bossart > > > > > > Reviewed-by: Shiraz Saleem > > > Reviewed-by: Parav Pandit > > > Reviewed-by: Dan Williams > > > Reviewed-by: Martin Habets > > > Link: > > > https://lore.kernel.org/r/20201113161859.1775473-2-david.m.ertman@in > > > tel.com > > > Signed-off-by: Dan Williams > > > --- > > > This patch is "To:" the maintainers that have a pending backlog of > > > driver updates dependent on this facility, and "Cc:" Greg. Greg, I > > > understand you have asked for more time to fully review this and > > > apply it to driver-core.git, likely for v5.12, but please consider > > > Acking it for v5.11 instead. It looks good to me and several other > stakeholders. > > > Namely, stakeholders that have pressure building up behind this > > > facility in particular Mellanox RDMA, but also SOF, Intel Ethernet, > > > and later on Compute Express Link. > > > > > > I will take the blame for the 2 months of silence that made this > > > awkward to take through driver-core.git, but at the same time I do > > > not want to see that communication mistake inconvenience other > > > parties that reasonably thought this was shaping up to land in v5.11. > > > > > > I am willing to host this version at: > > > > > > git://git.kernel.org/pub/scm/linux/kernel/git/djbw/linux > > > tags/auxiliary-bus-for-5.11 > > > > > > ...for all the independent drivers to have a common commit baseline. > > > It is not there yet pending Greg's Ack. > > > > > > For example implementations incorporating this patch, see Dave > > > Ertman's SOF series: > > > > > > https://lore.kernel.org/r/20201113161859.1775473-2-david.m.ertman@in > > > tel.com > > > > > > ...and Leon's mlx5 series: > > > > > > http://lore.kernel.org/r/20201026111849.1035786-1-l...@kernel.org > > > > > > PS: Greg I know I promised some review on newcomer patches to help > > > with your queue, unfortunately Intel-internal review is keeping my > > > plate full. Again, I do not want other stakeholder to be waiting on > > > me to resolve that backlog. > > > > Ok, I spent some hours today playing around with this. I wrote up a > > small test-patch for this (how did anyone test this thing???). > > We are running all verifications tests that we have over our > mlx5 driver. It includes devices reloads, power failures, FW reconfiguration > to > emulate different devices with and without error injections and many more. > Up till now, no new bugs that are not known to us were found. > Subfunction patchset [1] that is using auxiliary bus in mlx5 driver is also been used by verification and performance tests. [1] https://lore.kernel.org/linux-rdma/20201112192424.2742-1-pa...@nvidia.com/
RE: [PATCH v3 01/10] Add auxiliary bus support
> From: Dan Williams > Sent: Friday, November 6, 2020 1:56 AM > > On Thu, Nov 5, 2020 at 11:40 AM Parav Pandit wrote: > > > > > > > > > From: Ertman, David M > > > Sent: Friday, November 6, 2020 12:58 AM > > > Subject: RE: [PATCH v3 01/10] Add auxiliary bus support > > > > > > > -Original Message- > > > > From: Dan Williams > > > > Sent: Thursday, November 5, 2020 1:19 AM > > > > > > > > [..] > > > > > + > > > > > +Another use case is for the PCI device to be split out into > > > > > +multiple sub functions. For each sub function an > > > > > +auxiliary_device will be created. A PCI sub function driver > > > > > +will bind to such devices that will create its own one or more > > > > > +class devices. A PCI sub function auxiliary device will likely > > > > > +be contained in a struct with additional attributes such as > > > > > +user defined sub function number and optional attributes such > > > > > +as resources and a link to > > > > the > > > > > +parent device. These attributes could be used by systemd/udev; > > > > > +and > > > > hence should > > > > > +be initialized before a driver binds to an auxiliary_device. > > > > > > > > This does not read like an explicit example like the previous 2. > > > > Did you have something specific in mind? > > > > > > > > > > This was added by request of Parav. > > > > > This example describes the mlx5 PCI subfunction use case. > > I didn't follow your question about 'explicit example'. > > What part is missing to identify it as explicit example? > > Specifically listing "mlx5" so if someone reading this document thinks to > themselves "hey mlx5 sounds like my use case" they can go grep for that. Ah, I see. "mlx5" is not listed explicitly, because it is not included in this patchset. In various previous discussions in this thread, mlx5 subfunction use case is described that justifies the existence of the bus. I will be happy to update this documentation once mlx5 subfunction will be part of kernel so that grep actually shows valid output. (waiting to post them as it uses auxiliary bus :-)).
RE: [PATCH v3 01/10] Add auxiliary bus support
> From: Ertman, David M > Sent: Friday, November 6, 2020 12:58 AM > Subject: RE: [PATCH v3 01/10] Add auxiliary bus support > > > -Original Message- > > From: Dan Williams > > Sent: Thursday, November 5, 2020 1:19 AM > > [..] > > > + > > > +Another use case is for the PCI device to be split out into > > > +multiple sub functions. For each sub function an auxiliary_device > > > +will be created. A PCI sub function driver will bind to such > > > +devices that will create its own one or more class devices. A PCI > > > +sub function auxiliary device will likely be contained in a struct > > > +with additional attributes such as user defined sub function number > > > +and optional attributes such as resources and a link to > > the > > > +parent device. These attributes could be used by systemd/udev; and > > hence should > > > +be initialized before a driver binds to an auxiliary_device. > > > > This does not read like an explicit example like the previous 2. Did > > you have something specific in mind? > > > > This was added by request of Parav. > This example describes the mlx5 PCI subfunction use case. I didn't follow your question about 'explicit example'. What part is missing to identify it as explicit example?
RE: [PATCH mlx5-next v1 03/11] net/mlx5_core: Clean driver version and name
iver_ver_sz - strlen(string));
> - strncat(string, DRIVER_NAME, remaining_size);
> + strncat(string, KBUILD_MODNAME, remaining_size);
>
> remaining_size = max_t(int, 0, driver_ver_sz - strlen(string));
> strncat(string, ",", remaining_size);
> @@ -313,7 +312,7 @@ static int request_bar(struct pci_dev *pdev)
> return -ENODEV;
> }
>
> - err = pci_request_regions(pdev, DRIVER_NAME);
> + err = pci_request_regions(pdev, KBUILD_MODNAME);
> if (err)
> dev_err(>dev, "Couldn't get PCI resources,
> aborting\n");
>
> @@ -1617,7 +1616,7 @@ void mlx5_recover_device(struct mlx5_core_dev
> *dev) }
>
> static struct pci_driver mlx5_core_driver = {
> - .name = DRIVER_NAME,
> + .name = KBUILD_MODNAME,
> .id_table = mlx5_core_pci_table,
> .probe = init_one,
> .remove = remove_one,
> @@ -1643,6 +1642,10 @@ static int __init init(void) {
> int err;
>
> + WARN_ONCE(strcmp(MLX5_ADEV_NAME, KBUILD_MODNAME) ||
> + strlen(MLX5_ADEV_NAME) != strlen(KBUILD_MODNAME),
> + "mlx5_core name not in sync with kernel module name");
> +
In which case, both the strings are same but their length not?
You likely don't need the string length check.
> get_random_bytes(_owner_id, sizeof(sw_owner_id));
>
> mlx5_core_verify_params();
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/mlx5_core.h
> b/drivers/net/ethernet/mellanox/mlx5/core/mlx5_core.h
> index 8cec85ab419d..b285f1515e4e 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/mlx5_core.h
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/mlx5_core.h
> @@ -42,9 +42,6 @@
> #include
> #include
>
> -#define DRIVER_NAME "mlx5_core"
> -#define DRIVER_VERSION "5.0-0"
> -
> extern uint mlx5_core_debug_mask;
>
> #define mlx5_core_dbg(__dev, format, ...)\
> diff --git a/include/linux/mlx5/driver.h b/include/linux/mlx5/driver.h index
> 317257f8e0ad..ed1d030658d2 100644
> --- a/include/linux/mlx5/driver.h
> +++ b/include/linux/mlx5/driver.h
> @@ -56,6 +56,8 @@
> #include
> #include
>
> +#define MLX5_ADEV_NAME "mlx5_core"
> +
> enum {
> MLX5_BOARD_ID_LEN = 64,
> };
> --
> 2.28.0
Other than strlen removal check,
Reviewed-by: Parav Pandit
RE: WARNING in dma_map_page_attrs
> From: h...@lst.de > Sent: Wednesday, October 28, 2020 11:01 PM > > On Tue, Oct 27, 2020 at 12:52:30PM +, Parav Pandit wrote: > > > > > From: h...@lst.de > > > Sent: Tuesday, October 27, 2020 1:41 PM > > > > > > On Mon, Oct 26, 2020 at 05:23:48AM +, Parav Pandit wrote: > > > > Hi Christoph, > > > > > > > > > From: Jakub Kicinski > > > > > Sent: Saturday, October 24, 2020 11:45 PM > > > > > > > > > > CC: rdma, looks like rdma from the stack trace > > > > > > > > > > On Fri, 23 Oct 2020 20:07:17 -0700 syzbot wrote: > > > > > > syzbot has found a reproducer for the following issue on: > > > > > > > > > > > > HEAD commit:3cb12d27 Merge tag 'net-5.10-rc1' of > > > git://git.kernel.org/.. > > > > > > > > In [1] you mentioned that dma_mask should not be set for > dma_virt_ops. > > > > So patch [2] removed it. > > > > > > > > But check to validate the dma mask for all dma_ops was added in [3]. > > > > > > > > What is the right way? Did I misunderstood your comment about > > > dma_mask in [1]? > > > > > > No, I did not say we don't need the mask. I said copying over the > > > various dma-related fields from the parent is bogus. > > > > > > I think rxe (and ther other drivers/infiniband/sw drivers) need a > > > simple dma_coerce_mask_and_coherent and nothing else. > > > > I see. Does below fix make sense? > > Is DMA_MASK_NONE correct? > > DMA_MASK_NONE is gone in 5.10. I think you want DMA_BIT_MASK(64). > That isn't actually correct for 32-bit platforms, but good enough. Ok. thanks for the input. Sending updated fix to set 64-bit mask for 64-bit platform and 32-bit mask otherwise.
RE: WARNING in dma_map_page_attrs
> From: h...@lst.de
> Sent: Tuesday, October 27, 2020 1:41 PM
>
> On Mon, Oct 26, 2020 at 05:23:48AM +, Parav Pandit wrote:
> > Hi Christoph,
> >
> > > From: Jakub Kicinski
> > > Sent: Saturday, October 24, 2020 11:45 PM
> > >
> > > CC: rdma, looks like rdma from the stack trace
> > >
> > > On Fri, 23 Oct 2020 20:07:17 -0700 syzbot wrote:
> > > > syzbot has found a reproducer for the following issue on:
> > > >
> > > > HEAD commit:3cb12d27 Merge tag 'net-5.10-rc1' of
> git://git.kernel.org/..
> >
> > In [1] you mentioned that dma_mask should not be set for dma_virt_ops.
> > So patch [2] removed it.
> >
> > But check to validate the dma mask for all dma_ops was added in [3].
> >
> > What is the right way? Did I misunderstood your comment about
> dma_mask in [1]?
>
> No, I did not say we don't need the mask. I said copying over the various
> dma-related fields from the parent is bogus.
>
> I think rxe (and ther other drivers/infiniband/sw drivers) need a simple
> dma_coerce_mask_and_coherent and nothing else.
I see. Does below fix make sense?
Is DMA_MASK_NONE correct?
>From cfad78c35788b4ff604abedd96559500c5fd2a72 Mon Sep 17 00:00:00 2001
From: Parav Pandit
Date: Tue, 27 Oct 2020 14:20:07 +0200
Subject: [PATCH] RDMA: Fix software RDMA drivers for dma mapping error
A cited commit in fixes tag avoided setting dma_mask of the ib_device.
Commit [1] made dma_mask as mandetory field to be setup even for
dma_virt_ops based dma devices.
Fix it by setting empty DMA MASK for software based RDMA devices.
[1] commit: f959dcd6ddfd2 ("dma-direct: Fix potential NULL pointer dereference")
Reported-by: syzbot+34dc2fea3478e659a...@syzkaller.appspotmail.com
Fixes: e0477b34d9d1 ("RDMA: Explicitly pass in the dma_device to
ib_register_device")
Signed-off-by: Parav Pandit
---
drivers/infiniband/sw/rdmavt/vt.c | 5 +++--
drivers/infiniband/sw/rxe/rxe_verbs.c | 4 +++-
drivers/infiniband/sw/siw/siw_main.c | 5 +++--
3 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/drivers/infiniband/sw/rdmavt/vt.c
b/drivers/infiniband/sw/rdmavt/vt.c
index 52218684ad4a..1b456f4d4fcf 100644
--- a/drivers/infiniband/sw/rdmavt/vt.c
+++ b/drivers/infiniband/sw/rdmavt/vt.c
@@ -580,8 +580,9 @@ int rvt_register_device(struct rvt_dev_info *rdi)
/* DMA Operations */
rdi->ibdev.dev.dma_parms = rdi->ibdev.dev.parent->dma_parms;
- dma_set_coherent_mask(>ibdev.dev,
- rdi->ibdev.dev.parent->coherent_dma_mask);
+ ret = dma_coerce_mask_and_coherent(>ibdev.dev, DMA_MASK_NONE);
+ if (ret)
+ goto bail_wss;
/* Protection Domain */
spin_lock_init(>n_pds_lock);
diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.c
b/drivers/infiniband/sw/rxe/rxe_verbs.c
index 1fc022362fbe..357787688293 100644
--- a/drivers/infiniband/sw/rxe/rxe_verbs.c
+++ b/drivers/infiniband/sw/rxe/rxe_verbs.c
@@ -1130,7 +1130,9 @@ int rxe_register_device(struct rxe_dev *rxe, const char
*ibdev_name)
rxe->ndev->dev_addr);
dev->dev.dma_parms = >dma_parms;
dma_set_max_seg_size(>dev, UINT_MAX);
- dma_set_coherent_mask(>dev, dma_get_required_mask(>dev));
+ err = dma_coerce_mask_and_coherent(>dev, DMA_MASK_NONE);
+ if (err)
+ return err;
dev->uverbs_cmd_mask = BIT_ULL(IB_USER_VERBS_CMD_GET_CONTEXT)
| BIT_ULL(IB_USER_VERBS_CMD_CREATE_COMP_CHANNEL)
diff --git a/drivers/infiniband/sw/siw/siw_main.c
b/drivers/infiniband/sw/siw/siw_main.c
index ca8bc7296867..d3dc50a42dab 100644
--- a/drivers/infiniband/sw/siw/siw_main.c
+++ b/drivers/infiniband/sw/siw/siw_main.c
@@ -384,8 +384,9 @@ static struct siw_device *siw_device_create(struct
net_device *netdev)
base_dev->dev.parent = parent;
base_dev->dev.dma_parms = >dma_parms;
dma_set_max_seg_size(_dev->dev, UINT_MAX);
- dma_set_coherent_mask(_dev->dev,
- dma_get_required_mask(_dev->dev));
+ if (dma_coerce_mask_and_coherent(_dev->dev, DMA_MASK_NONE))
+ goto error;
+
base_dev->num_comp_vectors = num_possible_cpus();
xa_init_flags(>qp_xa, XA_FLAGS_ALLOC1);
--
2.26.2
RE: WARNING in dma_map_page_attrs
Hi Christoph,
> From: Jakub Kicinski
> Sent: Saturday, October 24, 2020 11:45 PM
>
> CC: rdma, looks like rdma from the stack trace
>
> On Fri, 23 Oct 2020 20:07:17 -0700 syzbot wrote:
> > syzbot has found a reproducer for the following issue on:
> >
> > HEAD commit:3cb12d27 Merge tag 'net-5.10-rc1' of git://git.kernel.org/..
In [1] you mentioned that dma_mask should not be set for dma_virt_ops.
So patch [2] removed it.
But check to validate the dma mask for all dma_ops was added in [3].
What is the right way? Did I misunderstood your comment about dma_mask in [1]?
[1] https://www.spinics.net/lists/linux-rdma/msg96374.html
[2] e0477b34d9d ("RDMA: Explicitly pass in the dma_device to
ib_register_device")
[3] f959dcd6ddfd ("dma-direct: Fix potential NULL pointer dereference")
> > WARNING: CPU: 1 PID: 8488 at kernel/dma/mapping.c:149
> > dma_map_page_attrs+0x493/0x700 kernel/dma/mapping.c:149 Modules
> linked in:
> > dma_map_single_attrs include/linux/dma-mapping.h:279 [inline]
> > ib_dma_map_single include/rdma/ib_verbs.h:3967 [inline]
> > ib_mad_post_receive_mads+0x23f/0xd60
> > drivers/infiniband/core/mad.c:2715
> > ib_mad_port_start drivers/infiniband/core/mad.c:2862 [inline]
> > ib_mad_port_open drivers/infiniband/core/mad.c:3016 [inline]
> > ib_mad_init_device+0x72b/0x1400 drivers/infiniband/core/mad.c:3092
> > add_client_context+0x405/0x5e0 drivers/infiniband/core/device.c:680
> > enable_device_and_get+0x1d5/0x3c0
> > drivers/infiniband/core/device.c:1301
> > ib_register_device drivers/infiniband/core/device.c:1376 [inline]
> > ib_register_device+0x7a7/0xa40 drivers/infiniband/core/device.c:1335
> > rxe_register_device+0x46d/0x570
> > drivers/infiniband/sw/rxe/rxe_verbs.c:1182
> > rxe_add+0x12fe/0x16d0 drivers/infiniband/sw/rxe/rxe.c:247
> > rxe_net_add+0x8c/0xe0 drivers/infiniband/sw/rxe/rxe_net.c:507
> > rxe_newlink drivers/infiniband/sw/rxe/rxe.c:269 [inline]
> > rxe_newlink+0xb7/0xe0 drivers/infiniband/sw/rxe/rxe.c:250
> > nldev_newlink+0x30e/0x540 drivers/infiniband/core/nldev.c:1555
> > rdma_nl_rcv_msg+0x367/0x690 drivers/infiniband/core/netlink.c:195
> > rdma_nl_rcv_skb drivers/infiniband/core/netlink.c:239 [inline]
> > rdma_nl_rcv+0x2f2/0x440 drivers/infiniband/core/netlink.c:259
> > netlink_unicast_kernel net/netlink/af_netlink.c:1304 [inline]
> > netlink_unicast+0x533/0x7d0 net/netlink/af_netlink.c:1330
> > netlink_sendmsg+0x856/0xd90 net/netlink/af_netlink.c:1919
> > sock_sendmsg_nosec net/socket.c:651 [inline]
> > sock_sendmsg+0xcf/0x120 net/socket.c:671
> > sys_sendmsg+0x6e8/0x810 net/socket.c:2353
> > ___sys_sendmsg+0xf3/0x170 net/socket.c:2407
> > __sys_sendmsg+0xe5/0x1b0 net/socket.c:2440
> > do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46
> > entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > RIP: 0033:0x443699
> > Code: 18 89 d0 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 00 48 89 f8 48
> > 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d
> > 01 f0 ff ff 0f 83 db 0d fc ff c3 66 2e 0f 1f 84 00 00 00 00
> > RSP: 002b:7ffc067db418 EFLAGS: 0246 ORIG_RAX:
> 002e
> > RAX: ffda RBX: 0003 RCX: 00443699
> > RDX: RSI: 22c0 RDI: 0003
> > RBP: 7ffc067db420 R08: 01bb R09: 01bb
> > R10: R11: 0246 R12: 7ffc067db430
> > R13: R14: R15:
> >
RE: [PATCH] RDMA/core: fix return error value to negative
> From: linux-rdma-ow...@vger.kernel.org
> On Behalf Of Li Heng
> Sent: Saturday, July 25, 2020 8:26 AM
>
> Fixes: 8d9ec9addd6c (IB/core: Add a sgid_attr pointer to struct rdma_ah_attr)
> Reported-by: Hulk Robot
> Signed-off-by: Li Heng
> ---
> drivers/infiniband/core/verbs.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
> index 53d6505c..f369f0a 100644
> --- a/drivers/infiniband/core/verbs.c
> +++ b/drivers/infiniband/core/verbs.c
> @@ -1712,7 +1712,7 @@ static int _ib_modify_qp(struct ib_qp *qp, struct
> ib_qp_attr *attr,
> if (!(rdma_protocol_ib(qp->device,
> attr->alt_ah_attr.port_num) &&
> rdma_protocol_ib(qp->device, port))) {
> - ret = EINVAL;
> + ret = -EINVAL;
> goto out;
> }
> }
> --
> 2.7.4
With below corrected fixes tag,
Fixes: 7a5c938b9ed0 ("IB/core: Check for rdma_protocol_ib only after validating
port_num")
Reviewed-by: Parav Pandit
Re: [REGRESSION] mlx5: Driver remove during hot unplug is broken
On 7/9/2020 3:36 PM, Niklas Schnelle wrote:
>
> On 7/8/20 5:44 PM, Parav Pandit wrote:
> ... snip ..
>>>
>>
>> It is likely because events_cleanup() freed the memory using kvfree() that
>> health recovery context is trying to access in notifier chain.
>>
>> While reviewing I see few more errors as below.
>> (a) mlx5_pci_err_detected() invokes mlx5_drain_health_wq() outside of
>> intf_state_mutex.
>> (b) mlx5_enter_error_state() in commit b6e0b6bebe0 read and updates dev
>> state outside of intf_state_mutex.
>> (c) due to drain_health_wq() introduction in mlx5_pci_close() in commit
>> 42ea9f1b5c625 health_wq is flushed twice.
>> (d) priv->events freed is kvfree() but allocated using kzalloc().
>>
>> This code certainly needs rework.
>>
>> In meantime to avoid this regression, I believe below hunk eliminates error
>> introduced in patch 41798df9bfc.
>> Will you please help test it?
>>
>> Shay and I did the review of below patch.
>> If it works I will get it through Saeed's tree and internal reviews.
>>
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c
>> b/drivers/net/ethernet/mellanox/mlx5/core/main.c
>> index ebec2318dbc4..529df5703737 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
>> @@ -785,11 +785,6 @@ static int mlx5_pci_init(struct mlx5_core_dev *dev,
>> struct pci_dev *pdev,
>>
>> static void mlx5_pci_close(struct mlx5_core_dev *dev)
>> {
>> - /* health work might still be active, and it needs pci bar in
>> -* order to know the NIC state. Therefore, drain the health WQ
>> -* before removing the pci bars
>> -*/
>> - mlx5_drain_health_wq(dev);
>> iounmap(dev->iseg);
>> pci_clear_master(dev->pdev);
>> release_bar(dev->pdev);
>> @@ -1235,6 +1230,7 @@ void mlx5_unload_one(struct mlx5_core_dev *dev, bool
>> cleanup)
>> if (cleanup) {
>> mlx5_unregister_device(dev);
>> mlx5_devlink_unregister(priv_to_devlink(dev));
>> + mlx5_drain_health_wq(dev);
> I think with the above you can remove the mlx5_drain_health_wq(dev) in
> remove_one()
> as that calls mlx5_unload_one() with cleanup == true.
Oh yes, I was supposed to remove from there too.
That was the whole point of removing from there. But I missed. :(
My bad that I didn't give share patch.
Please find the inline patch at the end of the email thread.
> Also I wonder if it is a problem
> that the order of mlx5_devlink_unregister(priv_to_devlink(dev)) and
> mlx5_unregister_device(dev)
> is switched compared to the 5.7 code. That said changing both seems to result
> in a deadlock
> though not the "leak of a command resource":
>
Current order in 5.8-rc4 of unregister_device and devlink_unregister is
the right one. That is, to unregister the netdevice before unregistering
devlink device.
Please do not change the order of mlx5_unregister_device() and
mlx5_devlink_unregister().
>>
> As is the patch above fixes the dereference but results in the same
> completion error
> as current 5.8-rc4
Below patch should hopefully fix it. It fixes the bug introduced in
commit 41798df9bfca.
Additionally it fixes one small change of 42ea9f1b5c62.
Please remove all previous changes and apply below changes.
Hopefully this should resolve.
From a260b2e6a6065a57c2fa621271483cd51d0a1abf Mon Sep 17 00:00:00 2001
From: Parav Pandit
Date: Thu, 9 Jul 2020 20:57:13 +0300
Subject: [PATCH] net/mlx5: Drain health wq after unregistering device
When health worker is disabled before unregistering the devices, and if
the firmware error is triggered while in middle of unregstering the
device, it leads to very long command timeout.
This error occurs because unregistering device involves several firmware
commands, and health recovery worker is disabled. So device state is
never updated as error.
cpu-0 cpu-1
- -
remove_one()
mlx5_drain_health_wq() poll_health()
mlx5_unregister_device()mlx5_trigger_health_work()
mlx5_cmd_exec()/* health work skipped */
/* timeout */
Hence, disable the health worker after unregistering the device and
before fully unloading the device, as health worker uses the device
during health recovery. Both such contexts are serialized by
intf_state_mutex.
Change-Id: I054df865726af42ea940300b439889e58d0956d1
Fixes: 41798df9bfca ("net/mlx5: Drain wq first during PCI device removal")
Fixes: 42ea9f1b5c62 ("net/mlx5: drain health workqueue in
RE: [REGRESSION] mlx5: Driver remove during hot unplug is broken
> From: Niklas Schnelle
> Sent: Wednesday, July 8, 2020 5:14 PM
> Hi Parav, Hi Shay,
>
> On 7/8/20 12:43 PM, Parav Pandit wrote:
> > Hi Niklas,
> >
> ... snip ...
> >>>
> >
> > Sorry for my late response.
> > Yes, this looks good and I also found same in my analysis.
> > With latest code mlx5_pci_close() already does drain_health_wq(), so the
> additional call in remove_one() is redundant.
> > It should be just removed.
> > If you can verify below hunk in your setup, it will be really helpful.
> > You still need patch 42ea9f1b5c6 in your tree.
> >
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c
> > b/drivers/net/ethernet/mellanox/mlx5/core/main.c
> > index 8b658908f044..ebec2318dbc4 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
> > @@ -1382,7 +1382,6 @@ static void remove_one(struct pci_dev *pdev)
> >
> > devlink_reload_disable(devlink);
> > mlx5_crdump_disable(dev);
> > - mlx5_drain_health_wq(dev);
> > mlx5_unload_one(dev, true);
> > mlx5_pci_close(dev);
> > mlx5_mdev_uninit(dev);
> >
>
> thanks for looking into this. Sadly it looks like this doesn't quite work, I'm
> getting the bewlow illegal dereference after removing the
> mlx5_drain_health_wq(dev) from remove_one() on top of v5.8-rc4.
> I think this is similar to what happned when I tried recreating the reordering
> on -rc3 which is why I contacted Shay as I wasn't able to get this to work
> again
> with the current code.
> (Sorry if there are formatting issues with the below, plugged this out of a
> x3270).
>
It is likely because events_cleanup() freed the memory using kvfree() that
health recovery context is trying to access in notifier chain.
While reviewing I see few more errors as below.
(a) mlx5_pci_err_detected() invokes mlx5_drain_health_wq() outside of
intf_state_mutex.
(b) mlx5_enter_error_state() in commit b6e0b6bebe0 read and updates dev state
outside of intf_state_mutex.
(c) due to drain_health_wq() introduction in mlx5_pci_close() in commit
42ea9f1b5c625 health_wq is flushed twice.
(d) priv->events freed is kvfree() but allocated using kzalloc().
This code certainly needs rework.
In meantime to avoid this regression, I believe below hunk eliminates error
introduced in patch 41798df9bfc.
Will you please help test it?
Shay and I did the review of below patch.
If it works I will get it through Saeed's tree and internal reviews.
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c
b/drivers/net/ethernet/mellanox/mlx5/core/main.c
index ebec2318dbc4..529df5703737 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
@@ -785,11 +785,6 @@ static int mlx5_pci_init(struct mlx5_core_dev *dev, struct
pci_dev *pdev,
static void mlx5_pci_close(struct mlx5_core_dev *dev)
{
- /* health work might still be active, and it needs pci bar in
-* order to know the NIC state. Therefore, drain the health WQ
-* before removing the pci bars
-*/
- mlx5_drain_health_wq(dev);
iounmap(dev->iseg);
pci_clear_master(dev->pdev);
release_bar(dev->pdev);
@@ -1235,6 +1230,7 @@ void mlx5_unload_one(struct mlx5_core_dev *dev, bool
cleanup)
if (cleanup) {
mlx5_unregister_device(dev);
mlx5_devlink_unregister(priv_to_devlink(dev));
+ mlx5_drain_health_wq(dev);
} else {
mlx5_detach_device(dev);
}
@@ -1366,6 +1362,11 @@ static int init_one(struct pci_dev *pdev, const struct
pci_device_id *id)
return 0;
err_load_one:
+ /* health work might still be active, and it needs pci bar in
+* order to know the NIC state. Therefore, drain the health WQ
+* before removing the pci bars
+*/
+ mlx5_drain_health_wq(dev);
mlx5_pci_close(dev);
pci_init_err:
mlx5_mdev_uninit(dev);
> 64.735421¨ Unable to handle kernel pointer dereference in virtual kernel
> address space 64.735470¨ Failing address: 6b6b6b6b6b6b6000 TEID:
> 6b6b6b6b6b6b6803 64.735476¨ Fault in home space mode while using kernel
> ASCE.
> 64.735484¨ AS:88da0007 R3:0024 64.735533¨ Oops:
> 0038 ilc:3 Ý#1¨ PREEMPT SMP 64.735538¨ Modules linked in: rpcrdma sunrpc
> rdma_ucm rdma_cm iw_cm ib_cm configfs mlx5_ib ib_uverbs ib_core
> mlx5_core dm_multipath dm_mod scsi_dh_rdac scsi_dh_emc scsi_dh_alua
> s390_trng ghash_s390 prng ctr aes_s390 des_s390 libdes sha3_512_s390
> sha3_256_s390 sha512_s390 sha1_s390 vfio_ccw vfi 64.735421¨ Unable to
> handle kernel pointer dereference in virtual kernel address space 64.735558¨
> CPU: 0 PID: 762
RE: [REGRESSION] mlx5: Driver remove during hot unplug is broken
Hi Niklas,
> From: Niklas Schnelle
> Sent: Monday, June 15, 2020 3:32 PM
>
> Hello Saeed,
>
> On 6/13/20 12:01 AM, Saeed Mahameed wrote:
> > On Fri, 2020-06-12 at 15:09 +0200, Niklas Schnelle wrote:
> >> Hello Parav, Hello Saeed,
> >>
> ... snip ...
> >>
> >> So without really knowing anything about these functions I would
> >> guess that with the device still registered the drained queue does
> >> not remain empty as new entries are added.
> >> Does that sound plausible to you?
> >>
> >
> > I don't think it is related, maybe this is similar to some issues
> > addressed lately by Shay's patches:
> >
> >
> https://patchwork.ozlabs.org/project/netdev/patch/20200611224708.23501
> > 4-2-sae...@mellanox.com/
> >
> https://patchwork.ozlabs.org/project/netdev/patch/20200611224708.23501
> > 4-3-sae...@mellanox.com/
> >
> > net/mlx5: drain health workqueue in case of driver load error
> > net/mlx5: Fix fatal error handling during device load
>
> I agree with your similarity assessment especially for the first commit.
> These do not fix the issue though, with mainline v5.8-rc1 which has both I'm
> still getting a hang over 50% of the time with the following detach sequence
> on z/VM:
>
> vmcp detach pcif ; echo 1 > /proc/cio_settle
>
> Since now the commit 41798df9bfca ("net/mlx5: Drain wq first during PCI
> device removal") no longer reverts cleanly I used the following diff to move
> the mlx5_drain_health_wq(dev) after the mlx5_unregister_devices(dev).
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c
> b/drivers/net/ethernet/mellanox/mlx5/core/main.c
> index 8b658908f044..63a196fd8e68 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
> @@ -1382,8 +1382,8 @@ static void remove_one(struct pci_dev *pdev)
>
> devlink_reload_disable(devlink);
> mlx5_crdump_disable(dev);
> - mlx5_drain_health_wq(dev);
> mlx5_unload_one(dev, true);
> + mlx5_drain_health_wq(dev);
> mlx5_pci_close(dev);
> mlx5_mdev_uninit(dev);
> mlx5_devlink_free(devlink);
>
>
> Note that this changed order also matches the call order in
> mlx5_pci_err_detected().
> With that change I've now done over two dozen detachments with varying
> time between attach and detach to have the driver at different stages of
> initialization.
> With the change all worked without a hitch.
>
> Best regards,
> Niklas Schnelle
> >
Sorry for my late response.
Yes, this looks good and I also found same in my analysis.
With latest code mlx5_pci_close() already does drain_health_wq(), so the
additional call in remove_one() is redundant.
It should be just removed.
If you can verify below hunk in your setup, it will be really helpful.
You still need patch 42ea9f1b5c6 in your tree.
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c
b/drivers/net/ethernet/mellanox/mlx5/core/main.c
index 8b658908f044..ebec2318dbc4 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
@@ -1382,7 +1382,6 @@ static void remove_one(struct pci_dev *pdev)
devlink_reload_disable(devlink);
mlx5_crdump_disable(dev);
- mlx5_drain_health_wq(dev);
mlx5_unload_one(dev, true);
mlx5_pci_close(dev);
mlx5_mdev_uninit(dev);
RE: [PATCH V3 1/7] mdev: class id support
> -Original Message-
> From: Jason Wang
> Sent: Friday, October 11, 2019 3:16 AM
> To: k...@vger.kernel.org; linux-s...@vger.kernel.org; linux-
> ker...@vger.kernel.org; dri-de...@lists.freedesktop.org; intel-
> g...@lists.freedesktop.org; intel-gvt-...@lists.freedesktop.org;
> kwankh...@nvidia.com; alex.william...@redhat.com; m...@redhat.com;
> tiwei@intel.com
> Cc: virtualizat...@lists.linux-foundation.org; net...@vger.kernel.org;
> coh...@redhat.com; maxime.coque...@redhat.com;
> cunming.li...@intel.com; zhihong.w...@intel.com;
> rob.mil...@broadcom.com; xiao.w.w...@intel.com;
> haotian.w...@sifive.com; zhen...@linux.intel.com; zhi.a.w...@intel.com;
> jani.nik...@linux.intel.com; joonas.lahti...@linux.intel.com;
> rodrigo.v...@intel.com; airl...@linux.ie; dan...@ffwll.ch;
> far...@linux.ibm.com; pa...@linux.ibm.com; seb...@linux.ibm.com;
> ober...@linux.ibm.com; heiko.carst...@de.ibm.com; g...@linux.ibm.com;
> borntrae...@de.ibm.com; akrow...@linux.ibm.com; fre...@linux.ibm.com;
> lingshan@intel.com; Ido Shamay ;
> epere...@redhat.com; l...@redhat.com; Parav Pandit
> ; christophe.de.dinec...@gmail.com;
> kevin.t...@intel.com; Jason Wang
> Subject: [PATCH V3 1/7] mdev: class id support
>
> Mdev bus only supports vfio driver right now, so it doesn't implement match
> method. But in the future, we may add drivers other than vfio, the first
> driver could be virtio-mdev. This means we need to add device class id
> support in bus match method to pair the mdev device and mdev driver
> correctly.
>
> So this patch adds id_table to mdev_driver and class_id for mdev device
> with the match method for mdev bus.
>
> Signed-off-by: Jason Wang
> ---
> Documentation/driver-api/vfio-mediated-device.rst | 7 ++-
> drivers/gpu/drm/i915/gvt/kvmgt.c | 1 +
> drivers/s390/cio/vfio_ccw_ops.c | 1 +
> drivers/s390/crypto/vfio_ap_ops.c | 1 +
> drivers/vfio/mdev/mdev_core.c | 11 +++
> drivers/vfio/mdev/mdev_driver.c | 14 ++
> drivers/vfio/mdev/mdev_private.h | 1 +
> drivers/vfio/mdev/vfio_mdev.c | 6 ++
> include/linux/mdev.h | 8
> include/linux/mod_devicetable.h | 8
> samples/vfio-mdev/mbochs.c| 1 +
> samples/vfio-mdev/mdpy.c | 1 +
> samples/vfio-mdev/mtty.c | 1 +
> 13 files changed, 60 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/driver-api/vfio-mediated-device.rst
> b/Documentation/driver-api/vfio-mediated-device.rst
> index 25eb7d5b834b..2035e48da7b2 100644
> --- a/Documentation/driver-api/vfio-mediated-device.rst
> +++ b/Documentation/driver-api/vfio-mediated-device.rst
> @@ -102,12 +102,14 @@ structure to represent a mediated device's driver::
>* @probe: called when new device created
>* @remove: called when device removed
>* @driver: device driver structure
> + * @id_table: the ids serviced by this driver
>*/
> struct mdev_driver {
>const char *name;
>int (*probe) (struct device *dev);
>void (*remove) (struct device *dev);
>struct device_driverdriver;
> + const struct mdev_class_id *id_table;
> };
>
> A mediated bus driver for mdev should use this structure in the function
> calls @@ -165,12 +167,15 @@ register itself with the mdev core driver::
> extern int mdev_register_device(struct device *dev,
>const struct mdev_parent_ops *ops);
>
> +It is also required to specify the class_id through::
> +
> + extern int mdev_set_class(struct device *dev, u16 id);
Drop extern.
In actual API you have correct signature, i.e. struct mdev_device.
s/struct device/struct mdev_device.
> +
> However, the mdev_parent_ops structure is not required in the function call
> that a driver should use to unregister itself with the mdev core driver::
>
> extern void mdev_unregister_device(struct device *dev);
>
> -
> Mediated Device Management Interface Through sysfs
> ==
>
> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c
> b/drivers/gpu/drm/i915/gvt/kvmgt.c
> index 343d79c1cb7e..17e9d4634c84 100644
> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> @@ -678,6 +678,7 @@ static int intel_vgpu_create(struct kobject *kobj,
> struct mdev_device *mdev)
>dev_name(mdev_dev(mdev)));
> ret = 0;
>
> + mdev_set_class(mdev, MDEV_ID_VFIO
RE: [PATCH v3 0/5] Introduce variable length mdev alias
Hi Cornelia, > -Original Message- > From: Cornelia Huck > Sent: Tuesday, September 17, 2019 5:14 AM > To: Parav Pandit > Cc: alex.william...@redhat.com; Jiri Pirko ; > kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org; linux- > ker...@vger.kernel.org; net...@vger.kernel.org > Subject: Re: [PATCH v3 0/5] Introduce variable length mdev alias > > On Sun, 1 Sep 2019 23:24:31 -0500 > Parav Pandit wrote: > > > To have consistent naming for the netdevice of a mdev and to have > > consistent naming of the devlink port [1] of a mdev, which is formed > > using phys_port_name of the devlink port, current UUID is not usable > > because UUID is too long. > > > > UUID in string format is 36-characters long and in binary 128-bit. > > Both formats are not able to fit within 15 characters limit of netdev > > name. > > > > It is desired to have mdev device naming consistent using UUID. > > So that widely used user space framework such as ovs [2] can make use > > of mdev representor in similar way as PCIe SR-IOV VF and PF representors. > > > > Hence, > > (a) mdev alias is created which is derived using sha1 from the mdev name. > > (b) Vendor driver describes how long an alias should be for the child > > mdev created for a given parent. > > (c) Mdev aliases are unique at system level. > > (d) alias is created optionally whenever parent requested. > > This ensures that non networking mdev parents can function without > > alias creation overhead. > > > > This design is discussed at [3]. > > > > An example systemd/udev extension will have, > > > > 1. netdev name created using mdev alias available in sysfs. > > > > mdev UUID=83b8f4f2-509f-382f-3c1e-e6bfe0fa1001 > > mdev 12 character alias=cd5b146a80a5 > > > > netdev name of this mdev = enmcd5b146a80a5 Here en = Ethernet link m = > > mediated device > > > > 2. devlink port phys_port_name created using mdev alias. > > devlink phys_port_name=pcd5b146a80a5 > > > > This patchset enables mdev core to maintain unique alias for a mdev. > > > > Patch-1 Introduces mdev alias using sha1. > > Patch-2 Ensures that mdev alias is unique in a system. > > Patch-3 Exposes mdev alias in a sysfs hirerchy, update Documentation > > Patch-4 Introduces mdev_alias() API. > > Patch-5 Extends mtty driver to optionally provide alias generation. > > This also enables to test UUID based sha1 collision and trigger error > > handling for duplicate sha1 results. > > > > [1] http://man7.org/linux/man-pages/man8/devlink-port.8.html > > [2] https://docs.openstack.org/os-vif/latest/user/plugins/ovs.html > > [3] https://patchwork.kernel.org/cover/11084231/ > > > > --- > > Changelog: > > v2->v3: > > - Addressed comment from Yunsheng Lin > > - Changed strcmp() ==0 to !strcmp() > > - Addressed comment from Cornelia Hunk > > - Merged sysfs Documentation patch with syfs patch > > - Added more description for alias return value > > v1->v2: > > - Corrected a typo from 'and' to 'an' > > - Addressed comments from Alex Williamson > > - Kept mdev_device naturally aligned > > - Added error checking for crypt_*() calls > > - Moved alias NULL check at beginning > > - Added mdev_alias() API > > - Updated mtty driver to show example mdev_alias() usage > > - Changed return type of generate_alias() from int to char* > > v0->v1: > > - Addressed comments from Alex Williamson, Cornelia Hunk and Mark > > Bloch > > - Moved alias length check outside of the parent lock > > - Moved alias and digest allocation from kvzalloc to kzalloc > > - [0] changed to alias > > - alias_length check is nested under get_alias_length callback check > > - Changed comments to start with an empty line > > - Added comment where alias memory ownership is handed over to mdev > > device > > - Fixed cleaunup of hash if mdev_bus_register() fails > > - Updated documentation for new sysfs alias file > > - Improved commit logs to make description more clear > > - Fixed inclusiong of alias for NULL check > > - Added ratelimited debug print for sha1 hash collision error > > > > Parav Pandit (5): > > mdev: Introduce sha1 based mdev alias > > mdev: Make mdev alias unique among all mdevs > > mdev: Expose mdev alias in sysfs tree > > mdev: Introduce an API mdev_alias > > mtty: Optionally support mtty alias > > > > .../driver-api/vfio-mediated-device.rst | 9 ++ > > drivers/vfio/mdev/mdev_core.c
RE: [PATCH v3 0/5] Introduce variable length mdev alias
Hi Alex, > -Original Message- > From: Alex Williamson > Sent: Friday, September 13, 2019 4:33 PM > To: Parav Pandit > Cc: Jiri Pirko ; kwankh...@nvidia.com; > coh...@redhat.com; da...@davemloft.net; k...@vger.kernel.org; linux- > ker...@vger.kernel.org; net...@vger.kernel.org > Subject: Re: [PATCH v3 0/5] Introduce variable length mdev alias > > On Wed, 11 Sep 2019 16:38:49 + > Parav Pandit wrote: > > > > -Original Message- > > > From: linux-kernel-ow...@vger.kernel.org > > ow...@vger.kernel.org> On Behalf Of Parav Pandit > > > Sent: Wednesday, September 11, 2019 10:31 AM > > > To: Alex Williamson > > > Cc: Jiri Pirko ; kwankh...@nvidia.com; > > > coh...@redhat.com; da...@davemloft.net; k...@vger.kernel.org; linux- > > > ker...@vger.kernel.org; net...@vger.kernel.org > > > Subject: RE: [PATCH v3 0/5] Introduce variable length mdev alias > > > > > > Hi Alex, > > > > > > > -Original Message- > > > > From: Alex Williamson > > > > Sent: Wednesday, September 11, 2019 8:56 AM > > > > To: Parav Pandit > > > > Cc: Jiri Pirko ; kwankh...@nvidia.com; > > > > coh...@redhat.com; da...@davemloft.net; k...@vger.kernel.org; > > > > linux- ker...@vger.kernel.org; net...@vger.kernel.org > > > > Subject: Re: [PATCH v3 0/5] Introduce variable length mdev alias > > > > > > > > On Mon, 9 Sep 2019 20:42:32 + > > > > Parav Pandit wrote: > > > > > > > > > Hi Alex, > > > > > > > > > > > -Original Message- > > > > > > From: Parav Pandit > > > > > > Sent: Sunday, September 1, 2019 11:25 PM > > > > > > To: alex.william...@redhat.com; Jiri Pirko > > > > > > ; kwankh...@nvidia.com; coh...@redhat.com; > > > > > > da...@davemloft.net > > > > > > Cc: k...@vger.kernel.org; linux-kernel@vger.kernel.org; > > > > > > net...@vger.kernel.org; Parav Pandit > > > > > > Subject: [PATCH v3 0/5] Introduce variable length mdev alias > > > > > > > > > > > > To have consistent naming for the netdevice of a mdev and to > > > > > > have consistent naming of the devlink port [1] of a mdev, > > > > > > which is formed using phys_port_name of the devlink port, > > > > > > current UUID is not usable because UUID is too long. > > > > > > > > > > > > UUID in string format is 36-characters long and in binary 128-bit. > > > > > > Both formats are not able to fit within 15 characters limit of > > > > > > netdev > > > > name. > > > > > > > > > > > > It is desired to have mdev device naming consistent using UUID. > > > > > > So that widely used user space framework such as ovs [2] can > > > > > > make use of mdev representor in similar way as PCIe SR-IOV VF > > > > > > and PF > > > > representors. > > > > > > > > > > > > Hence, > > > > > > (a) mdev alias is created which is derived using sha1 from the > > > > > > mdev > > > > name. > > > > > > (b) Vendor driver describes how long an alias should be for > > > > > > the child mdev created for a given parent. > > > > > > (c) Mdev aliases are unique at system level. > > > > > > (d) alias is created optionally whenever parent requested. > > > > > > This ensures that non networking mdev parents can function > > > > > > without alias creation overhead. > > > > > > > > > > > > This design is discussed at [3]. > > > > > > > > > > > > An example systemd/udev extension will have, > > > > > > > > > > > > 1. netdev name created using mdev alias available in sysfs. > > > > > > > > > > > > mdev UUID=83b8f4f2-509f-382f-3c1e-e6bfe0fa1001 > > > > > > mdev 12 character alias=cd5b146a80a5 > > > > > > > > > > > > netdev name of this mdev = enmcd5b146a80a5 Here en = Ethernet > > > > > > link m = mediated device > > > > > > > > > > > > 2. devlink port phys_port_name created using mdev alias. > > > > > > devlink phys_port_name=pcd5b146a80a5 > > > > > > > > > &g
RE: [PATCH v3 0/5] Introduce variable length mdev alias
> -Original Message- > From: linux-kernel-ow...@vger.kernel.org ow...@vger.kernel.org> On Behalf Of Parav Pandit > Sent: Wednesday, September 11, 2019 10:31 AM > To: Alex Williamson > Cc: Jiri Pirko ; kwankh...@nvidia.com; > coh...@redhat.com; da...@davemloft.net; k...@vger.kernel.org; linux- > ker...@vger.kernel.org; net...@vger.kernel.org > Subject: RE: [PATCH v3 0/5] Introduce variable length mdev alias > > Hi Alex, > > > -Original Message- > > From: Alex Williamson > > Sent: Wednesday, September 11, 2019 8:56 AM > > To: Parav Pandit > > Cc: Jiri Pirko ; kwankh...@nvidia.com; > > coh...@redhat.com; da...@davemloft.net; k...@vger.kernel.org; linux- > > ker...@vger.kernel.org; net...@vger.kernel.org > > Subject: Re: [PATCH v3 0/5] Introduce variable length mdev alias > > > > On Mon, 9 Sep 2019 20:42:32 + > > Parav Pandit wrote: > > > > > Hi Alex, > > > > > > > -Original Message- > > > > From: Parav Pandit > > > > Sent: Sunday, September 1, 2019 11:25 PM > > > > To: alex.william...@redhat.com; Jiri Pirko ; > > > > kwankh...@nvidia.com; coh...@redhat.com; da...@davemloft.net > > > > Cc: k...@vger.kernel.org; linux-kernel@vger.kernel.org; > > > > net...@vger.kernel.org; Parav Pandit > > > > Subject: [PATCH v3 0/5] Introduce variable length mdev alias > > > > > > > > To have consistent naming for the netdevice of a mdev and to have > > > > consistent naming of the devlink port [1] of a mdev, which is > > > > formed using phys_port_name of the devlink port, current UUID is > > > > not usable because UUID is too long. > > > > > > > > UUID in string format is 36-characters long and in binary 128-bit. > > > > Both formats are not able to fit within 15 characters limit of > > > > netdev > > name. > > > > > > > > It is desired to have mdev device naming consistent using UUID. > > > > So that widely used user space framework such as ovs [2] can make > > > > use of mdev representor in similar way as PCIe SR-IOV VF and PF > > representors. > > > > > > > > Hence, > > > > (a) mdev alias is created which is derived using sha1 from the > > > > mdev > > name. > > > > (b) Vendor driver describes how long an alias should be for the > > > > child mdev created for a given parent. > > > > (c) Mdev aliases are unique at system level. > > > > (d) alias is created optionally whenever parent requested. > > > > This ensures that non networking mdev parents can function without > > > > alias creation overhead. > > > > > > > > This design is discussed at [3]. > > > > > > > > An example systemd/udev extension will have, > > > > > > > > 1. netdev name created using mdev alias available in sysfs. > > > > > > > > mdev UUID=83b8f4f2-509f-382f-3c1e-e6bfe0fa1001 > > > > mdev 12 character alias=cd5b146a80a5 > > > > > > > > netdev name of this mdev = enmcd5b146a80a5 Here en = Ethernet link > > > > m = mediated device > > > > > > > > 2. devlink port phys_port_name created using mdev alias. > > > > devlink phys_port_name=pcd5b146a80a5 > > > > > > > > This patchset enables mdev core to maintain unique alias for a mdev. > > > > > > > > Patch-1 Introduces mdev alias using sha1. > > > > Patch-2 Ensures that mdev alias is unique in a system. > > > > Patch-3 Exposes mdev alias in a sysfs hirerchy, update > > > > Documentation > > > > Patch-4 Introduces mdev_alias() API. > > > > Patch-5 Extends mtty driver to optionally provide alias generation. > > > > This also enables to test UUID based sha1 collision and trigger > > > > error handling for duplicate sha1 results. > > > > > > > > [1] http://man7.org/linux/man-pages/man8/devlink-port.8.html > > > > [2] https://docs.openstack.org/os-vif/latest/user/plugins/ovs.html > > > > [3] https://patchwork.kernel.org/cover/11084231/ > > > > > > > > --- > > > > Changelog: > > > > v2->v3: > > > > - Addressed comment from Yunsheng Lin > > > > - Changed strcmp() ==0 to !strcmp() > > > > - Addressed comment from Cornelia Hunk > > > > - Merged sysfs Documentation patch with syfs patch > > > >
RE: [PATCH v3 0/5] Introduce variable length mdev alias
Hi Alex, > -Original Message- > From: Alex Williamson > Sent: Wednesday, September 11, 2019 8:56 AM > To: Parav Pandit > Cc: Jiri Pirko ; kwankh...@nvidia.com; > coh...@redhat.com; da...@davemloft.net; k...@vger.kernel.org; linux- > ker...@vger.kernel.org; net...@vger.kernel.org > Subject: Re: [PATCH v3 0/5] Introduce variable length mdev alias > > On Mon, 9 Sep 2019 20:42:32 + > Parav Pandit wrote: > > > Hi Alex, > > > > > -Original Message- > > > From: Parav Pandit > > > Sent: Sunday, September 1, 2019 11:25 PM > > > To: alex.william...@redhat.com; Jiri Pirko ; > > > kwankh...@nvidia.com; coh...@redhat.com; da...@davemloft.net > > > Cc: k...@vger.kernel.org; linux-kernel@vger.kernel.org; > > > net...@vger.kernel.org; Parav Pandit > > > Subject: [PATCH v3 0/5] Introduce variable length mdev alias > > > > > > To have consistent naming for the netdevice of a mdev and to have > > > consistent naming of the devlink port [1] of a mdev, which is formed > > > using phys_port_name of the devlink port, current UUID is not usable > > > because UUID is too long. > > > > > > UUID in string format is 36-characters long and in binary 128-bit. > > > Both formats are not able to fit within 15 characters limit of netdev > name. > > > > > > It is desired to have mdev device naming consistent using UUID. > > > So that widely used user space framework such as ovs [2] can make > > > use of mdev representor in similar way as PCIe SR-IOV VF and PF > representors. > > > > > > Hence, > > > (a) mdev alias is created which is derived using sha1 from the mdev > name. > > > (b) Vendor driver describes how long an alias should be for the > > > child mdev created for a given parent. > > > (c) Mdev aliases are unique at system level. > > > (d) alias is created optionally whenever parent requested. > > > This ensures that non networking mdev parents can function without > > > alias creation overhead. > > > > > > This design is discussed at [3]. > > > > > > An example systemd/udev extension will have, > > > > > > 1. netdev name created using mdev alias available in sysfs. > > > > > > mdev UUID=83b8f4f2-509f-382f-3c1e-e6bfe0fa1001 > > > mdev 12 character alias=cd5b146a80a5 > > > > > > netdev name of this mdev = enmcd5b146a80a5 Here en = Ethernet link m > > > = mediated device > > > > > > 2. devlink port phys_port_name created using mdev alias. > > > devlink phys_port_name=pcd5b146a80a5 > > > > > > This patchset enables mdev core to maintain unique alias for a mdev. > > > > > > Patch-1 Introduces mdev alias using sha1. > > > Patch-2 Ensures that mdev alias is unique in a system. > > > Patch-3 Exposes mdev alias in a sysfs hirerchy, update Documentation > > > Patch-4 Introduces mdev_alias() API. > > > Patch-5 Extends mtty driver to optionally provide alias generation. > > > This also enables to test UUID based sha1 collision and trigger > > > error handling for duplicate sha1 results. > > > > > > [1] http://man7.org/linux/man-pages/man8/devlink-port.8.html > > > [2] https://docs.openstack.org/os-vif/latest/user/plugins/ovs.html > > > [3] https://patchwork.kernel.org/cover/11084231/ > > > > > > --- > > > Changelog: > > > v2->v3: > > > - Addressed comment from Yunsheng Lin > > > - Changed strcmp() ==0 to !strcmp() > > > - Addressed comment from Cornelia Hunk > > > - Merged sysfs Documentation patch with syfs patch > > > - Added more description for alias return value > > > > Did you get a chance review this updated series? > > I addressed Cornelia's and yours comment. > > I do not think allocating alias memory twice, once for comparison and > > once for storing is good idea or moving alias generation logic inside > > the mdev_list_lock(). So I didn't address that suggestion of Cornelia. > > Sorry, I'm at LPC this week. I agree, I don't think the double allocation is > necessary, I thought the comment was sufficient to clarify null'ing the > variable. It's awkward, but seems correct. > > I'm not sure what we do with this patch series though, has the real > consumer of this even been proposed? It feels optimistic to include at this > point. We've used the sample driver as a placeholder in the past for > mdev_uuid(), but we arrived at that via a conversion rather than explicitly > adding the API. Please let me know where the consumer patches stand, > perhaps it would make more sense for them to go in together rather than > risk adding an unused API. Thanks, > Given that consumer patch series is relatively large (around 15+ patches), I was considering to merge this one as pre-series to it. Its ok to combine this with consumer patch series. But wanted to have it reviewed beforehand, so that churn is less in actual consumer series which is more mlx5_core and devlink/netdev centric. So if you can add Review-by, it will be easier to combine with consumer series. And if we merge it with consumer series, it will come through Dave Miller's tree instead of your tree. Would that work for you?
RE: [PATCH v3 0/5] Introduce variable length mdev alias
Hi Alex, > -Original Message- > From: Parav Pandit > Sent: Sunday, September 1, 2019 11:25 PM > To: alex.william...@redhat.com; Jiri Pirko ; > kwankh...@nvidia.com; coh...@redhat.com; da...@davemloft.net > Cc: k...@vger.kernel.org; linux-kernel@vger.kernel.org; > net...@vger.kernel.org; Parav Pandit > Subject: [PATCH v3 0/5] Introduce variable length mdev alias > > To have consistent naming for the netdevice of a mdev and to have consistent > naming of the devlink port [1] of a mdev, which is formed using > phys_port_name of the devlink port, current UUID is not usable because UUID > is too long. > > UUID in string format is 36-characters long and in binary 128-bit. > Both formats are not able to fit within 15 characters limit of netdev name. > > It is desired to have mdev device naming consistent using UUID. > So that widely used user space framework such as ovs [2] can make use of > mdev representor in similar way as PCIe SR-IOV VF and PF representors. > > Hence, > (a) mdev alias is created which is derived using sha1 from the mdev name. > (b) Vendor driver describes how long an alias should be for the child mdev > created for a given parent. > (c) Mdev aliases are unique at system level. > (d) alias is created optionally whenever parent requested. > This ensures that non networking mdev parents can function without alias > creation overhead. > > This design is discussed at [3]. > > An example systemd/udev extension will have, > > 1. netdev name created using mdev alias available in sysfs. > > mdev UUID=83b8f4f2-509f-382f-3c1e-e6bfe0fa1001 > mdev 12 character alias=cd5b146a80a5 > > netdev name of this mdev = enmcd5b146a80a5 Here en = Ethernet link m = > mediated device > > 2. devlink port phys_port_name created using mdev alias. > devlink phys_port_name=pcd5b146a80a5 > > This patchset enables mdev core to maintain unique alias for a mdev. > > Patch-1 Introduces mdev alias using sha1. > Patch-2 Ensures that mdev alias is unique in a system. > Patch-3 Exposes mdev alias in a sysfs hirerchy, update Documentation > Patch-4 Introduces mdev_alias() API. > Patch-5 Extends mtty driver to optionally provide alias generation. > This also enables to test UUID based sha1 collision and trigger error handling > for duplicate sha1 results. > > [1] http://man7.org/linux/man-pages/man8/devlink-port.8.html > [2] https://docs.openstack.org/os-vif/latest/user/plugins/ovs.html > [3] https://patchwork.kernel.org/cover/11084231/ > > --- > Changelog: > v2->v3: > - Addressed comment from Yunsheng Lin > - Changed strcmp() ==0 to !strcmp() > - Addressed comment from Cornelia Hunk > - Merged sysfs Documentation patch with syfs patch > - Added more description for alias return value Did you get a chance review this updated series? I addressed Cornelia's and yours comment. I do not think allocating alias memory twice, once for comparison and once for storing is good idea or moving alias generation logic inside the mdev_list_lock(). So I didn't address that suggestion of Cornelia. > v1->v2: > - Corrected a typo from 'and' to 'an' > - Addressed comments from Alex Williamson > - Kept mdev_device naturally aligned > - Added error checking for crypt_*() calls > - Moved alias NULL check at beginning > - Added mdev_alias() API > - Updated mtty driver to show example mdev_alias() usage > - Changed return type of generate_alias() from int to char* > v0->v1: > - Addressed comments from Alex Williamson, Cornelia Hunk and Mark Bloch > - Moved alias length check outside of the parent lock > - Moved alias and digest allocation from kvzalloc to kzalloc > - [0] changed to alias > - alias_length check is nested under get_alias_length callback check > - Changed comments to start with an empty line > - Added comment where alias memory ownership is handed over to mdev > device > - Fixed cleaunup of hash if mdev_bus_register() fails > - Updated documentation for new sysfs alias file > - Improved commit logs to make description more clear > - Fixed inclusiong of alias for NULL check > - Added ratelimited debug print for sha1 hash collision error > > Parav Pandit (5): > mdev: Introduce sha1 based mdev alias > mdev: Make mdev alias unique among all mdevs > mdev: Expose mdev alias in sysfs tree > mdev: Introduce an API mdev_alias > mtty: Optionally support mtty alias > > .../driver-api/vfio-mediated-device.rst | 9 ++ > drivers/vfio/mdev/mdev_core.c | 142 +- > drivers/vfio/mdev/mdev_private.h | 5 +- > drivers/vfio/mdev/mdev_sysfs.c| 26 +++- > include/linux/mdev.h | 5 + > samples/vfio-mdev/mtty.c | 13 ++ > 6 files changed, 190 insertions(+), 10 deletions(-) > > -- > 2.19.2
RE: [PATCH v2 5/6] mdev: Update sysfs documentation
> -Original Message-
> From: Cornelia Huck
> Sent: Monday, September 2, 2019 8:07 PM
> To: Parav Pandit
> Cc: alex.william...@redhat.com; Jiri Pirko ;
> kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org;
> linux-kernel@vger.kernel.org; net...@vger.kernel.org
> Subject: Re: [PATCH v2 5/6] mdev: Update sysfs documentation
>
> On Fri, 30 Aug 2019 13:10:17 +
> Parav Pandit wrote:
>
> > > -Original Message-
> > > From: Cornelia Huck
> > > Sent: Friday, August 30, 2019 6:19 PM
> > > To: Parav Pandit
> > > Cc: alex.william...@redhat.com; Jiri Pirko ;
> > > kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org;
> > > linux- ker...@vger.kernel.org; net...@vger.kernel.org
> > > Subject: Re: [PATCH v2 5/6] mdev: Update sysfs documentation
> > >
> > > On Thu, 29 Aug 2019 06:19:03 -0500
> > > Parav Pandit wrote:
> > >
> > > > Updated documentation for optional read only sysfs attribute.
> > >
> > > I'd probably merge this into the patch introducing the attribute.
> > >
> > Ok. I will spin v3.
> >
> > > >
> > > > Signed-off-by: Parav Pandit
> > > > ---
> > > > Documentation/driver-api/vfio-mediated-device.rst | 5 +
> > > > 1 file changed, 5 insertions(+)
> > > >
> > > > diff --git a/Documentation/driver-api/vfio-mediated-device.rst
> > > > b/Documentation/driver-api/vfio-mediated-device.rst
> > > > index 25eb7d5b834b..0ab03d3f5629 100644
> > > > --- a/Documentation/driver-api/vfio-mediated-device.rst
> > > > +++ b/Documentation/driver-api/vfio-mediated-device.rst
> > > > @@ -270,6 +270,7 @@ Directories and Files Under the sysfs for Each
> > > > mdev
> > > Device
> > > > |--- remove
> > > > |--- mdev_type {link to its type}
> > > > |--- vendor-specific-attributes [optional]
> > > > + |--- alias [optional]
> > >
> > > "optional" implies "not always present" to me, not "might return a
> > > read error if not available". Don't know if there's a better way to
> > > tag this? Or make it really optional? :)
> >
> > May be write it as,
> >
> > alias [ optional when requested by parent ]
>
> I'm not sure what 'optional when requested' is supposed to mean...
> maybe something like 'content optional' or so?
>
> >
> > >
> > > >
> > > > * remove (write only)
> > > >
> > > > @@ -281,6 +282,10 @@ Example::
> > > >
> > > > # echo 1 > /sys/bus/mdev/devices/$mdev_UUID/remove
> > > >
> > > > +* alias (read only)
> > > > +Whenever a parent requested to generate an alias, each mdev is
> > > > +assigned a unique alias by the mdev core. This file shows the
> > > > +alias of the
> > > mdev device.
> > >
> > > It's not really the parent, but the vendor driver requesting this,
> > > right? Also,
> > At mdev level, it only knows parent->ops structure, whether parent is
> registered by vendor driver or something else.
>
> Who else is supposed to create the mdev device?
If you nitpick the language what is the vendor id for sample mttty driver?
Mtty is not a 'vendor driver' per say.
>
> >
> > > "each mdev" is a bit ambiguous,
> > It is in context of the parent. Sentence is not starting with "each mdev".
> > But may be more verbosely written as,
> >
> > Whenever a parent requested to generate an alias, Each mdev device of
> > such parent is assigned unique alias by the mdev core. This file shows the
> alias of the mdev device.
>
> I'd really leave the parent out of this: this seems more like an
> implementation detail. It's more that alias may either contain an alias, or
> return a read error if no alias has been generated. Who requested the alias
> to be generated is probably not really of interest to the userspace reader.
>
The documentation is for user and developer both.
It is not the right claim that 'only user care' for this.
Otherwise all the .ko diagrams and API description etc doesn't make any sense
to the user.
For user it doesn't matter whether alias length is provided by 'vendor driver'
or 'registered parent'.
This note on who should specify the alias length is mainly for the developers.
> >
> > > created via that driver. Lastly, if we stick with the "returns an
> > > error if not implemented" approach, that should also be mentioned
> here.
> > Ok. Will spin v3 to describe it.
> >
> > >
> > > > +
> > > > Mediated device Hot plug
> > > >
> > > >
> >
RE: [PATCH v2 1/6] mdev: Introduce sha1 based mdev alias
> -Original Message- > From: Cornelia Huck > Sent: Monday, September 2, 2019 8:16 PM > To: Parav Pandit > Cc: alex.william...@redhat.com; Jiri Pirko ; > kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org; > linux-kernel@vger.kernel.org; net...@vger.kernel.org > Subject: Re: [PATCH v2 1/6] mdev: Introduce sha1 based mdev alias > > On Fri, 30 Aug 2019 15:45:13 + > Parav Pandit wrote: > > > > > > > > This detour via the local variable looks weird to me. Can > > > > > > > you either create the alias directly in the mdev (would need > > > > > > > to happen later in the function, but I'm not sure why you > > > > > > > generate the alias before checking for duplicates anyway), or do > an explicit copy? > > > > > > Alias duplicate check is done after generating it, because > > > > > > duplicate alias are > > > > > not allowed. > > > > > > The probability of collision is rare. > > > > > > So it is speculatively generated without hold the lock, > > > > > > because there is no > > > > > need to hold the lock. > > > > > > It is compared along with guid while mutex lock is held in single > loop. > > > > > > And if it is duplicate, there is no need to allocate mdev. > > > > > > > > > > > > It will be sub optimal to run through the mdev list 2nd time > > > > > > after mdev > > > > > creation and after generating alias for duplicate check. > > > > > > > > > > Ok, but what about copying it? I find this "set local variable > > > > > to NULL after ownership is transferred" pattern a bit unintuitive. > > > > > Copying it to the mdev (and then unconditionally freeing it) > > > > > looks more > > > obvious to me. > > > > Its not unconditionally freed. > > > > > > That's not what I have been saying :( > > > > > Ah I see. You want to allocate alias memory twice; once inside mdev device > and another one in _create() function. > > _create() one you want to free unconditionally. > > > > Well, passing pointer is fine. > > It's not that it doesn't work, but it feels fragile due to its > non-obviousness. And its well commented as Alex asked. > > > mdev_register_device() has similar little tricky pattern that makes parent = > NULL on __find_parent_device() finds duplicate one. > > I don't think that the two are comparable. > They are very similar. Why parent should be marked null otherwise. > > > > Ownership transfer is more straight forward code. > > I have to disagree here. > Ok. It is better than allocating memory twice. So I prefer to stick to this method. > > > > It is similar to device_initialize(), device init sequence code, where once > device_initialize is done, freeing the device memory will be left to the > put_device(), we don't call kfree() on mdev device. > > This does not really look similar to me: devices are refcounted structures, > while strings aren't; you transfer a local pointer to a refcounted structure > and then discard the local reference.
[PATCH v3 1/5] mdev: Introduce sha1 based mdev alias
Some vendor drivers want an identifier for an mdev device that is
shorter than the UUID, due to length restrictions in the consumers of
that identifier.
Add a callback that allows a vendor driver to request an alias of a
specified length to be generated for an mdev device. If generated,
that alias is checked for collisions.
It is an optional attribute.
mdev alias is generated using sha1 from the mdev name.
Signed-off-by: Parav Pandit
---
Changelog:
v1->v2:
- Kept mdev_device naturally aligned
- Added error checking for crypt_*() calls
- Corrected a typo from 'and' to 'an'
- Changed return type of generate_alias() from int to char*
v0->v1:
- Moved alias length check outside of the parent lock
- Moved alias and digest allocation from kvzalloc to kzalloc
- [0] changed to alias
- alias_length check is nested under get_alias_length callback check
- Changed comments to start with an empty line
- Fixed cleaunup of hash if mdev_bus_register() fails
- Added comment where alias memory ownership is handed over to mdev device
- Updated commit log to indicate motivation for this feature
---
drivers/vfio/mdev/mdev_core.c| 123 ++-
drivers/vfio/mdev/mdev_private.h | 5 +-
drivers/vfio/mdev/mdev_sysfs.c | 13 ++--
include/linux/mdev.h | 4 +
4 files changed, 135 insertions(+), 10 deletions(-)
diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
index b558d4cfd082..3bdff0469607 100644
--- a/drivers/vfio/mdev/mdev_core.c
+++ b/drivers/vfio/mdev/mdev_core.c
@@ -10,9 +10,11 @@
#include
#include
#include
+#include
#include
#include
#include
+#include
#include "mdev_private.h"
@@ -27,6 +29,8 @@ static struct class_compat *mdev_bus_compat_class;
static LIST_HEAD(mdev_list);
static DEFINE_MUTEX(mdev_list_lock);
+static struct crypto_shash *alias_hash;
+
struct device *mdev_parent_dev(struct mdev_device *mdev)
{
return mdev->parent->dev;
@@ -150,6 +154,16 @@ int mdev_register_device(struct device *dev, const struct
mdev_parent_ops *ops)
if (!ops || !ops->create || !ops->remove || !ops->supported_type_groups)
return -EINVAL;
+ if (ops->get_alias_length) {
+ unsigned int digest_size;
+ unsigned int aligned_len;
+
+ aligned_len = roundup(ops->get_alias_length(), 2);
+ digest_size = crypto_shash_digestsize(alias_hash);
+ if (aligned_len / 2 > digest_size)
+ return -EINVAL;
+ }
+
dev = get_device(dev);
if (!dev)
return -EINVAL;
@@ -259,6 +273,7 @@ static void mdev_device_free(struct mdev_device *mdev)
mutex_unlock(_list_lock);
dev_dbg(>dev, "MDEV: destroying\n");
+ kfree(mdev->alias);
kfree(mdev);
}
@@ -269,18 +284,101 @@ static void mdev_device_release(struct device *dev)
mdev_device_free(mdev);
}
-int mdev_device_create(struct kobject *kobj,
- struct device *dev, const guid_t *uuid)
+static const char *
+generate_alias(const char *uuid, unsigned int max_alias_len)
+{
+ struct shash_desc *hash_desc;
+ unsigned int digest_size;
+ unsigned char *digest;
+ unsigned int alias_len;
+ char *alias;
+ int ret;
+
+ /*
+* Align to multiple of 2 as bin2hex will generate
+* even number of bytes.
+*/
+ alias_len = roundup(max_alias_len, 2);
+ alias = kzalloc(alias_len + 1, GFP_KERNEL);
+ if (!alias)
+ return ERR_PTR(-ENOMEM);
+
+ /* Allocate and init descriptor */
+ hash_desc = kvzalloc(sizeof(*hash_desc) +
+crypto_shash_descsize(alias_hash),
+GFP_KERNEL);
+ if (!hash_desc) {
+ ret = -ENOMEM;
+ goto desc_err;
+ }
+
+ hash_desc->tfm = alias_hash;
+
+ digest_size = crypto_shash_digestsize(alias_hash);
+
+ digest = kzalloc(digest_size, GFP_KERNEL);
+ if (!digest) {
+ ret = -ENOMEM;
+ goto digest_err;
+ }
+ ret = crypto_shash_init(hash_desc);
+ if (ret)
+ goto hash_err;
+
+ ret = crypto_shash_update(hash_desc, uuid, UUID_STRING_LEN);
+ if (ret)
+ goto hash_err;
+
+ ret = crypto_shash_final(hash_desc, digest);
+ if (ret)
+ goto hash_err;
+
+ bin2hex(alias, digest, min_t(unsigned int, digest_size, alias_len / 2));
+ /*
+* When alias length is odd, zero out an additional last byte
+* that bin2hex has copied.
+*/
+ if (max_alias_len % 2)
+ alias[max_alias_len] = 0;
+
+ kfree(digest);
+ kvfree(hash_desc);
+ return alias;
+
+hash_err:
+ kfree(digest);
+digest_err:
+ kvfree(hash_desc);
+desc_err:
+ kfree(alias);
+ retur
[PATCH v3 3/5] mdev: Expose mdev alias in sysfs tree
Expose the optional alias for an mdev device as a sysfs attribute.
This way, userspace tools such as udev may make use of the alias, for
example to create a netdevice name for the mdev.
Updated documentation for optional read only sysfs attribute.
Signed-off-by: Parav Pandit
---
Changelog:
v2->v3:
- Merged sysfs documentation patch with sysfs addition
- Added more description for alias return value
v0->v1:
- Addressed comments from Cornelia Huck
- Updated commit description
---
Documentation/driver-api/vfio-mediated-device.rst | 9 +
drivers/vfio/mdev/mdev_sysfs.c| 13 +
2 files changed, 22 insertions(+)
diff --git a/Documentation/driver-api/vfio-mediated-device.rst
b/Documentation/driver-api/vfio-mediated-device.rst
index 25eb7d5b834b..0b7d2bf843b6 100644
--- a/Documentation/driver-api/vfio-mediated-device.rst
+++ b/Documentation/driver-api/vfio-mediated-device.rst
@@ -270,6 +270,7 @@ Directories and Files Under the sysfs for Each mdev Device
|--- remove
|--- mdev_type {link to its type}
|--- vendor-specific-attributes [optional]
+ |--- alias
* remove (write only)
@@ -281,6 +282,14 @@ Example::
# echo 1 > /sys/bus/mdev/devices/$mdev_UUID/remove
+* alias (read only, optional)
+Whenever a parent requested to generate an alias, each mdev device of such
+parent is assigned unique alias by the mdev core.
+This file shows the alias of the mdev device.
+
+Reading file either returns valid alias when assigned or returns error code
+-EOPNOTSUPP when unsupported.
+
Mediated device Hot plug
diff --git a/drivers/vfio/mdev/mdev_sysfs.c b/drivers/vfio/mdev/mdev_sysfs.c
index 43afe0e80b76..59f4e3cc5233 100644
--- a/drivers/vfio/mdev/mdev_sysfs.c
+++ b/drivers/vfio/mdev/mdev_sysfs.c
@@ -246,7 +246,20 @@ static ssize_t remove_store(struct device *dev, struct
device_attribute *attr,
static DEVICE_ATTR_WO(remove);
+static ssize_t alias_show(struct device *device,
+ struct device_attribute *attr, char *buf)
+{
+ struct mdev_device *dev = mdev_from_dev(device);
+
+ if (!dev->alias)
+ return -EOPNOTSUPP;
+
+ return sprintf(buf, "%s\n", dev->alias);
+}
+static DEVICE_ATTR_RO(alias);
+
static const struct attribute *mdev_device_attrs[] = {
+ _attr_alias.attr,
_attr_remove.attr,
NULL,
};
--
2.19.2
[PATCH v3 4/5] mdev: Introduce an API mdev_alias
Introduce an API mdev_alias() to provide access to optionally generated
alias.
Signed-off-by: Parav Pandit
---
drivers/vfio/mdev/mdev_core.c | 12
include/linux/mdev.h | 1 +
2 files changed, 13 insertions(+)
diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
index c8cd40366783..9eec556fbdd4 100644
--- a/drivers/vfio/mdev/mdev_core.c
+++ b/drivers/vfio/mdev/mdev_core.c
@@ -517,6 +517,18 @@ struct device *mdev_get_iommu_device(struct device *dev)
}
EXPORT_SYMBOL(mdev_get_iommu_device);
+/**
+ * mdev_alias: Return alias string of a mdev device
+ * @mdev: Pointer to the mdev device
+ * mdev_alias() returns alias string of a mdev device if alias is present,
+ * returns NULL otherwise.
+ */
+const char *mdev_alias(struct mdev_device *mdev)
+{
+ return mdev->alias;
+}
+EXPORT_SYMBOL(mdev_alias);
+
static int __init mdev_init(void)
{
int ret;
diff --git a/include/linux/mdev.h b/include/linux/mdev.h
index f036fe9854ee..6da82213bc4e 100644
--- a/include/linux/mdev.h
+++ b/include/linux/mdev.h
@@ -148,5 +148,6 @@ void mdev_unregister_driver(struct mdev_driver *drv);
struct device *mdev_parent_dev(struct mdev_device *mdev);
struct device *mdev_dev(struct mdev_device *mdev);
struct mdev_device *mdev_from_dev(struct device *dev);
+const char *mdev_alias(struct mdev_device *mdev);
#endif /* MDEV_H */
--
2.19.2
[PATCH v3 0/5] Introduce variable length mdev alias
To have consistent naming for the netdevice of a mdev and to have consistent naming of the devlink port [1] of a mdev, which is formed using phys_port_name of the devlink port, current UUID is not usable because UUID is too long. UUID in string format is 36-characters long and in binary 128-bit. Both formats are not able to fit within 15 characters limit of netdev name. It is desired to have mdev device naming consistent using UUID. So that widely used user space framework such as ovs [2] can make use of mdev representor in similar way as PCIe SR-IOV VF and PF representors. Hence, (a) mdev alias is created which is derived using sha1 from the mdev name. (b) Vendor driver describes how long an alias should be for the child mdev created for a given parent. (c) Mdev aliases are unique at system level. (d) alias is created optionally whenever parent requested. This ensures that non networking mdev parents can function without alias creation overhead. This design is discussed at [3]. An example systemd/udev extension will have, 1. netdev name created using mdev alias available in sysfs. mdev UUID=83b8f4f2-509f-382f-3c1e-e6bfe0fa1001 mdev 12 character alias=cd5b146a80a5 netdev name of this mdev = enmcd5b146a80a5 Here en = Ethernet link m = mediated device 2. devlink port phys_port_name created using mdev alias. devlink phys_port_name=pcd5b146a80a5 This patchset enables mdev core to maintain unique alias for a mdev. Patch-1 Introduces mdev alias using sha1. Patch-2 Ensures that mdev alias is unique in a system. Patch-3 Exposes mdev alias in a sysfs hirerchy, update Documentation Patch-4 Introduces mdev_alias() API. Patch-5 Extends mtty driver to optionally provide alias generation. This also enables to test UUID based sha1 collision and trigger error handling for duplicate sha1 results. [1] http://man7.org/linux/man-pages/man8/devlink-port.8.html [2] https://docs.openstack.org/os-vif/latest/user/plugins/ovs.html [3] https://patchwork.kernel.org/cover/11084231/ --- Changelog: v2->v3: - Addressed comment from Yunsheng Lin - Changed strcmp() ==0 to !strcmp() - Addressed comment from Cornelia Hunk - Merged sysfs Documentation patch with syfs patch - Added more description for alias return value v1->v2: - Corrected a typo from 'and' to 'an' - Addressed comments from Alex Williamson - Kept mdev_device naturally aligned - Added error checking for crypt_*() calls - Moved alias NULL check at beginning - Added mdev_alias() API - Updated mtty driver to show example mdev_alias() usage - Changed return type of generate_alias() from int to char* v0->v1: - Addressed comments from Alex Williamson, Cornelia Hunk and Mark Bloch - Moved alias length check outside of the parent lock - Moved alias and digest allocation from kvzalloc to kzalloc - [0] changed to alias - alias_length check is nested under get_alias_length callback check - Changed comments to start with an empty line - Added comment where alias memory ownership is handed over to mdev device - Fixed cleaunup of hash if mdev_bus_register() fails - Updated documentation for new sysfs alias file - Improved commit logs to make description more clear - Fixed inclusiong of alias for NULL check - Added ratelimited debug print for sha1 hash collision error Parav Pandit (5): mdev: Introduce sha1 based mdev alias mdev: Make mdev alias unique among all mdevs mdev: Expose mdev alias in sysfs tree mdev: Introduce an API mdev_alias mtty: Optionally support mtty alias .../driver-api/vfio-mediated-device.rst | 9 ++ drivers/vfio/mdev/mdev_core.c | 142 +- drivers/vfio/mdev/mdev_private.h | 5 +- drivers/vfio/mdev/mdev_sysfs.c| 26 +++- include/linux/mdev.h | 5 + samples/vfio-mdev/mtty.c | 13 ++ 6 files changed, 190 insertions(+), 10 deletions(-) -- 2.19.2
[PATCH v3 2/5] mdev: Make mdev alias unique among all mdevs
Mdev alias should be unique among all the mdevs, so that when such alias
is used by the mdev users to derive other objects, there is no
collision in a given system.
Signed-off-by: Parav Pandit
---
Changelog:
v2->v3:
- Changed strcmp() ==0 to !strcmp()
v1->v2:
- Moved alias NULL check at beginning
v0->v1:
- Fixed inclusiong of alias for NULL check
- Added ratelimited debug print for sha1 hash collision error
---
drivers/vfio/mdev/mdev_core.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
index 3bdff0469607..c8cd40366783 100644
--- a/drivers/vfio/mdev/mdev_core.c
+++ b/drivers/vfio/mdev/mdev_core.c
@@ -388,6 +388,13 @@ int mdev_device_create(struct kobject *kobj, struct device
*dev,
ret = -EEXIST;
goto mdev_fail;
}
+ if (alias && tmp->alias && !strcmp(alias, tmp->alias)) {
+ mutex_unlock(_list_lock);
+ ret = -EEXIST;
+ dev_dbg_ratelimited(dev, "Hash collision in alias
creation for UUID %pUl\n",
+ uuid);
+ goto mdev_fail;
+ }
}
mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
--
2.19.2
[PATCH v3 5/5] mtty: Optionally support mtty alias
Provide a module parameter to set alias length to optionally generate
mdev alias.
Example to request mdev alias.
$ modprobe mtty alias_length=12
Make use of mtty_alias() API when alias_length module parameter is set.
Signed-off-by: Parav Pandit
---
Changelog:
v1->v2:
- Added mdev_alias() usage sample
---
samples/vfio-mdev/mtty.c | 13 +
1 file changed, 13 insertions(+)
diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c
index 92e770a06ea2..075d65440bc0 100644
--- a/samples/vfio-mdev/mtty.c
+++ b/samples/vfio-mdev/mtty.c
@@ -150,6 +150,10 @@ static const struct file_operations vd_fops = {
.owner = THIS_MODULE,
};
+static unsigned int mtty_alias_length;
+module_param_named(alias_length, mtty_alias_length, uint, 0444);
+MODULE_PARM_DESC(alias_length, "mdev alias length; default=0");
+
/* function prototypes */
static int mtty_trigger_interrupt(const guid_t *uuid);
@@ -770,6 +774,9 @@ static int mtty_create(struct kobject *kobj, struct
mdev_device *mdev)
list_add(_state->next, _devices_list);
mutex_unlock(_list_lock);
+ if (mtty_alias_length)
+ dev_dbg(mdev_dev(mdev), "alias is %s\n", mdev_alias(mdev));
+
return 0;
}
@@ -1410,6 +1417,11 @@ static struct attribute_group *mdev_type_groups[] = {
NULL,
};
+static unsigned int mtty_get_alias_length(void)
+{
+ return mtty_alias_length;
+}
+
static const struct mdev_parent_ops mdev_fops = {
.owner = THIS_MODULE,
.dev_attr_groups= mtty_dev_groups,
@@ -1422,6 +1434,7 @@ static const struct mdev_parent_ops mdev_fops = {
.read = mtty_read,
.write = mtty_write,
.ioctl = mtty_ioctl,
+ .get_alias_length = mtty_get_alias_length
};
static void mtty_device_release(struct device *dev)
--
2.19.2
RE: [PATCH v2 1/6] mdev: Introduce sha1 based mdev alias
> -Original Message- > From: Cornelia Huck > Sent: Friday, August 30, 2019 7:32 PM > To: Parav Pandit > Cc: alex.william...@redhat.com; Jiri Pirko ; > kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org; linux- > ker...@vger.kernel.org; net...@vger.kernel.org > Subject: Re: [PATCH v2 1/6] mdev: Introduce sha1 based mdev alias > > On Fri, 30 Aug 2019 12:58:04 + > Parav Pandit wrote: > > > > -Original Message- > > > From: Cornelia Huck > > > Sent: Friday, August 30, 2019 6:09 PM > > > To: Parav Pandit > > > Cc: alex.william...@redhat.com; Jiri Pirko ; > > > kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org; > > > linux- ker...@vger.kernel.org; net...@vger.kernel.org > > > Subject: Re: [PATCH v2 1/6] mdev: Introduce sha1 based mdev alias > > > > > > On Fri, 30 Aug 2019 12:33:22 + > > > Parav Pandit wrote: > > > > > > > > -Original Message- > > > > > From: Cornelia Huck > > > > > Sent: Friday, August 30, 2019 2:47 PM > > > > > To: Parav Pandit > > > > > Cc: alex.william...@redhat.com; Jiri Pirko ; > > > > > kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org; > > > > > linux- ker...@vger.kernel.org; net...@vger.kernel.org > > > > > Subject: Re: [PATCH v2 1/6] mdev: Introduce sha1 based mdev > > > > > alias > > > > > > > > > > On Thu, 29 Aug 2019 06:18:59 -0500 Parav Pandit > > > > > wrote: > > > > > > > > > > > Some vendor drivers want an identifier for an mdev device that > > > > > > is shorter than the UUID, due to length restrictions in the > > > > > > consumers of that identifier. > > > > > > > > > > > > Add a callback that allows a vendor driver to request an alias > > > > > > of a specified length to be generated for an mdev device. If > > > > > > generated, that alias is checked for collisions. > > > > > > > > > > > > It is an optional attribute. > > > > > > mdev alias is generated using sha1 from the mdev name. > > > > > > > > > > > > Signed-off-by: Parav Pandit > > > > > > > > > > > > --- > > > > > > Changelog: > > > > > > v1->v2: > > > > > > - Kept mdev_device naturally aligned > > > > > > - Added error checking for crypt_*() calls > > > > > > - Corrected a typo from 'and' to 'an' > > > > > > - Changed return type of generate_alias() from int to char* > > > > > > v0->v1: > > > > > > - Moved alias length check outside of the parent lock > > > > > > - Moved alias and digest allocation from kvzalloc to kzalloc > > > > > > - [0] changed to alias > > > > > > - alias_length check is nested under get_alias_length > > > > > > callback check > > > > > > - Changed comments to start with an empty line > > > > > > - Fixed cleaunup of hash if mdev_bus_register() fails > > > > > > - Added comment where alias memory ownership is handed over > > > > > > to mdev device > > > > > > - Updated commit log to indicate motivation for this feature > > > > > > --- > > > > > > drivers/vfio/mdev/mdev_core.c| 123 > > > > > ++- > > > > > > drivers/vfio/mdev/mdev_private.h | 5 +- > > > > > > drivers/vfio/mdev/mdev_sysfs.c | 13 ++-- > > > > > > include/linux/mdev.h | 4 + > > > > > > 4 files changed, 135 insertions(+), 10 deletions(-) > > > > > > > > ...and detached from the local variable here. Who is freeing it? > > > > > The comment states that it is done by the mdev, but I don't see it? > > > > > > > > > mdev_device_free() frees it. > > > > > > Ah yes, I overlooked the kfree(). > > > > > > > once its assigned to mdev, mdev is the owner of it. > > > > > > > > > This detour via the local variable looks weird to me. Can you > > > > > either create the alias directly in the mdev (would need to > > > > > happen later in the function, but I'm not sure why you generate > > > > > the alias before check
RE: [PATCH v2 5/6] mdev: Update sysfs documentation
> -Original Message-
> From: Cornelia Huck
> Sent: Friday, August 30, 2019 6:19 PM
> To: Parav Pandit
> Cc: alex.william...@redhat.com; Jiri Pirko ;
> kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org; linux-
> ker...@vger.kernel.org; net...@vger.kernel.org
> Subject: Re: [PATCH v2 5/6] mdev: Update sysfs documentation
>
> On Thu, 29 Aug 2019 06:19:03 -0500
> Parav Pandit wrote:
>
> > Updated documentation for optional read only sysfs attribute.
>
> I'd probably merge this into the patch introducing the attribute.
>
Ok. I will spin v3.
> >
> > Signed-off-by: Parav Pandit
> > ---
> > Documentation/driver-api/vfio-mediated-device.rst | 5 +
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/Documentation/driver-api/vfio-mediated-device.rst
> > b/Documentation/driver-api/vfio-mediated-device.rst
> > index 25eb7d5b834b..0ab03d3f5629 100644
> > --- a/Documentation/driver-api/vfio-mediated-device.rst
> > +++ b/Documentation/driver-api/vfio-mediated-device.rst
> > @@ -270,6 +270,7 @@ Directories and Files Under the sysfs for Each mdev
> Device
> > |--- remove
> > |--- mdev_type {link to its type}
> > |--- vendor-specific-attributes [optional]
> > + |--- alias [optional]
>
> "optional" implies "not always present" to me, not "might return a read error
> if
> not available". Don't know if there's a better way to tag this? Or make it
> really
> optional? :)
May be write it as,
alias [ optional when requested by parent ]
>
> >
> > * remove (write only)
> >
> > @@ -281,6 +282,10 @@ Example::
> >
> > # echo 1 > /sys/bus/mdev/devices/$mdev_UUID/remove
> >
> > +* alias (read only)
> > +Whenever a parent requested to generate an alias, each mdev is
> > +assigned a unique alias by the mdev core. This file shows the alias of the
> mdev device.
>
> It's not really the parent, but the vendor driver requesting this, right?
> Also,
At mdev level, it only knows parent->ops structure, whether parent is
registered by vendor driver or something else.
> "each mdev" is a bit ambiguous,
It is in context of the parent. Sentence is not starting with "each mdev".
But may be more verbosely written as,
Whenever a parent requested to generate an alias, Each mdev device of such
parent is assigned
unique alias by the mdev core. This file shows the alias of the mdev device.
> created via that driver. Lastly, if we stick with the "returns an error if not
> implemented" approach, that should also be mentioned here.
Ok. Will spin v3 to describe it.
>
> > +
> > Mediated device Hot plug
> >
> >
RE: [PATCH v2 2/6] mdev: Make mdev alias unique among all mdevs
> -Original Message-
> From: Cornelia Huck
> Sent: Friday, August 30, 2019 6:11 PM
> To: Parav Pandit
> Cc: alex.william...@redhat.com; Jiri Pirko ;
> kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org; linux-
> ker...@vger.kernel.org; net...@vger.kernel.org
> Subject: Re: [PATCH v2 2/6] mdev: Make mdev alias unique among all mdevs
>
> On Thu, 29 Aug 2019 06:19:00 -0500
> Parav Pandit wrote:
>
> > Mdev alias should be unique among all the mdevs, so that when such
> > alias is used by the mdev users to derive other objects, there is no
> > collision in a given system.
> >
> > Signed-off-by: Parav Pandit
> >
> > ---
> > Changelog:
> > v1->v2:
> > - Moved alias NULL check at beginning
> > v0->v1:
> > - Fixed inclusiong of alias for NULL check
> > - Added ratelimited debug print for sha1 hash collision error
> > ---
> > drivers/vfio/mdev/mdev_core.c | 7 +++
> > 1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/vfio/mdev/mdev_core.c
> > b/drivers/vfio/mdev/mdev_core.c index 3bdff0469607..c9bf2ac362b9
> > 100644
> > --- a/drivers/vfio/mdev/mdev_core.c
> > +++ b/drivers/vfio/mdev/mdev_core.c
> > @@ -388,6 +388,13 @@ int mdev_device_create(struct kobject *kobj, struct
> device *dev,
> > ret = -EEXIST;
> > goto mdev_fail;
> > }
> > + if (alias && tmp->alias && strcmp(alias, tmp->alias) == 0) {
> > + mutex_unlock(_list_lock);
> > + ret = -EEXIST;
> > + dev_dbg_ratelimited(dev, "Hash collision in alias
> creation for UUID %pUl\n",
> > + uuid);
> > + goto mdev_fail;
> > + }
> > }
> >
> > mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
>
> Any reason not to merge this into the first patch?
No. It surely can be merged. Its easy to start with smaller patches instead of
splitting. :-)
Doing uniqueness comparison was easy to split as independent functionality, so
did as 2nd patch.
But either way is ok.
RE: [PATCH v2 1/6] mdev: Introduce sha1 based mdev alias
> -Original Message- > From: Cornelia Huck > Sent: Friday, August 30, 2019 6:09 PM > To: Parav Pandit > Cc: alex.william...@redhat.com; Jiri Pirko ; > kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org; linux- > ker...@vger.kernel.org; net...@vger.kernel.org > Subject: Re: [PATCH v2 1/6] mdev: Introduce sha1 based mdev alias > > On Fri, 30 Aug 2019 12:33:22 + > Parav Pandit wrote: > > > > -Original Message- > > > From: Cornelia Huck > > > Sent: Friday, August 30, 2019 2:47 PM > > > To: Parav Pandit > > > Cc: alex.william...@redhat.com; Jiri Pirko ; > > > kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org; > > > linux- ker...@vger.kernel.org; net...@vger.kernel.org > > > Subject: Re: [PATCH v2 1/6] mdev: Introduce sha1 based mdev alias > > > > > > On Thu, 29 Aug 2019 06:18:59 -0500 > > > Parav Pandit wrote: > > > > > > > Some vendor drivers want an identifier for an mdev device that is > > > > shorter than the UUID, due to length restrictions in the consumers > > > > of that identifier. > > > > > > > > Add a callback that allows a vendor driver to request an alias of > > > > a specified length to be generated for an mdev device. If > > > > generated, that alias is checked for collisions. > > > > > > > > It is an optional attribute. > > > > mdev alias is generated using sha1 from the mdev name. > > > > > > > > Signed-off-by: Parav Pandit > > > > > > > > --- > > > > Changelog: > > > > v1->v2: > > > > - Kept mdev_device naturally aligned > > > > - Added error checking for crypt_*() calls > > > > - Corrected a typo from 'and' to 'an' > > > > - Changed return type of generate_alias() from int to char* > > > > v0->v1: > > > > - Moved alias length check outside of the parent lock > > > > - Moved alias and digest allocation from kvzalloc to kzalloc > > > > - [0] changed to alias > > > > - alias_length check is nested under get_alias_length callback > > > > check > > > > - Changed comments to start with an empty line > > > > - Fixed cleaunup of hash if mdev_bus_register() fails > > > > - Added comment where alias memory ownership is handed over to > > > > mdev device > > > > - Updated commit log to indicate motivation for this feature > > > > --- > > > > drivers/vfio/mdev/mdev_core.c| 123 > > > ++- > > > > drivers/vfio/mdev/mdev_private.h | 5 +- > > > > drivers/vfio/mdev/mdev_sysfs.c | 13 ++-- > > > > include/linux/mdev.h | 4 + > > > > 4 files changed, 135 insertions(+), 10 deletions(-) > > > > ...and detached from the local variable here. Who is freeing it? The > > > comment states that it is done by the mdev, but I don't see it? > > > > > mdev_device_free() frees it. > > Ah yes, I overlooked the kfree(). > > > once its assigned to mdev, mdev is the owner of it. > > > > > This detour via the local variable looks weird to me. Can you either > > > create the alias directly in the mdev (would need to happen later in > > > the function, but I'm not sure why you generate the alias before > > > checking for duplicates anyway), or do an explicit copy? > > Alias duplicate check is done after generating it, because duplicate alias > > are > not allowed. > > The probability of collision is rare. > > So it is speculatively generated without hold the lock, because there is no > need to hold the lock. > > It is compared along with guid while mutex lock is held in single loop. > > And if it is duplicate, there is no need to allocate mdev. > > > > It will be sub optimal to run through the mdev list 2nd time after mdev > creation and after generating alias for duplicate check. > > Ok, but what about copying it? I find this "set local variable to NULL after > ownership is transferred" pattern a bit unintuitive. Copying it to the mdev > (and > then unconditionally freeing it) looks more obvious to me. Its not unconditionally freed. Its freed in the error unwinding path. I think its ok along with the comment that describes this error path area.
RE: [PATCH v2 1/6] mdev: Introduce sha1 based mdev alias
> -Original Message-
> From: Cornelia Huck
> Sent: Friday, August 30, 2019 2:47 PM
> To: Parav Pandit
> Cc: alex.william...@redhat.com; Jiri Pirko ;
> kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org; linux-
> ker...@vger.kernel.org; net...@vger.kernel.org
> Subject: Re: [PATCH v2 1/6] mdev: Introduce sha1 based mdev alias
>
> On Thu, 29 Aug 2019 06:18:59 -0500
> Parav Pandit wrote:
>
> > Some vendor drivers want an identifier for an mdev device that is
> > shorter than the UUID, due to length restrictions in the consumers of
> > that identifier.
> >
> > Add a callback that allows a vendor driver to request an alias of a
> > specified length to be generated for an mdev device. If generated,
> > that alias is checked for collisions.
> >
> > It is an optional attribute.
> > mdev alias is generated using sha1 from the mdev name.
> >
> > Signed-off-by: Parav Pandit
> >
> > ---
> > Changelog:
> > v1->v2:
> > - Kept mdev_device naturally aligned
> > - Added error checking for crypt_*() calls
> > - Corrected a typo from 'and' to 'an'
> > - Changed return type of generate_alias() from int to char*
> > v0->v1:
> > - Moved alias length check outside of the parent lock
> > - Moved alias and digest allocation from kvzalloc to kzalloc
> > - [0] changed to alias
> > - alias_length check is nested under get_alias_length callback check
> > - Changed comments to start with an empty line
> > - Fixed cleaunup of hash if mdev_bus_register() fails
> > - Added comment where alias memory ownership is handed over to mdev
> > device
> > - Updated commit log to indicate motivation for this feature
> > ---
> > drivers/vfio/mdev/mdev_core.c| 123
> ++-
> > drivers/vfio/mdev/mdev_private.h | 5 +-
> > drivers/vfio/mdev/mdev_sysfs.c | 13 ++--
> > include/linux/mdev.h | 4 +
> > 4 files changed, 135 insertions(+), 10 deletions(-)
> >
>
> (...)
>
> > +static const char *
> > +generate_alias(const char *uuid, unsigned int max_alias_len) {
> > + struct shash_desc *hash_desc;
> > + unsigned int digest_size;
> > + unsigned char *digest;
> > + unsigned int alias_len;
> > + char *alias;
> > + int ret;
> > +
> > + /*
> > +* Align to multiple of 2 as bin2hex will generate
> > +* even number of bytes.
> > +*/
> > + alias_len = roundup(max_alias_len, 2);
> > + alias = kzalloc(alias_len + 1, GFP_KERNEL);
>
> This function allocates alias...
>
> > + if (!alias)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + /* Allocate and init descriptor */
> > + hash_desc = kvzalloc(sizeof(*hash_desc) +
> > +crypto_shash_descsize(alias_hash),
> > +GFP_KERNEL);
> > + if (!hash_desc) {
> > + ret = -ENOMEM;
> > + goto desc_err;
> > + }
> > +
> > + hash_desc->tfm = alias_hash;
> > +
> > + digest_size = crypto_shash_digestsize(alias_hash);
> > +
> > + digest = kzalloc(digest_size, GFP_KERNEL);
> > + if (!digest) {
> > + ret = -ENOMEM;
> > + goto digest_err;
> > + }
> > + ret = crypto_shash_init(hash_desc);
> > + if (ret)
> > + goto hash_err;
> > +
> > + ret = crypto_shash_update(hash_desc, uuid, UUID_STRING_LEN);
> > + if (ret)
> > + goto hash_err;
> > +
> > + ret = crypto_shash_final(hash_desc, digest);
> > + if (ret)
> > + goto hash_err;
> > +
> > + bin2hex(alias, digest, min_t(unsigned int, digest_size, alias_len / 2));
> > + /*
> > +* When alias length is odd, zero out an additional last byte
> > +* that bin2hex has copied.
> > +*/
> > + if (max_alias_len % 2)
> > + alias[max_alias_len] = 0;
> > +
> > + kfree(digest);
> > + kvfree(hash_desc);
> > + return alias;
>
> ...and returns it here on success...
>
> > +
> > +hash_err:
> > + kfree(digest);
> > +digest_err:
> > + kvfree(hash_desc);
> > +desc_err:
> > + kfree(alias);
> > + return ERR_PTR(ret);
> > +}
> > +
> > +int mdev_device_create(struct kobject *kobj, struct device *dev,
> > + const char *uuid_str, const guid_t *uuid)
> > {
> > int ret;
> > struct mdev_device *mdev, *tmp;
> > struct mdev_parent *parent;
> >
RE: [PATCH internal net-next 0/2] Minor refactor in devlink
> -Original Message- > From: linux-kernel-ow...@vger.kernel.org ow...@vger.kernel.org> On Behalf Of Parav Pandit > Sent: Friday, August 30, 2019 9:41 AM > To: linux-kernel@vger.kernel.org; Jiri Pirko > Cc: Parav Pandit > Subject: [PATCH internal net-next 0/2] Minor refactor in devlink > > Two minor refactors in devlink. > > Patch-1 Explicitly defines devlink port index as unsigned int > Patch-2 Uses switch-case to handle different port flavours attributes > > Parav Pandit (2): > devlink: Make port index data type as unsigned int > devlink: Use switch-case instead of if-else > > include/net/devlink.h | 2 +- > net/core/devlink.c| 44 --- > 2 files changed, 26 insertions(+), 20 deletions(-) > > -- > 2.19.2 I am sorry for noise. By mistake send to wrong list.
[PATCH internal net-next 0/2] Minor refactor in devlink
Two minor refactors in devlink. Patch-1 Explicitly defines devlink port index as unsigned int Patch-2 Uses switch-case to handle different port flavours attributes Parav Pandit (2): devlink: Make port index data type as unsigned int devlink: Use switch-case instead of if-else include/net/devlink.h | 2 +- net/core/devlink.c| 44 --- 2 files changed, 26 insertions(+), 20 deletions(-) -- 2.19.2
[PATCH internal net-next 1/2] devlink: Make port index data type as unsigned int
Devlink port index attribute is returned to users as u32 through
netlink response.
Change index data type from 'unsigned' to 'unsigned int' to avoid any
size ambiguity and to avoid below checkpatch.pl warning.
WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
81: FILE: include/net/devlink.h:81:
+ unsigned index;
Signed-off-by: Parav Pandit
---
include/net/devlink.h | 2 +-
net/core/devlink.c| 5 +++--
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/include/net/devlink.h b/include/net/devlink.h
index 7f43c48f54cd..13523b0a0642 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -75,7 +75,7 @@ struct devlink_port {
struct list_head list;
struct list_head param_list;
struct devlink *devlink;
- unsigned index;
+ unsigned int index;
bool registered;
spinlock_t type_lock; /* Protects type and type_dev
* pointer consistency.
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 650f36379203..b7091329987a 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -136,7 +136,7 @@ static struct devlink *devlink_get_from_info(struct
genl_info *info)
}
static struct devlink_port *devlink_port_get_by_index(struct devlink *devlink,
- int port_index)
+ unsigned int port_index)
{
struct devlink_port *devlink_port;
@@ -147,7 +147,8 @@ static struct devlink_port
*devlink_port_get_by_index(struct devlink *devlink,
return NULL;
}
-static bool devlink_port_index_exists(struct devlink *devlink, int port_index)
+static bool devlink_port_index_exists(struct devlink *devlink,
+ unsigned int port_index)
{
return devlink_port_get_by_index(devlink, port_index);
}
--
2.19.2
[PATCH internal net-next 2/2] devlink: Use switch-case instead of if-else
Make core more readable with switch-case for various port flavours.
Signed-off-by: Parav Pandit
---
net/core/devlink.c | 39 ++-
1 file changed, 22 insertions(+), 17 deletions(-)
diff --git a/net/core/devlink.c b/net/core/devlink.c
index b7091329987a..6e52d639dac6 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -510,32 +510,37 @@ static int devlink_nl_port_attrs_put(struct sk_buff *msg,
return 0;
if (nla_put_u16(msg, DEVLINK_ATTR_PORT_FLAVOUR, attrs->flavour))
return -EMSGSIZE;
- if (devlink_port->attrs.flavour == DEVLINK_PORT_FLAVOUR_PCI_PF) {
+ switch (devlink_port->attrs.flavour) {
+ case DEVLINK_PORT_FLAVOUR_PCI_PF:
if (nla_put_u16(msg, DEVLINK_ATTR_PORT_PCI_PF_NUMBER,
attrs->pci_pf.pf))
return -EMSGSIZE;
- } else if (devlink_port->attrs.flavour == DEVLINK_PORT_FLAVOUR_PCI_VF) {
+ break;
+ case DEVLINK_PORT_FLAVOUR_PCI_VF:
if (nla_put_u16(msg, DEVLINK_ATTR_PORT_PCI_PF_NUMBER,
attrs->pci_vf.pf) ||
nla_put_u16(msg, DEVLINK_ATTR_PORT_PCI_VF_NUMBER,
attrs->pci_vf.vf))
return -EMSGSIZE;
+ break;
+ case DEVLINK_PORT_FLAVOUR_PHYSICAL:
+ case DEVLINK_PORT_FLAVOUR_CPU:
+ case DEVLINK_PORT_FLAVOUR_DSA:
+ if (nla_put_u32(msg, DEVLINK_ATTR_PORT_NUMBER,
+ attrs->phys.port_number))
+ return -EMSGSIZE;
+ if (!attrs->split)
+ return 0;
+ if (nla_put_u32(msg, DEVLINK_ATTR_PORT_SPLIT_GROUP,
+ attrs->phys.port_number))
+ return -EMSGSIZE;
+ if (nla_put_u32(msg, DEVLINK_ATTR_PORT_SPLIT_SUBPORT_NUMBER,
+ attrs->phys.split_subport_number))
+ return -EMSGSIZE;
+ break;
+ default:
+ break;
}
- if (devlink_port->attrs.flavour != DEVLINK_PORT_FLAVOUR_PHYSICAL &&
- devlink_port->attrs.flavour != DEVLINK_PORT_FLAVOUR_CPU &&
- devlink_port->attrs.flavour != DEVLINK_PORT_FLAVOUR_DSA)
- return 0;
- if (nla_put_u32(msg, DEVLINK_ATTR_PORT_NUMBER,
- attrs->phys.port_number))
- return -EMSGSIZE;
- if (!attrs->split)
- return 0;
- if (nla_put_u32(msg, DEVLINK_ATTR_PORT_SPLIT_GROUP,
- attrs->phys.port_number))
- return -EMSGSIZE;
- if (nla_put_u32(msg, DEVLINK_ATTR_PORT_SPLIT_SUBPORT_NUMBER,
- attrs->phys.split_subport_number))
- return -EMSGSIZE;
return 0;
}
--
2.19.2
RE: [PATCH v2 1/6] mdev: Introduce sha1 based mdev alias
> -Original Message-
> From: Yunsheng Lin
> Sent: Thursday, August 29, 2019 5:57 PM
> To: Parav Pandit ; alex.william...@redhat.com; Jiri
> Pirko ; kwankh...@nvidia.com; coh...@redhat.com;
> da...@davemloft.net
> Cc: k...@vger.kernel.org; linux-kernel@vger.kernel.org;
> net...@vger.kernel.org
> Subject: Re: [PATCH v2 1/6] mdev: Introduce sha1 based mdev alias
>
> On 2019/8/29 19:18, Parav Pandit wrote:
> > Some vendor drivers want an identifier for an mdev device that is
> > shorter than the UUID, due to length restrictions in the consumers of
> > that identifier.
> >
> > Add a callback that allows a vendor driver to request an alias of a
> > specified length to be generated for an mdev device. If generated,
> > that alias is checked for collisions.
> >
> > It is an optional attribute.
> > mdev alias is generated using sha1 from the mdev name.
> >
> > Signed-off-by: Parav Pandit
> >
> > ---
> > Changelog:
> > v1->v2:
> > - Kept mdev_device naturally aligned
> > - Added error checking for crypt_*() calls
> > - Corrected a typo from 'and' to 'an'
> > - Changed return type of generate_alias() from int to char*
> > v0->v1:
> > - Moved alias length check outside of the parent lock
> > - Moved alias and digest allocation from kvzalloc to kzalloc
> > - [0] changed to alias
> > - alias_length check is nested under get_alias_length callback check
> > - Changed comments to start with an empty line
> > - Fixed cleaunup of hash if mdev_bus_register() fails
> > - Added comment where alias memory ownership is handed over to mdev
> > device
> > - Updated commit log to indicate motivation for this feature
> > ---
> > drivers/vfio/mdev/mdev_core.c| 123
> ++-
> > drivers/vfio/mdev/mdev_private.h | 5 +-
> > drivers/vfio/mdev/mdev_sysfs.c | 13 ++--
> > include/linux/mdev.h | 4 +
> > 4 files changed, 135 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/vfio/mdev/mdev_core.c
> > b/drivers/vfio/mdev/mdev_core.c index b558d4cfd082..3bdff0469607
> > 100644
> > --- a/drivers/vfio/mdev/mdev_core.c
> > +++ b/drivers/vfio/mdev/mdev_core.c
> > @@ -10,9 +10,11 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > +#include
> >
> > #include "mdev_private.h"
> >
> > @@ -27,6 +29,8 @@ static struct class_compat *mdev_bus_compat_class;
> > static LIST_HEAD(mdev_list); static DEFINE_MUTEX(mdev_list_lock);
> >
> > +static struct crypto_shash *alias_hash;
> > +
> > struct device *mdev_parent_dev(struct mdev_device *mdev) {
> > return mdev->parent->dev;
> > @@ -150,6 +154,16 @@ int mdev_register_device(struct device *dev, const
> struct mdev_parent_ops *ops)
> > if (!ops || !ops->create || !ops->remove || !ops-
> >supported_type_groups)
> > return -EINVAL;
> >
> > + if (ops->get_alias_length) {
> > + unsigned int digest_size;
> > + unsigned int aligned_len;
> > +
> > + aligned_len = roundup(ops->get_alias_length(), 2);
> > + digest_size = crypto_shash_digestsize(alias_hash);
> > + if (aligned_len / 2 > digest_size)
> > + return -EINVAL;
> > + }
> > +
> > dev = get_device(dev);
> > if (!dev)
> > return -EINVAL;
> > @@ -259,6 +273,7 @@ static void mdev_device_free(struct mdev_device
> *mdev)
> > mutex_unlock(_list_lock);
> >
> > dev_dbg(>dev, "MDEV: destroying\n");
> > + kfree(mdev->alias);
> > kfree(mdev);
> > }
> >
> > @@ -269,18 +284,101 @@ static void mdev_device_release(struct device
> *dev)
> > mdev_device_free(mdev);
> > }
> >
> > -int mdev_device_create(struct kobject *kobj,
> > - struct device *dev, const guid_t *uuid)
> > +static const char *
> > +generate_alias(const char *uuid, unsigned int max_alias_len) {
> > + struct shash_desc *hash_desc;
> > + unsigned int digest_size;
> > + unsigned char *digest;
> > + unsigned int alias_len;
> > + char *alias;
> > + int ret;
> > +
> > + /*
> > +* Align to multiple of 2 as bin2hex will generate
> > +* even number of bytes.
> > +*/
> > + alias_len = roundup(max_alias_len, 2);
> > + alias = kzalloc(alias_len + 1, GFP_KERNEL);
>
> It seems the mtty_alias_length in mtty.c can be set from module parameter,
> and user can set a very large number, maybe limit the max of the alias_len
> before calling kzalloc?
This is already guarded in mdev_register_device().
User cannot request alias length bigger than the digest size of sha1 (which is
20 bytes).
[PATCH v2 1/6] mdev: Introduce sha1 based mdev alias
Some vendor drivers want an identifier for an mdev device that is
shorter than the UUID, due to length restrictions in the consumers of
that identifier.
Add a callback that allows a vendor driver to request an alias of a
specified length to be generated for an mdev device. If generated,
that alias is checked for collisions.
It is an optional attribute.
mdev alias is generated using sha1 from the mdev name.
Signed-off-by: Parav Pandit
---
Changelog:
v1->v2:
- Kept mdev_device naturally aligned
- Added error checking for crypt_*() calls
- Corrected a typo from 'and' to 'an'
- Changed return type of generate_alias() from int to char*
v0->v1:
- Moved alias length check outside of the parent lock
- Moved alias and digest allocation from kvzalloc to kzalloc
- [0] changed to alias
- alias_length check is nested under get_alias_length callback check
- Changed comments to start with an empty line
- Fixed cleaunup of hash if mdev_bus_register() fails
- Added comment where alias memory ownership is handed over to mdev device
- Updated commit log to indicate motivation for this feature
---
drivers/vfio/mdev/mdev_core.c| 123 ++-
drivers/vfio/mdev/mdev_private.h | 5 +-
drivers/vfio/mdev/mdev_sysfs.c | 13 ++--
include/linux/mdev.h | 4 +
4 files changed, 135 insertions(+), 10 deletions(-)
diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
index b558d4cfd082..3bdff0469607 100644
--- a/drivers/vfio/mdev/mdev_core.c
+++ b/drivers/vfio/mdev/mdev_core.c
@@ -10,9 +10,11 @@
#include
#include
#include
+#include
#include
#include
#include
+#include
#include "mdev_private.h"
@@ -27,6 +29,8 @@ static struct class_compat *mdev_bus_compat_class;
static LIST_HEAD(mdev_list);
static DEFINE_MUTEX(mdev_list_lock);
+static struct crypto_shash *alias_hash;
+
struct device *mdev_parent_dev(struct mdev_device *mdev)
{
return mdev->parent->dev;
@@ -150,6 +154,16 @@ int mdev_register_device(struct device *dev, const struct
mdev_parent_ops *ops)
if (!ops || !ops->create || !ops->remove || !ops->supported_type_groups)
return -EINVAL;
+ if (ops->get_alias_length) {
+ unsigned int digest_size;
+ unsigned int aligned_len;
+
+ aligned_len = roundup(ops->get_alias_length(), 2);
+ digest_size = crypto_shash_digestsize(alias_hash);
+ if (aligned_len / 2 > digest_size)
+ return -EINVAL;
+ }
+
dev = get_device(dev);
if (!dev)
return -EINVAL;
@@ -259,6 +273,7 @@ static void mdev_device_free(struct mdev_device *mdev)
mutex_unlock(_list_lock);
dev_dbg(>dev, "MDEV: destroying\n");
+ kfree(mdev->alias);
kfree(mdev);
}
@@ -269,18 +284,101 @@ static void mdev_device_release(struct device *dev)
mdev_device_free(mdev);
}
-int mdev_device_create(struct kobject *kobj,
- struct device *dev, const guid_t *uuid)
+static const char *
+generate_alias(const char *uuid, unsigned int max_alias_len)
+{
+ struct shash_desc *hash_desc;
+ unsigned int digest_size;
+ unsigned char *digest;
+ unsigned int alias_len;
+ char *alias;
+ int ret;
+
+ /*
+* Align to multiple of 2 as bin2hex will generate
+* even number of bytes.
+*/
+ alias_len = roundup(max_alias_len, 2);
+ alias = kzalloc(alias_len + 1, GFP_KERNEL);
+ if (!alias)
+ return ERR_PTR(-ENOMEM);
+
+ /* Allocate and init descriptor */
+ hash_desc = kvzalloc(sizeof(*hash_desc) +
+crypto_shash_descsize(alias_hash),
+GFP_KERNEL);
+ if (!hash_desc) {
+ ret = -ENOMEM;
+ goto desc_err;
+ }
+
+ hash_desc->tfm = alias_hash;
+
+ digest_size = crypto_shash_digestsize(alias_hash);
+
+ digest = kzalloc(digest_size, GFP_KERNEL);
+ if (!digest) {
+ ret = -ENOMEM;
+ goto digest_err;
+ }
+ ret = crypto_shash_init(hash_desc);
+ if (ret)
+ goto hash_err;
+
+ ret = crypto_shash_update(hash_desc, uuid, UUID_STRING_LEN);
+ if (ret)
+ goto hash_err;
+
+ ret = crypto_shash_final(hash_desc, digest);
+ if (ret)
+ goto hash_err;
+
+ bin2hex(alias, digest, min_t(unsigned int, digest_size, alias_len / 2));
+ /*
+* When alias length is odd, zero out an additional last byte
+* that bin2hex has copied.
+*/
+ if (max_alias_len % 2)
+ alias[max_alias_len] = 0;
+
+ kfree(digest);
+ kvfree(hash_desc);
+ return alias;
+
+hash_err:
+ kfree(digest);
+digest_err:
+ kvfree(hash_desc);
+desc_err:
+ kfree(alias);
+ retur
[PATCH v2 2/6] mdev: Make mdev alias unique among all mdevs
Mdev alias should be unique among all the mdevs, so that when such alias
is used by the mdev users to derive other objects, there is no
collision in a given system.
Signed-off-by: Parav Pandit
---
Changelog:
v1->v2:
- Moved alias NULL check at beginning
v0->v1:
- Fixed inclusiong of alias for NULL check
- Added ratelimited debug print for sha1 hash collision error
---
drivers/vfio/mdev/mdev_core.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
index 3bdff0469607..c9bf2ac362b9 100644
--- a/drivers/vfio/mdev/mdev_core.c
+++ b/drivers/vfio/mdev/mdev_core.c
@@ -388,6 +388,13 @@ int mdev_device_create(struct kobject *kobj, struct device
*dev,
ret = -EEXIST;
goto mdev_fail;
}
+ if (alias && tmp->alias && strcmp(alias, tmp->alias) == 0) {
+ mutex_unlock(_list_lock);
+ ret = -EEXIST;
+ dev_dbg_ratelimited(dev, "Hash collision in alias
creation for UUID %pUl\n",
+ uuid);
+ goto mdev_fail;
+ }
}
mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
--
2.19.2
[PATCH v2 5/6] mdev: Update sysfs documentation
Updated documentation for optional read only sysfs attribute.
Signed-off-by: Parav Pandit
---
Documentation/driver-api/vfio-mediated-device.rst | 5 +
1 file changed, 5 insertions(+)
diff --git a/Documentation/driver-api/vfio-mediated-device.rst
b/Documentation/driver-api/vfio-mediated-device.rst
index 25eb7d5b834b..0ab03d3f5629 100644
--- a/Documentation/driver-api/vfio-mediated-device.rst
+++ b/Documentation/driver-api/vfio-mediated-device.rst
@@ -270,6 +270,7 @@ Directories and Files Under the sysfs for Each mdev Device
|--- remove
|--- mdev_type {link to its type}
|--- vendor-specific-attributes [optional]
+ |--- alias [optional]
* remove (write only)
@@ -281,6 +282,10 @@ Example::
# echo 1 > /sys/bus/mdev/devices/$mdev_UUID/remove
+* alias (read only)
+Whenever a parent requested to generate an alias, each mdev is assigned a
unique
+alias by the mdev core. This file shows the alias of the mdev device.
+
Mediated device Hot plug
--
2.19.2
[PATCH v2 6/6] mtty: Optionally support mtty alias
Provide a module parameter to set alias length to optionally generate
mdev alias.
Example to request mdev alias.
$ modprobe mtty alias_length=12
Make use of mtty_alias() API when alias_length module parameter is set.
Signed-off-by: Parav Pandit
---
Changelog:
v1->v2:
- Added mdev_alias() usage sample
---
samples/vfio-mdev/mtty.c | 13 +
1 file changed, 13 insertions(+)
diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c
index 92e770a06ea2..075d65440bc0 100644
--- a/samples/vfio-mdev/mtty.c
+++ b/samples/vfio-mdev/mtty.c
@@ -150,6 +150,10 @@ static const struct file_operations vd_fops = {
.owner = THIS_MODULE,
};
+static unsigned int mtty_alias_length;
+module_param_named(alias_length, mtty_alias_length, uint, 0444);
+MODULE_PARM_DESC(alias_length, "mdev alias length; default=0");
+
/* function prototypes */
static int mtty_trigger_interrupt(const guid_t *uuid);
@@ -770,6 +774,9 @@ static int mtty_create(struct kobject *kobj, struct
mdev_device *mdev)
list_add(_state->next, _devices_list);
mutex_unlock(_list_lock);
+ if (mtty_alias_length)
+ dev_dbg(mdev_dev(mdev), "alias is %s\n", mdev_alias(mdev));
+
return 0;
}
@@ -1410,6 +1417,11 @@ static struct attribute_group *mdev_type_groups[] = {
NULL,
};
+static unsigned int mtty_get_alias_length(void)
+{
+ return mtty_alias_length;
+}
+
static const struct mdev_parent_ops mdev_fops = {
.owner = THIS_MODULE,
.dev_attr_groups= mtty_dev_groups,
@@ -1422,6 +1434,7 @@ static const struct mdev_parent_ops mdev_fops = {
.read = mtty_read,
.write = mtty_write,
.ioctl = mtty_ioctl,
+ .get_alias_length = mtty_get_alias_length
};
static void mtty_device_release(struct device *dev)
--
2.19.2
[PATCH v2 4/6] mdev: Introduce an API mdev_alias
Introduce an API mdev_alias() to provide access to optionally generated
alias.
Signed-off-by: Parav Pandit
---
drivers/vfio/mdev/mdev_core.c | 12
include/linux/mdev.h | 1 +
2 files changed, 13 insertions(+)
diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
index c9bf2ac362b9..5399ed6f1612 100644
--- a/drivers/vfio/mdev/mdev_core.c
+++ b/drivers/vfio/mdev/mdev_core.c
@@ -517,6 +517,18 @@ struct device *mdev_get_iommu_device(struct device *dev)
}
EXPORT_SYMBOL(mdev_get_iommu_device);
+/**
+ * mdev_alias: Return alias string of a mdev device
+ * @mdev: Pointer to the mdev device
+ * mdev_alias() returns alias string of a mdev device if alias is present,
+ * returns NULL otherwise.
+ */
+const char *mdev_alias(struct mdev_device *mdev)
+{
+ return mdev->alias;
+}
+EXPORT_SYMBOL(mdev_alias);
+
static int __init mdev_init(void)
{
int ret;
diff --git a/include/linux/mdev.h b/include/linux/mdev.h
index f036fe9854ee..6da82213bc4e 100644
--- a/include/linux/mdev.h
+++ b/include/linux/mdev.h
@@ -148,5 +148,6 @@ void mdev_unregister_driver(struct mdev_driver *drv);
struct device *mdev_parent_dev(struct mdev_device *mdev);
struct device *mdev_dev(struct mdev_device *mdev);
struct mdev_device *mdev_from_dev(struct device *dev);
+const char *mdev_alias(struct mdev_device *mdev);
#endif /* MDEV_H */
--
2.19.2
[PATCH v2 3/6] mdev: Expose mdev alias in sysfs tree
Expose the optional alias for an mdev device as a sysfs attribute.
This way, userspace tools such as udev may make use of the alias, for
example to create a netdevice name for the mdev.
Signed-off-by: Parav Pandit
---
Changelog:
v0->v1:
- Addressed comments from Cornelia Huck
- Updated commit description
---
drivers/vfio/mdev/mdev_sysfs.c | 13 +
1 file changed, 13 insertions(+)
diff --git a/drivers/vfio/mdev/mdev_sysfs.c b/drivers/vfio/mdev/mdev_sysfs.c
index 43afe0e80b76..59f4e3cc5233 100644
--- a/drivers/vfio/mdev/mdev_sysfs.c
+++ b/drivers/vfio/mdev/mdev_sysfs.c
@@ -246,7 +246,20 @@ static ssize_t remove_store(struct device *dev, struct
device_attribute *attr,
static DEVICE_ATTR_WO(remove);
+static ssize_t alias_show(struct device *device,
+ struct device_attribute *attr, char *buf)
+{
+ struct mdev_device *dev = mdev_from_dev(device);
+
+ if (!dev->alias)
+ return -EOPNOTSUPP;
+
+ return sprintf(buf, "%s\n", dev->alias);
+}
+static DEVICE_ATTR_RO(alias);
+
static const struct attribute *mdev_device_attrs[] = {
+ _attr_alias.attr,
_attr_remove.attr,
NULL,
};
--
2.19.2
[PATCH v2 0/6] Introduce variable length mdev alias
To have consistent naming for the netdevice of a mdev and to have consistent naming of the devlink port [1] of a mdev, which is formed using phys_port_name of the devlink port, current UUID is not usable because UUID is too long. UUID in string format is 36-characters long and in binary 128-bit. Both formats are not able to fit within 15 characters limit of netdev name. It is desired to have mdev device naming consistent using UUID. So that widely used user space framework such as ovs [2] can make use of mdev representor in similar way as PCIe SR-IOV VF and PF representors. Hence, (a) mdev alias is created which is derived using sha1 from the mdev name. (b) Vendor driver describes how long an alias should be for the child mdev created for a given parent. (c) Mdev aliases are unique at system level. (d) alias is created optionally whenever parent requested. This ensures that non networking mdev parents can function without alias creation overhead. This design is discussed at [3]. An example systemd/udev extension will have, 1. netdev name created using mdev alias available in sysfs. mdev UUID=83b8f4f2-509f-382f-3c1e-e6bfe0fa1001 mdev 12 character alias=cd5b146a80a5 netdev name of this mdev = enmcd5b146a80a5 Here en = Ethernet link m = mediated device 2. devlink port phys_port_name created using mdev alias. devlink phys_port_name=pcd5b146a80a5 This patchset enables mdev core to maintain unique alias for a mdev. Patch-1 Introduces mdev alias using sha1. Patch-2 Ensures that mdev alias is unique in a system. Patch-3 Exposes mdev alias in a sysfs hirerchy. Patch-4 Introduces mdev_alias() API. Patch-5 Updated sysfs documentation Patch-6 Extends mtty driver to optionally provide alias generation. This also enables to test UUID based sha1 collision and trigger error handling for duplicate sha1 results. [1] http://man7.org/linux/man-pages/man8/devlink-port.8.html [2] https://docs.openstack.org/os-vif/latest/user/plugins/ovs.html [3] https://patchwork.kernel.org/cover/11084231/ --- Changelog: v1->v2: - Corrected a typo from 'and' to 'an' - Addressed comments from Alex Williamson - Kept mdev_device naturally aligned - Added error checking for crypt_*() calls - Moved alias NULL check at beginning - Added mdev_alias() API - Updated mtty driver to show example mdev_alias() usage - Changed return type of generate_alias() from int to char* v0->v1: - Addressed comments from Alex Williamson, Cornelia Hunk and Mark Bloch - Moved alias length check outside of the parent lock - Moved alias and digest allocation from kvzalloc to kzalloc - [0] changed to alias - alias_length check is nested under get_alias_length callback check - Changed comments to start with an empty line - Added comment where alias memory ownership is handed over to mdev device - Fixed cleaunup of hash if mdev_bus_register() fails - Updated documentation for new sysfs alias file - Improved commit logs to make description more clear - Fixed inclusiong of alias for NULL check - Added ratelimited debug print for sha1 hash collision error Parav Pandit (6): mdev: Introduce sha1 based mdev alias mdev: Make mdev alias unique among all mdevs mdev: Expose mdev alias in sysfs tree mdev: Introduce an API mdev_alias mdev: Update sysfs documentation mtty: Optionally support mtty alias .../driver-api/vfio-mediated-device.rst | 5 + drivers/vfio/mdev/mdev_core.c | 142 +- drivers/vfio/mdev/mdev_private.h | 5 +- drivers/vfio/mdev/mdev_sysfs.c| 26 +++- include/linux/mdev.h | 5 + samples/vfio-mdev/mtty.c | 13 ++ 6 files changed, 186 insertions(+), 10 deletions(-) -- 2.19.2
RE: [PATCH v1 2/5] mdev: Make mdev alias unique among all mdevs
> -Original Message-
> From: Alex Williamson
> Sent: Thursday, August 29, 2019 3:07 AM
> To: Parav Pandit
> Cc: Jiri Pirko ; kwankh...@nvidia.com;
> coh...@redhat.com; da...@davemloft.net; k...@vger.kernel.org; linux-
> ker...@vger.kernel.org; net...@vger.kernel.org
> Subject: Re: [PATCH v1 2/5] mdev: Make mdev alias unique among all mdevs
>
> On Tue, 27 Aug 2019 14:16:51 -0500
> Parav Pandit wrote:
>
> > Mdev alias should be unique among all the mdevs, so that when such
> > alias is used by the mdev users to derive other objects, there is no
> > collision in a given system.
> >
> > Signed-off-by: Parav Pandit
> >
> > ---
> > Changelog:
> > v0->v1:
> > - Fixed inclusiong of alias for NULL check
> > - Added ratelimited debug print for sha1 hash collision error
> > ---
> > drivers/vfio/mdev/mdev_core.c | 7 +++
> > 1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/vfio/mdev/mdev_core.c
> > b/drivers/vfio/mdev/mdev_core.c index 62d29f57fe0c..4b9899e40665
> > 100644
> > --- a/drivers/vfio/mdev/mdev_core.c
> > +++ b/drivers/vfio/mdev/mdev_core.c
> > @@ -375,6 +375,13 @@ int mdev_device_create(struct kobject *kobj, struct
> device *dev,
> > ret = -EEXIST;
> > goto mdev_fail;
> > }
> > + if (tmp->alias && alias && strcmp(tmp->alias, alias) == 0) {
>
> Nit, test if the device we adding has an alias before the device we're testing
> against. The compiler can better optimize keeping alias hot.
> Thanks,
>
Ok. will do.
> Alex
RE: [PATCH v1 1/5] mdev: Introduce sha1 based mdev alias
> -Original Message-
> From: Alex Williamson
> Sent: Thursday, August 29, 2019 3:05 AM
> To: Parav Pandit
> Cc: Jiri Pirko ; kwankh...@nvidia.com;
> coh...@redhat.com; da...@davemloft.net; k...@vger.kernel.org; linux-
> ker...@vger.kernel.org; net...@vger.kernel.org
> Subject: Re: [PATCH v1 1/5] mdev: Introduce sha1 based mdev alias
>
> On Wed, 28 Aug 2019 15:25:44 -0600
> Alex Williamson wrote:
>
> > On Tue, 27 Aug 2019 14:16:50 -0500
> > Parav Pandit wrote:
> > > module_init(mdev_init)
> > > diff --git a/drivers/vfio/mdev/mdev_private.h
> b/drivers/vfio/mdev/mdev_private.h
> > > index 7d922950caaf..cf1c0d9842c6 100644
> > > --- a/drivers/vfio/mdev/mdev_private.h
> > > +++ b/drivers/vfio/mdev/mdev_private.h
> > > @@ -33,6 +33,7 @@ struct mdev_device {
> > > struct kobject *type_kobj;
> > > struct device *iommu_device;
> > > bool active;
> > > + const char *alias;
>
> Nit, put this above active to avoid creating a hole in the structure.
> Thanks,
>
Ack.
> Alex
RE: [PATCH v1 1/5] mdev: Introduce sha1 based mdev alias
> -Original Message-
> From: Alex Williamson
> Sent: Thursday, August 29, 2019 2:56 AM
> To: Parav Pandit
> Cc: Jiri Pirko ; kwankh...@nvidia.com;
> coh...@redhat.com; da...@davemloft.net; k...@vger.kernel.org; linux-
> ker...@vger.kernel.org; net...@vger.kernel.org
> Subject: Re: [PATCH v1 1/5] mdev: Introduce sha1 based mdev alias
>
> > + /* Allocate and init descriptor */
> > + hash_desc = kvzalloc(sizeof(*hash_desc) +
> > +crypto_shash_descsize(alias_hash),
> > +GFP_KERNEL);
> > + if (!hash_desc)
> > + goto desc_err;
> > +
> > + hash_desc->tfm = alias_hash;
> > +
> > + digest_size = crypto_shash_digestsize(alias_hash);
> > +
> > + digest = kzalloc(digest_size, GFP_KERNEL);
> > + if (!digest) {
> > + ret = -ENOMEM;
> > + goto digest_err;
> > + }
> > + crypto_shash_init(hash_desc);
> > + crypto_shash_update(hash_desc, uuid, UUID_STRING_LEN);
> > + crypto_shash_final(hash_desc, digest);
>
> All of these can fail and many, if not most, of the callers appear that they
> might
> test the return value. Thanks,
Right. Changing the signature and honoring return value in v2.
>
> Alex
[PATCH v1 2/5] mdev: Make mdev alias unique among all mdevs
Mdev alias should be unique among all the mdevs, so that when such alias
is used by the mdev users to derive other objects, there is no
collision in a given system.
Signed-off-by: Parav Pandit
---
Changelog:
v0->v1:
- Fixed inclusiong of alias for NULL check
- Added ratelimited debug print for sha1 hash collision error
---
drivers/vfio/mdev/mdev_core.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
index 62d29f57fe0c..4b9899e40665 100644
--- a/drivers/vfio/mdev/mdev_core.c
+++ b/drivers/vfio/mdev/mdev_core.c
@@ -375,6 +375,13 @@ int mdev_device_create(struct kobject *kobj, struct device
*dev,
ret = -EEXIST;
goto mdev_fail;
}
+ if (tmp->alias && alias && strcmp(tmp->alias, alias) == 0) {
+ mutex_unlock(_list_lock);
+ ret = -EEXIST;
+ dev_dbg_ratelimited(dev, "Hash collision in alias
creation for UUID %pUl\n",
+ uuid);
+ goto mdev_fail;
+ }
}
mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
--
2.19.2
[PATCH v1 4/5] mdev: Update sysfs documentation
Updated documentation for optional read only sysfs attribute.
Signed-off-by: Parav Pandit
---
Documentation/driver-api/vfio-mediated-device.rst | 5 +
1 file changed, 5 insertions(+)
diff --git a/Documentation/driver-api/vfio-mediated-device.rst
b/Documentation/driver-api/vfio-mediated-device.rst
index 25eb7d5b834b..0ab03d3f5629 100644
--- a/Documentation/driver-api/vfio-mediated-device.rst
+++ b/Documentation/driver-api/vfio-mediated-device.rst
@@ -270,6 +270,7 @@ Directories and Files Under the sysfs for Each mdev Device
|--- remove
|--- mdev_type {link to its type}
|--- vendor-specific-attributes [optional]
+ |--- alias [optional]
* remove (write only)
@@ -281,6 +282,10 @@ Example::
# echo 1 > /sys/bus/mdev/devices/$mdev_UUID/remove
+* alias (read only)
+Whenever a parent requested to generate an alias, each mdev is assigned a
unique
+alias by the mdev core. This file shows the alias of the mdev device.
+
Mediated device Hot plug
--
2.19.2
[PATCH v1 1/5] mdev: Introduce sha1 based mdev alias
Some vendor drivers want an identifier for an mdev device that is
shorter than the UUID, due to length restrictions in the consumers of
that identifier.
Add a callback that allows a vendor driver to request an alias of a
specified length to be generated for an mdev device. If generated,
that alias is checked for collisions.
It is an optional attribute.
mdev alias is generated using sha1 from the mdev name.
Signed-off-by: Parav Pandit
---
Changelog:
v0->v1:
- Moved alias length check outside of the parent lock
- Moved alias and digest allocation from kvzalloc to kzalloc
- [0] changed to alias
- alias_length check is nested under get_alias_length callback check
- Changed comments to start with an empty line
- Fixed cleaunup of hash if mdev_bus_register() fails
- Added comment where alias memory ownership is handed over to mdev device
- Updated commit log to indicate motivation for this feature
---
drivers/vfio/mdev/mdev_core.c| 110 ++-
drivers/vfio/mdev/mdev_private.h | 5 +-
drivers/vfio/mdev/mdev_sysfs.c | 13 ++--
include/linux/mdev.h | 4 ++
4 files changed, 122 insertions(+), 10 deletions(-)
diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
index b558d4cfd082..62d29f57fe0c 100644
--- a/drivers/vfio/mdev/mdev_core.c
+++ b/drivers/vfio/mdev/mdev_core.c
@@ -10,9 +10,11 @@
#include
#include
#include
+#include
#include
#include
#include
+#include
#include "mdev_private.h"
@@ -27,6 +29,8 @@ static struct class_compat *mdev_bus_compat_class;
static LIST_HEAD(mdev_list);
static DEFINE_MUTEX(mdev_list_lock);
+static struct crypto_shash *alias_hash;
+
struct device *mdev_parent_dev(struct mdev_device *mdev)
{
return mdev->parent->dev;
@@ -150,6 +154,16 @@ int mdev_register_device(struct device *dev, const struct
mdev_parent_ops *ops)
if (!ops || !ops->create || !ops->remove || !ops->supported_type_groups)
return -EINVAL;
+ if (ops->get_alias_length) {
+ unsigned int digest_size;
+ unsigned int aligned_len;
+
+ aligned_len = roundup(ops->get_alias_length(), 2);
+ digest_size = crypto_shash_digestsize(alias_hash);
+ if (aligned_len / 2 > digest_size)
+ return -EINVAL;
+ }
+
dev = get_device(dev);
if (!dev)
return -EINVAL;
@@ -259,6 +273,7 @@ static void mdev_device_free(struct mdev_device *mdev)
mutex_unlock(_list_lock);
dev_dbg(>dev, "MDEV: destroying\n");
+ kfree(mdev->alias);
kfree(mdev);
}
@@ -269,18 +284,88 @@ static void mdev_device_release(struct device *dev)
mdev_device_free(mdev);
}
-int mdev_device_create(struct kobject *kobj,
- struct device *dev, const guid_t *uuid)
+static const char *
+generate_alias(const char *uuid, unsigned int max_alias_len)
+{
+ struct shash_desc *hash_desc;
+ unsigned int digest_size;
+ unsigned char *digest;
+ unsigned int alias_len;
+ char *alias;
+ int ret = 0;
+
+ /*
+* Align to multiple of 2 as bin2hex will generate
+* even number of bytes.
+*/
+ alias_len = roundup(max_alias_len, 2);
+ alias = kzalloc(alias_len + 1, GFP_KERNEL);
+ if (!alias)
+ return NULL;
+
+ /* Allocate and init descriptor */
+ hash_desc = kvzalloc(sizeof(*hash_desc) +
+crypto_shash_descsize(alias_hash),
+GFP_KERNEL);
+ if (!hash_desc)
+ goto desc_err;
+
+ hash_desc->tfm = alias_hash;
+
+ digest_size = crypto_shash_digestsize(alias_hash);
+
+ digest = kzalloc(digest_size, GFP_KERNEL);
+ if (!digest) {
+ ret = -ENOMEM;
+ goto digest_err;
+ }
+ crypto_shash_init(hash_desc);
+ crypto_shash_update(hash_desc, uuid, UUID_STRING_LEN);
+ crypto_shash_final(hash_desc, digest);
+ bin2hex(alias, digest, min_t(unsigned int, digest_size, alias_len / 2));
+ /*
+* When alias length is odd, zero out and additional last byte
+* that bin2hex has copied.
+*/
+ if (max_alias_len % 2)
+ alias[max_alias_len] = 0;
+
+ kfree(digest);
+ kvfree(hash_desc);
+ return alias;
+
+digest_err:
+ kvfree(hash_desc);
+desc_err:
+ kfree(alias);
+ return NULL;
+}
+
+int mdev_device_create(struct kobject *kobj, struct device *dev,
+ const char *uuid_str, const guid_t *uuid)
{
int ret;
struct mdev_device *mdev, *tmp;
struct mdev_parent *parent;
struct mdev_type *type = to_mdev_type(kobj);
+ const char *alias = NULL;
parent = mdev_get_parent(type->parent);
if (!parent)
[PATCH v1 5/5] mtty: Optionally support mtty alias
Provide a module parameter to set alias length to optionally generate
mdev alias.
Example to request mdev alias.
$ modprobe mtty alias_length=12
Signed-off-by: Parav Pandit
---
samples/vfio-mdev/mtty.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c
index 92e770a06ea2..92208245b057 100644
--- a/samples/vfio-mdev/mtty.c
+++ b/samples/vfio-mdev/mtty.c
@@ -1410,6 +1410,15 @@ static struct attribute_group *mdev_type_groups[] = {
NULL,
};
+static unsigned int mtty_alias_length;
+module_param_named(alias_length, mtty_alias_length, uint, 0444);
+MODULE_PARM_DESC(alias_length, "mdev alias length; default=0");
+
+static unsigned int mtty_get_alias_length(void)
+{
+ return mtty_alias_length;
+}
+
static const struct mdev_parent_ops mdev_fops = {
.owner = THIS_MODULE,
.dev_attr_groups= mtty_dev_groups,
@@ -1422,6 +1431,7 @@ static const struct mdev_parent_ops mdev_fops = {
.read = mtty_read,
.write = mtty_write,
.ioctl = mtty_ioctl,
+ .get_alias_length = mtty_get_alias_length
};
static void mtty_device_release(struct device *dev)
--
2.19.2
[PATCH v1 3/5] mdev: Expose mdev alias in sysfs tree
Expose the optional alias for an mdev device as a sysfs attribute.
This way, userspace tools such as udev may make use of the alias, for
example to create a netdevice name for the mdev.
Signed-off-by: Parav Pandit
---
Changelog:
v0->v1:
- Addressed comments from Cornelia Huck
- Updated commit description
---
drivers/vfio/mdev/mdev_sysfs.c | 13 +
1 file changed, 13 insertions(+)
diff --git a/drivers/vfio/mdev/mdev_sysfs.c b/drivers/vfio/mdev/mdev_sysfs.c
index 43afe0e80b76..59f4e3cc5233 100644
--- a/drivers/vfio/mdev/mdev_sysfs.c
+++ b/drivers/vfio/mdev/mdev_sysfs.c
@@ -246,7 +246,20 @@ static ssize_t remove_store(struct device *dev, struct
device_attribute *attr,
static DEVICE_ATTR_WO(remove);
+static ssize_t alias_show(struct device *device,
+ struct device_attribute *attr, char *buf)
+{
+ struct mdev_device *dev = mdev_from_dev(device);
+
+ if (!dev->alias)
+ return -EOPNOTSUPP;
+
+ return sprintf(buf, "%s\n", dev->alias);
+}
+static DEVICE_ATTR_RO(alias);
+
static const struct attribute *mdev_device_attrs[] = {
+ _attr_alias.attr,
_attr_remove.attr,
NULL,
};
--
2.19.2
[PATCH v1 0/5] Introduce variable length mdev alias
To have consistent naming for the netdevice of a mdev and to have consistent naming of the devlink port [1] of a mdev, which is formed using phys_port_name of the devlink port, current UUID is not usable because UUID is too long. UUID in string format is 36-characters long and in binary 128-bit. Both formats are not able to fit within 15 characters limit of netdev name. It is desired to have mdev device naming consistent using UUID. So that widely used user space framework such as ovs [2] can make use of mdev representor in similar way as PCIe SR-IOV VF and PF representors. Hence, (a) mdev alias is created which is derived using sha1 from the mdev name. (b) Vendor driver describes how long an alias should be for the child mdev created for a given parent. (c) Mdev aliases are unique at system level. (d) alias is created optionally whenever parent requested. This ensures that non networking mdev parents can function without alias creation overhead. This design is discussed at [3]. An example systemd/udev extension will have, 1. netdev name created using mdev alias available in sysfs. mdev UUID=83b8f4f2-509f-382f-3c1e-e6bfe0fa1001 mdev 12 character alias=cd5b146a80a5 netdev name of this mdev = enmcd5b146a80a5 Here en = Ethernet link m = mediated device 2. devlink port phys_port_name created using mdev alias. devlink phys_port_name=pcd5b146a80a5 This patchset enables mdev core to maintain unique alias for a mdev. Patch-1 Introduces mdev alias using sha1. Patch-2 Ensures that mdev alias is unique in a system. Patch-3 Exposes mdev alias in a sysfs hirerchy. Patch-4 Extends mtty driver to optionally provide alias generation. This also enables to test UUID based sha1 collision and trigger error handling for duplicate sha1 results. In future when networking driver wants to use mdev alias, mdev_alias() API will be added to derive devlink port name. [1] http://man7.org/linux/man-pages/man8/devlink-port.8.html [2] https://docs.openstack.org/os-vif/latest/user/plugins/ovs.html [3] https://patchwork.kernel.org/cover/11084231/ --- Changelog: v0->v1: - Addressed comments from Alex Williamson, Cornelia Hunk and Mark Bloch - Moved alias length check outside of the parent lock - Moved alias and digest allocation from kvzalloc to kzalloc - [0] changed to alias - alias_length check is nested under get_alias_length callback check - Changed comments to start with an empty line - Added comment where alias memory ownership is handed over to mdev device - Fixed cleaunup of hash if mdev_bus_register() fails - Updated documentation for new sysfs alias file - Improved commit logs to make description more clear - Fixed inclusiong of alias for NULL check - Added ratelimited debug print for sha1 hash collision error Parav Pandit (5): mdev: Introduce sha1 based mdev alias mdev: Make mdev alias unique among all mdevs mdev: Expose mdev alias in sysfs tree mdev: Update sysfs documentation mtty: Optionally support mtty alias .../driver-api/vfio-mediated-device.rst | 5 + drivers/vfio/mdev/mdev_core.c | 117 +- drivers/vfio/mdev/mdev_private.h | 5 +- drivers/vfio/mdev/mdev_sysfs.c| 26 +++- include/linux/mdev.h | 4 + samples/vfio-mdev/mtty.c | 10 ++ 6 files changed, 157 insertions(+), 10 deletions(-) -- 2.19.2
RE: [PATCH 2/4] mdev: Make mdev alias unique among all mdevs
> -Original Message-
> From: Alex Williamson
> Sent: Tuesday, August 27, 2019 9:55 PM
> To: Parav Pandit
> Cc: Cornelia Huck ; Jiri Pirko ;
> kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org;
> linux-kernel@vger.kernel.org; net...@vger.kernel.org
> Subject: Re: [PATCH 2/4] mdev: Make mdev alias unique among all mdevs
>
> On Tue, 27 Aug 2019 16:13:27 +
> Parav Pandit wrote:
>
> > > -Original Message-
> > > From: Alex Williamson
> > > Sent: Tuesday, August 27, 2019 8:59 PM
> > > To: Cornelia Huck
> > > Cc: Parav Pandit ; Jiri Pirko
> > > ; kwankh...@nvidia.com; da...@davemloft.net;
> > > k...@vger.kernel.org; linux- ker...@vger.kernel.org;
> > > net...@vger.kernel.org
> > > Subject: Re: [PATCH 2/4] mdev: Make mdev alias unique among all
> > > mdevs
> > >
> > > On Tue, 27 Aug 2019 13:29:46 +0200
> > > Cornelia Huck wrote:
> > >
> > > > On Tue, 27 Aug 2019 11:08:59 + Parav Pandit
> > > > wrote:
> > > >
> > > > > > -Original Message-
> > > > > > From: Cornelia Huck
> > > > > > Sent: Tuesday, August 27, 2019 3:59 PM
> > > > > > To: Parav Pandit
> > > > > > Cc: alex.william...@redhat.com; Jiri Pirko
> > > > > > ; kwankh...@nvidia.com;
> > > > > > da...@davemloft.net; k...@vger.kernel.org;
> > > > > > linux- ker...@vger.kernel.org; net...@vger.kernel.org
> > > > > > Subject: Re: [PATCH 2/4] mdev: Make mdev alias unique among
> > > > > > all mdevs
> > > > > >
> > > > > > On Mon, 26 Aug 2019 15:41:17 -0500 Parav Pandit
> > > > > > wrote:
> > > > > >
> > > > > > > Mdev alias should be unique among all the mdevs, so that
> > > > > > > when such alias is used by the mdev users to derive other
> > > > > > > objects, there is no collision in a given system.
> > > > > > >
> > > > > > > Signed-off-by: Parav Pandit
> > > > > > > ---
> > > > > > > drivers/vfio/mdev/mdev_core.c | 5 +
> > > > > > > 1 file changed, 5 insertions(+)
> > > > > > >
> > > > > > > diff --git a/drivers/vfio/mdev/mdev_core.c
> > > > > > > b/drivers/vfio/mdev/mdev_core.c index
> > > > > > > e825ff38b037..6eb37f0c6369
> > > > > > > 100644
> > > > > > > --- a/drivers/vfio/mdev/mdev_core.c
> > > > > > > +++ b/drivers/vfio/mdev/mdev_core.c
> > > > > > > @@ -375,6 +375,11 @@ int mdev_device_create(struct kobject
> > > > > > > *kobj,
> > > struct
> > > > > > device *dev,
> > > > > > > ret = -EEXIST;
> > > > > > > goto mdev_fail;
> > > > > > > }
> > > > > > > + if (tmp->alias && strcmp(tmp->alias, alias) == 0) {
> > > > > >
> > > > > > Any way we can relay to the caller that the uuid was fine, but
> > > > > > that we had a hash collision? Duplicate uuids are much more
> > > > > > obvious than
> > > a collision here.
> > > > > >
> > > > > How do you want to relay this rare event?
> > > > > Netlink interface has way to return the error message back, but
> > > > > sysfs is
> > > limited due to its error code based interface.
> > > >
> > > > I don't know, that's why I asked :)
> > > >
> > > > The problem is that "uuid already used" and "hash collision" are
> > > > indistinguishable. While "use a different uuid" will probably work
> > > > in both cases, "increase alias length" might be a good alternative
> > > > in some cases.
> > > >
> > > > But if there is no good way to relay the problem, we can live with it.
> > >
> > > It's a rare event, maybe just dev_dbg(dev, "Hash collision creating alias
> \"%s\"
> > > for mdev device %pUl\n",...
> > >
> > Ok.
> > dev_dbg_once() to avoid message flood.
>
> I'd suggest a rate-limit rather than a once. The fact that the kernel may
> have
> experienced a collision at some time in the past does not help someone
> debug why they can't create a device now. The only way we're going to get a
> flood is if a user sufficiently privileged to create mdev devices stumbles
> onto
> a collision and continues to repeat the same operation. That falls into
> shoot-yourself-in-the-foot behavior imo.
> Thanks,
>
Ok. Will do.
RE: [PATCH 0/4] Introduce variable length mdev alias
> -Original Message- > From: Alex Williamson > Sent: Tuesday, August 27, 2019 11:19 PM > To: Parav Pandit > Cc: Jiri Pirko ; kwankh...@nvidia.com; > coh...@redhat.com; da...@davemloft.net; k...@vger.kernel.org; linux- > ker...@vger.kernel.org; net...@vger.kernel.org > Subject: Re: [PATCH 0/4] Introduce variable length mdev alias > > On Tue, 27 Aug 2019 13:11:17 + > Parav Pandit wrote: > > > Hi Alex, Cornelia, > > > > > -Original Message- > > > From: kvm-ow...@vger.kernel.org On > > > Behalf Of Parav Pandit > > > Sent: Tuesday, August 27, 2019 2:11 AM > > > To: alex.william...@redhat.com; Jiri Pirko ; > > > kwankh...@nvidia.com; coh...@redhat.com; da...@davemloft.net > > > Cc: k...@vger.kernel.org; linux-kernel@vger.kernel.org; > > > net...@vger.kernel.org; Parav Pandit > > > Subject: [PATCH 0/4] Introduce variable length mdev alias > > > > > > To have consistent naming for the netdevice of a mdev and to have > > > consistent naming of the devlink port [1] of a mdev, which is formed > > > using phys_port_name of the devlink port, current UUID is not usable > > > because UUID is too long. > > > > > > UUID in string format is 36-characters long and in binary 128-bit. > > > Both formats are not able to fit within 15 characters limit of netdev > name. > > > > > > It is desired to have mdev device naming consistent using UUID. > > > So that widely used user space framework such as ovs [2] can make > > > use of mdev representor in similar way as PCIe SR-IOV VF and PF > representors. > > > > > > Hence, > > > (a) mdev alias is created which is derived using sha1 from the mdev > name. > > > (b) Vendor driver describes how long an alias should be for the > > > child mdev created for a given parent. > > > (c) Mdev aliases are unique at system level. > > > (d) alias is created optionally whenever parent requested. > > > This ensures that non networking mdev parents can function without > > > alias creation overhead. > > > > > > This design is discussed at [3]. > > > > > > An example systemd/udev extension will have, > > > > > > 1. netdev name created using mdev alias available in sysfs. > > > > > > mdev UUID=83b8f4f2-509f-382f-3c1e-e6bfe0fa1001 > > > mdev 12 character alias=cd5b146a80a5 > > > > > > netdev name of this mdev = enmcd5b146a80a5 Here en = Ethernet link m > > > = mediated device > > > > > > 2. devlink port phys_port_name created using mdev alias. > > > devlink phys_port_name=pcd5b146a80a5 > > > > > > This patchset enables mdev core to maintain unique alias for a mdev. > > > > > > Patch-1 Introduces mdev alias using sha1. > > > Patch-2 Ensures that mdev alias is unique in a system. > > > Patch-3 Exposes mdev alias in a sysfs hirerchy. > > > Patch-4 Extends mtty driver to optionally provide alias generation. > > > This also enables to test UUID based sha1 collision and trigger > > > error handling for duplicate sha1 results. > > > > > > In future when networking driver wants to use mdev alias, > > > mdev_alias() API will be added to derive devlink port name. > > > > > Now that majority of above patches looks in shape and I addressed all > > comments, In next v1 post, I was considering to include mdev_alias() > > and have example use in mtty driver. > > > > This way, subsequent series of mlx5_core who intents to use > > mdev_alias() API makes it easy to review and merge through Dave M, > > netdev tree. Is that ok with you? > > What would be the timing for the mlx5_core use case? Can we coordinate > within the same development cycle? I wouldn't want someone to come > clean up the sample driver and remove the API ;) Thanks, > We targeted it for 5.4. mdev_alias was the only known user interface issue, which is resolved. Some more internal reviews are in progress. It might be tight for 5.4, if not 5.4, it should happen in 5.5. I agree, that is why I was holding up to be part of this series. Since its very small API, even if there is any merge conflict, it is easy to resolve. If this change can be merged through netdev tree, its better to include it as part of mlx5_core's mdev series. So both options are fine, a direction from you is better to have.
RE: [PATCH 2/4] mdev: Make mdev alias unique among all mdevs
> -Original Message-
> From: Alex Williamson
> Sent: Tuesday, August 27, 2019 8:54 PM
> To: Parav Pandit
> Cc: Mark Bloch ; Jiri Pirko ;
> kwankh...@nvidia.com; coh...@redhat.com; da...@davemloft.net;
> k...@vger.kernel.org; linux-kernel@vger.kernel.org; net...@vger.kernel.org
> Subject: Re: [PATCH 2/4] mdev: Make mdev alias unique among all mdevs
>
> On Tue, 27 Aug 2019 04:28:37 +
> Parav Pandit wrote:
>
> > Hi Mark,
> >
> > > -Original Message-
> > > From: Mark Bloch
> > > Sent: Tuesday, August 27, 2019 4:32 AM
> > > To: Parav Pandit ; alex.william...@redhat.com;
> > > Jiri Pirko ; kwankh...@nvidia.com;
> > > coh...@redhat.com; da...@davemloft.net
> > > Cc: k...@vger.kernel.org; linux-kernel@vger.kernel.org;
> > > net...@vger.kernel.org
> > > Subject: Re: [PATCH 2/4] mdev: Make mdev alias unique among all
> > > mdevs
> > >
> > >
> > >
> > > On 8/26/19 1:41 PM, Parav Pandit wrote:
> > > > Mdev alias should be unique among all the mdevs, so that when such
> > > > alias is used by the mdev users to derive other objects, there is
> > > > no collision in a given system.
> > > >
> > > > Signed-off-by: Parav Pandit
> > > > ---
> > > > drivers/vfio/mdev/mdev_core.c | 5 +
> > > > 1 file changed, 5 insertions(+)
> > > >
> > > > diff --git a/drivers/vfio/mdev/mdev_core.c
> > > > b/drivers/vfio/mdev/mdev_core.c index e825ff38b037..6eb37f0c6369
> > > > 100644
> > > > --- a/drivers/vfio/mdev/mdev_core.c
> > > > +++ b/drivers/vfio/mdev/mdev_core.c
> > > > @@ -375,6 +375,11 @@ int mdev_device_create(struct kobject *kobj,
> > > struct device *dev,
> > > > ret = -EEXIST;
> > > > goto mdev_fail;
> > > > }
> > > > + if (tmp->alias && strcmp(tmp->alias, alias) == 0) {
> > >
> > > alias can be NULL here no?
> > >
> > If alias is NULL, tmp->alias would also be null because for given parent
> > either
> we have alias or we don’t.
> > So its not possible to have tmp->alias as null and alias as non null.
> > But it may be good/defensive to add check for both.
>
> mdev_list is a global list of all mdev devices, how can we make any
> assumptions that an element has the same parent? Thanks,
>
Oh yes, right. If tmp->alias is not_null but alias can be NULL.
I will fix the check.
> Alex
>
> > > > + mutex_unlock(_list_lock);
> > > > + ret = -EEXIST;
> > > > + goto mdev_fail;
> > > > + }
> > > > }
> > > >
> > > > mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
> > > >
> > >
> > > Mark
RE: [PATCH 2/4] mdev: Make mdev alias unique among all mdevs
> -Original Message-
> From: Alex Williamson
> Sent: Tuesday, August 27, 2019 8:59 PM
> To: Cornelia Huck
> Cc: Parav Pandit ; Jiri Pirko ;
> kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org; linux-
> ker...@vger.kernel.org; net...@vger.kernel.org
> Subject: Re: [PATCH 2/4] mdev: Make mdev alias unique among all mdevs
>
> On Tue, 27 Aug 2019 13:29:46 +0200
> Cornelia Huck wrote:
>
> > On Tue, 27 Aug 2019 11:08:59 +
> > Parav Pandit wrote:
> >
> > > > -Original Message-
> > > > From: Cornelia Huck
> > > > Sent: Tuesday, August 27, 2019 3:59 PM
> > > > To: Parav Pandit
> > > > Cc: alex.william...@redhat.com; Jiri Pirko ;
> > > > kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org;
> > > > linux- ker...@vger.kernel.org; net...@vger.kernel.org
> > > > Subject: Re: [PATCH 2/4] mdev: Make mdev alias unique among all
> > > > mdevs
> > > >
> > > > On Mon, 26 Aug 2019 15:41:17 -0500 Parav Pandit
> > > > wrote:
> > > >
> > > > > Mdev alias should be unique among all the mdevs, so that when
> > > > > such alias is used by the mdev users to derive other objects,
> > > > > there is no collision in a given system.
> > > > >
> > > > > Signed-off-by: Parav Pandit
> > > > > ---
> > > > > drivers/vfio/mdev/mdev_core.c | 5 +
> > > > > 1 file changed, 5 insertions(+)
> > > > >
> > > > > diff --git a/drivers/vfio/mdev/mdev_core.c
> > > > > b/drivers/vfio/mdev/mdev_core.c index e825ff38b037..6eb37f0c6369
> > > > > 100644
> > > > > --- a/drivers/vfio/mdev/mdev_core.c
> > > > > +++ b/drivers/vfio/mdev/mdev_core.c
> > > > > @@ -375,6 +375,11 @@ int mdev_device_create(struct kobject *kobj,
> struct
> > > > device *dev,
> > > > > ret = -EEXIST;
> > > > > goto mdev_fail;
> > > > > }
> > > > > + if (tmp->alias && strcmp(tmp->alias, alias) == 0) {
> > > >
> > > > Any way we can relay to the caller that the uuid was fine, but
> > > > that we had a hash collision? Duplicate uuids are much more obvious than
> a collision here.
> > > >
> > > How do you want to relay this rare event?
> > > Netlink interface has way to return the error message back, but sysfs is
> limited due to its error code based interface.
> >
> > I don't know, that's why I asked :)
> >
> > The problem is that "uuid already used" and "hash collision" are
> > indistinguishable. While "use a different uuid" will probably work in
> > both cases, "increase alias length" might be a good alternative in
> > some cases.
> >
> > But if there is no good way to relay the problem, we can live with it.
>
> It's a rare event, maybe just dev_dbg(dev, "Hash collision creating alias
> \"%s\"
> for mdev device %pUl\n",...
>
Ok.
dev_dbg_once() to avoid message flood.
> Thanks,
> Alex
>
> > > > > + mutex_unlock(_list_lock);
> > > > > + ret = -EEXIST;
> > > > > + goto mdev_fail;
> > > > > + }
> > > > > }
> > > > >
> > > > > mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
> > >
> >
RE: [PATCH 0/4] Introduce variable length mdev alias
Hi Alex, Cornelia, > -Original Message- > From: kvm-ow...@vger.kernel.org On Behalf > Of Parav Pandit > Sent: Tuesday, August 27, 2019 2:11 AM > To: alex.william...@redhat.com; Jiri Pirko ; > kwankh...@nvidia.com; coh...@redhat.com; da...@davemloft.net > Cc: k...@vger.kernel.org; linux-kernel@vger.kernel.org; > net...@vger.kernel.org; Parav Pandit > Subject: [PATCH 0/4] Introduce variable length mdev alias > > To have consistent naming for the netdevice of a mdev and to have consistent > naming of the devlink port [1] of a mdev, which is formed using > phys_port_name of the devlink port, current UUID is not usable because UUID > is too long. > > UUID in string format is 36-characters long and in binary 128-bit. > Both formats are not able to fit within 15 characters limit of netdev name. > > It is desired to have mdev device naming consistent using UUID. > So that widely used user space framework such as ovs [2] can make use of > mdev representor in similar way as PCIe SR-IOV VF and PF representors. > > Hence, > (a) mdev alias is created which is derived using sha1 from the mdev name. > (b) Vendor driver describes how long an alias should be for the child mdev > created for a given parent. > (c) Mdev aliases are unique at system level. > (d) alias is created optionally whenever parent requested. > This ensures that non networking mdev parents can function without alias > creation overhead. > > This design is discussed at [3]. > > An example systemd/udev extension will have, > > 1. netdev name created using mdev alias available in sysfs. > > mdev UUID=83b8f4f2-509f-382f-3c1e-e6bfe0fa1001 > mdev 12 character alias=cd5b146a80a5 > > netdev name of this mdev = enmcd5b146a80a5 Here en = Ethernet link m = > mediated device > > 2. devlink port phys_port_name created using mdev alias. > devlink phys_port_name=pcd5b146a80a5 > > This patchset enables mdev core to maintain unique alias for a mdev. > > Patch-1 Introduces mdev alias using sha1. > Patch-2 Ensures that mdev alias is unique in a system. > Patch-3 Exposes mdev alias in a sysfs hirerchy. > Patch-4 Extends mtty driver to optionally provide alias generation. > This also enables to test UUID based sha1 collision and trigger error handling > for duplicate sha1 results. > > In future when networking driver wants to use mdev alias, mdev_alias() API > will > be added to derive devlink port name. > Now that majority of above patches looks in shape and I addressed all comments, In next v1 post, I was considering to include mdev_alias() and have example use in mtty driver. This way, subsequent series of mlx5_core who intents to use mdev_alias() API makes it easy to review and merge through Dave M, netdev tree. Is that ok with you?
RE: [PATCH 3/4] mdev: Expose mdev alias in sysfs tree
> -Original Message-
> From: Cornelia Huck
> Sent: Tuesday, August 27, 2019 5:25 PM
> To: Parav Pandit
> Cc: alex.william...@redhat.com; Jiri Pirko ;
> kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org; linux-
> ker...@vger.kernel.org; net...@vger.kernel.org
> Subject: Re: [PATCH 3/4] mdev: Expose mdev alias in sysfs tree
>
> On Tue, 27 Aug 2019 11:52:21 +
> Parav Pandit wrote:
>
> > > -Original Message-
> > > From: Cornelia Huck
> > > Sent: Tuesday, August 27, 2019 5:05 PM
> > > To: Parav Pandit
> > > Cc: alex.william...@redhat.com; Jiri Pirko ;
> > > kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org;
> > > linux- ker...@vger.kernel.org; net...@vger.kernel.org
> > > Subject: Re: [PATCH 3/4] mdev: Expose mdev alias in sysfs tree
> > >
> > > On Tue, 27 Aug 2019 11:07:37 +
> > > Parav Pandit wrote:
> > >
> > > > > -Original Message-
> > > > > From: Cornelia Huck
> > > > > Sent: Tuesday, August 27, 2019 4:17 PM
> > > > > To: Parav Pandit
> > > > > Cc: alex.william...@redhat.com; Jiri Pirko ;
> > > > > kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org;
> > > > > linux- ker...@vger.kernel.org; net...@vger.kernel.org
> > > > > Subject: Re: [PATCH 3/4] mdev: Expose mdev alias in sysfs tree
> > > > >
> > > > > On Mon, 26 Aug 2019 15:41:18 -0500 Parav Pandit
> > > > > wrote:
> > >
> > > > > > +static ssize_t alias_show(struct device *device,
> > > > > > + struct device_attribute *attr, char *buf) {
> > > > > > + struct mdev_device *dev = mdev_from_dev(device);
> > > > > > +
> > > > > > + if (!dev->alias)
> > > > > > + return -EOPNOTSUPP;
> > > > >
> > > > > I'm wondering how to make this consumable by userspace in the
> > > > > easiest
> > > way.
> > > > > - As you do now (userspace gets an error when trying to read)?
> > > > > - Returning an empty value (nothing to see here, move along)?
> > > > No. This is confusing, to return empty value, because it says that
> > > > there is an
> > > alias but it is some weird empty string.
> > > > If there is alias, it shows exactly what it is.
> > > > If no alias it returns an error code = unsupported -> inline with
> > > > other widely
> > > used subsystem.
> > > >
> > > > > - Or not creating the attribute at all? That would match what
> > > > > userspace
> > > > > sees on older kernels, so it needs to be able to deal with
> > > > > that
> > > > New sysfs files can appear. Tool cannot say that I was not
> > > > expecting this file
> > > here.
> > > > User space is supposed to work with the file they are off interest.
> > > > Mdev interface has option to specify vendor specific files, though
> > > > in usual
> > > manner it's not recommended.
> > > > So there is no old user space, new kernel issue here.
> > >
> > > I'm not talking about old userspace/new kernel, but new userspace/old
> kernel.
> > > Code that wants to consume this attribute needs to be able to cope
> > > with its absence anyway.
> > >
> > Old kernel doesn't have alias file.
> > If some tool tries to read this file it will fail to open non existing
> > file; open()
> system call is already taking care of it.
>
> Yes, that was exactly my argument?
I misunderstood probably.
I re-read all 3 options you posted.
I do not see any issue in reporting error code similar to other widely used
netdev subsystem, hence propose what is posted in the patch.
RE: [PATCH 1/4] mdev: Introduce sha1 based mdev alias
> -Original Message- > From: Cornelia Huck > Sent: Tuesday, August 27, 2019 5:11 PM > To: Parav Pandit > Cc: alex.william...@redhat.com; Jiri Pirko ; > kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org; linux- > ker...@vger.kernel.org; net...@vger.kernel.org > Subject: Re: [PATCH 1/4] mdev: Introduce sha1 based mdev alias > > On Tue, 27 Aug 2019 11:33:54 + > Parav Pandit wrote: > > > > -Original Message- > > > From: Cornelia Huck > > > Sent: Tuesday, August 27, 2019 4:54 PM > > > To: Parav Pandit > > > Cc: alex.william...@redhat.com; Jiri Pirko ; > > > kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org; > > > linux- ker...@vger.kernel.org; net...@vger.kernel.org > > > Subject: Re: [PATCH 1/4] mdev: Introduce sha1 based mdev alias > > > > > > On Tue, 27 Aug 2019 11:12:23 + > > > Parav Pandit wrote: > > > > > > > > -Original Message- > > > > > From: Cornelia Huck > > > > > Sent: Tuesday, August 27, 2019 3:54 PM > > > > > To: Parav Pandit > > > > > Cc: alex.william...@redhat.com; Jiri Pirko ; > > > > > kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org; > > > > > linux- ker...@vger.kernel.org; net...@vger.kernel.org > > > > > Subject: Re: [PATCH 1/4] mdev: Introduce sha1 based mdev alias > > > > > > > > > > > What about: > > > > > > > > > > * @get_alias_length: optional callback to specify length of the > > > > > alias to > > > create > > > > > *Returns unsigned integer: length of the alias to > > > > > be created, > > > > > * 0 to not create an > > > > > alias > > > > > > > > > Ack. > > > > > > > > > I also think it might be beneficial to add a device parameter > > > > > here now (rather than later); that seems to be something that makes > sense. > > > > > > > > > Without showing the use, it shouldn't be added. > > > > > > It just feels like an omission: Why should the vendor driver only be > > > able to return one value here, without knowing which device it is for? > > > If a driver supports different devices, it may have different > > > requirements for them. > > > > > Sure. Lets first have this requirement to add it. > > I am against adding this length field itself without an actual vendor use > > case, > which is adding some complexity in code today. > > But it was ok to have length field instead of bool. > > > > Lets not further add "no-requirement futuristic knobs" which hasn't shown > > its > need yet. > > When a vendor driver needs it, there is nothing prevents such addition. > > Frankly, I do not see how it adds complexity; the other callbacks have device > arguments already, Other ioctls such as create, remove, mmap, likely need to access the parent. Hence it make sense to have parent pointer in there. I am not against complexity, I am just saying, at present there is no use-case. Let have use case and we add it. > and the vendor driver is free to ignore it if it does not have > a use for it. I'd rather add the argument before a possible future user tries > weird hacks to allow multiple values, but I'll leave the decision to the > maintainers. Why would a possible future user tries a weird hack? If user needs to access parent device, that driver maintainer should ask for it.
RE: [PATCH 3/4] mdev: Expose mdev alias in sysfs tree
> -Original Message-
> From: Cornelia Huck
> Sent: Tuesday, August 27, 2019 5:05 PM
> To: Parav Pandit
> Cc: alex.william...@redhat.com; Jiri Pirko ;
> kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org; linux-
> ker...@vger.kernel.org; net...@vger.kernel.org
> Subject: Re: [PATCH 3/4] mdev: Expose mdev alias in sysfs tree
>
> On Tue, 27 Aug 2019 11:07:37 +
> Parav Pandit wrote:
>
> > > -Original Message-
> > > From: Cornelia Huck
> > > Sent: Tuesday, August 27, 2019 4:17 PM
> > > To: Parav Pandit
> > > Cc: alex.william...@redhat.com; Jiri Pirko ;
> > > kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org;
> > > linux- ker...@vger.kernel.org; net...@vger.kernel.org
> > > Subject: Re: [PATCH 3/4] mdev: Expose mdev alias in sysfs tree
> > >
> > > On Mon, 26 Aug 2019 15:41:18 -0500
> > > Parav Pandit wrote:
>
> > > > +static ssize_t alias_show(struct device *device,
> > > > + struct device_attribute *attr, char *buf) {
> > > > + struct mdev_device *dev = mdev_from_dev(device);
> > > > +
> > > > + if (!dev->alias)
> > > > + return -EOPNOTSUPP;
> > >
> > > I'm wondering how to make this consumable by userspace in the easiest
> way.
> > > - As you do now (userspace gets an error when trying to read)?
> > > - Returning an empty value (nothing to see here, move along)?
> > No. This is confusing, to return empty value, because it says that there is
> > an
> alias but it is some weird empty string.
> > If there is alias, it shows exactly what it is.
> > If no alias it returns an error code = unsupported -> inline with other
> > widely
> used subsystem.
> >
> > > - Or not creating the attribute at all? That would match what userspace
> > > sees on older kernels, so it needs to be able to deal with that
> > New sysfs files can appear. Tool cannot say that I was not expecting this
> > file
> here.
> > User space is supposed to work with the file they are off interest.
> > Mdev interface has option to specify vendor specific files, though in usual
> manner it's not recommended.
> > So there is no old user space, new kernel issue here.
>
> I'm not talking about old userspace/new kernel, but new userspace/old kernel.
> Code that wants to consume this attribute needs to be able to cope with its
> absence anyway.
>
Old kernel doesn't have alias file.
If some tool tries to read this file it will fail to open non existing file;
open() system call is already taking care of it.
RE: [PATCH 1/4] mdev: Introduce sha1 based mdev alias
> -Original Message-
> From: Cornelia Huck
> Sent: Tuesday, August 27, 2019 4:54 PM
> To: Parav Pandit
> Cc: alex.william...@redhat.com; Jiri Pirko ;
> kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org; linux-
> ker...@vger.kernel.org; net...@vger.kernel.org
> Subject: Re: [PATCH 1/4] mdev: Introduce sha1 based mdev alias
>
> On Tue, 27 Aug 2019 11:12:23 +
> Parav Pandit wrote:
>
> > > -Original Message-
> > > From: Cornelia Huck
> > > Sent: Tuesday, August 27, 2019 3:54 PM
> > > To: Parav Pandit
> > > Cc: alex.william...@redhat.com; Jiri Pirko ;
> > > kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org;
> > > linux- ker...@vger.kernel.org; net...@vger.kernel.org
> > > Subject: Re: [PATCH 1/4] mdev: Introduce sha1 based mdev alias
> > >
> > > On Mon, 26 Aug 2019 15:41:16 -0500
> > > Parav Pandit wrote:
> > >
> > > > Whenever a parent requests to generate mdev alias, generate a mdev
> > > > alias.
> > > > It is an optional attribute that parent can request to generate
> > > > for each of its child mdev.
> > > > mdev alias is generated using sha1 from the mdev name.
> > >
> > > Maybe add some motivation here as well?
> > >
> > > "Some vendor drivers want an identifier for an mdev device that is
> > > shorter than the uuid, due to length restrictions in the consumers of that
> identifier.
> > >
> > > Add a callback that allows a vendor driver to request an alias of a
> > > specified length to be generated (via sha1) for an mdev device. If
> > > generated, that alias is checked for collisions."
> > >
> > I did described the motivation in the cover letter with example and this
> design discussion thread.
>
> Yes, but adding it to the patch description makes it available in the git
> history.
>
Ok.
> > I will include above summary in v1.
> >
> > > What about:
> > >
> > > * @get_alias_length: optional callback to specify length of the alias to
> create
> > > *Returns unsigned integer: length of the alias to be
> > > created,
> > > * 0 to not create an alias
> > >
> > Ack.
> >
> > > I also think it might be beneficial to add a device parameter here
> > > now (rather than later); that seems to be something that makes sense.
> > >
> > Without showing the use, it shouldn't be added.
>
> It just feels like an omission: Why should the vendor driver only be able to
> return one value here, without knowing which device it is for?
> If a driver supports different devices, it may have different requirements for
> them.
>
Sure. Lets first have this requirement to add it.
I am against adding this length field itself without an actual vendor use case,
which is adding some complexity in code today.
But it was ok to have length field instead of bool.
Lets not further add "no-requirement futuristic knobs" which hasn't shown its
need yet.
When a vendor driver needs it, there is nothing prevents such addition.
> >
> > > > * Parent device that support mediated device should be
> > > > registered with
> > > mdev
> > > > * module with mdev_parent_ops structure.
> > > > **/
> > > > @@ -92,6 +95,7 @@ struct mdev_parent_ops {
> > > > long(*ioctl)(struct mdev_device *mdev, unsigned int cmd,
> > > > unsigned long arg);
> > > > int (*mmap)(struct mdev_device *mdev, struct vm_area_struct
> > > *vma);
> > > > + unsigned int (*get_alias_length)(void);
> > > > };
> > > >
> > > > /* interface for exporting mdev supported type attributes */
> >
RE: [PATCH 1/4] mdev: Introduce sha1 based mdev alias
> -Original Message-
> From: Cornelia Huck
> Sent: Tuesday, August 27, 2019 3:54 PM
> To: Parav Pandit
> Cc: alex.william...@redhat.com; Jiri Pirko ;
> kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org; linux-
> ker...@vger.kernel.org; net...@vger.kernel.org
> Subject: Re: [PATCH 1/4] mdev: Introduce sha1 based mdev alias
>
> On Mon, 26 Aug 2019 15:41:16 -0500
> Parav Pandit wrote:
> >
> > static int __init mdev_init(void)
> > {
> > + alias_hash = crypto_alloc_shash("sha1", 0, 0);
> > + if (!alias_hash)
> > + return -ENOMEM;
> > +
> > return mdev_bus_register();
>
> Don't you need to call crypto_free_shash() if mdev_bus_register() fails?
>
Missed to answer this in previous reply.
Yes, took care of it in v1.
Mark Bloch also pointed it to me.
RE: [PATCH 1/4] mdev: Introduce sha1 based mdev alias
> -Original Message-
> From: Cornelia Huck
> Sent: Tuesday, August 27, 2019 3:54 PM
> To: Parav Pandit
> Cc: alex.william...@redhat.com; Jiri Pirko ;
> kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org; linux-
> ker...@vger.kernel.org; net...@vger.kernel.org
> Subject: Re: [PATCH 1/4] mdev: Introduce sha1 based mdev alias
>
> On Mon, 26 Aug 2019 15:41:16 -0500
> Parav Pandit wrote:
>
> > Whenever a parent requests to generate mdev alias, generate a mdev
> > alias.
> > It is an optional attribute that parent can request to generate for
> > each of its child mdev.
> > mdev alias is generated using sha1 from the mdev name.
>
> Maybe add some motivation here as well?
>
> "Some vendor drivers want an identifier for an mdev device that is shorter
> than
> the uuid, due to length restrictions in the consumers of that identifier.
>
> Add a callback that allows a vendor driver to request an alias of a specified
> length to be generated (via sha1) for an mdev device. If generated, that
> alias is
> checked for collisions."
>
I did described the motivation in the cover letter with example and this design
discussion thread.
I will include above summary in v1.
> What about:
>
> * @get_alias_length: optional callback to specify length of the alias to
> create
> *Returns unsigned integer: length of the alias to be
> created,
> * 0 to not create an alias
>
Ack.
> I also think it might be beneficial to add a device parameter here now (rather
> than later); that seems to be something that makes sense.
>
Without showing the use, it shouldn't be added.
> > * Parent device that support mediated device should be registered with
> mdev
> > * module with mdev_parent_ops structure.
> > **/
> > @@ -92,6 +95,7 @@ struct mdev_parent_ops {
> > long(*ioctl)(struct mdev_device *mdev, unsigned int cmd,
> > unsigned long arg);
> > int (*mmap)(struct mdev_device *mdev, struct vm_area_struct
> *vma);
> > + unsigned int (*get_alias_length)(void);
> > };
> >
> > /* interface for exporting mdev supported type attributes */
RE: [PATCH 2/4] mdev: Make mdev alias unique among all mdevs
> -Original Message-
> From: Cornelia Huck
> Sent: Tuesday, August 27, 2019 3:59 PM
> To: Parav Pandit
> Cc: alex.william...@redhat.com; Jiri Pirko ;
> kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org; linux-
> ker...@vger.kernel.org; net...@vger.kernel.org
> Subject: Re: [PATCH 2/4] mdev: Make mdev alias unique among all mdevs
>
> On Mon, 26 Aug 2019 15:41:17 -0500
> Parav Pandit wrote:
>
> > Mdev alias should be unique among all the mdevs, so that when such
> > alias is used by the mdev users to derive other objects, there is no
> > collision in a given system.
> >
> > Signed-off-by: Parav Pandit
> > ---
> > drivers/vfio/mdev/mdev_core.c | 5 +
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/vfio/mdev/mdev_core.c
> > b/drivers/vfio/mdev/mdev_core.c index e825ff38b037..6eb37f0c6369
> > 100644
> > --- a/drivers/vfio/mdev/mdev_core.c
> > +++ b/drivers/vfio/mdev/mdev_core.c
> > @@ -375,6 +375,11 @@ int mdev_device_create(struct kobject *kobj, struct
> device *dev,
> > ret = -EEXIST;
> > goto mdev_fail;
> > }
> > + if (tmp->alias && strcmp(tmp->alias, alias) == 0) {
>
> Any way we can relay to the caller that the uuid was fine, but that we had a
> hash collision? Duplicate uuids are much more obvious than a collision here.
>
How do you want to relay this rare event?
Netlink interface has way to return the error message back, but sysfs is
limited due to its error code based interface.
> > + mutex_unlock(_list_lock);
> > + ret = -EEXIST;
> > + goto mdev_fail;
> > + }
> > }
> >
> > mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
RE: [PATCH 3/4] mdev: Expose mdev alias in sysfs tree
> -Original Message-
> From: Cornelia Huck
> Sent: Tuesday, August 27, 2019 4:17 PM
> To: Parav Pandit
> Cc: alex.william...@redhat.com; Jiri Pirko ;
> kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org; linux-
> ker...@vger.kernel.org; net...@vger.kernel.org
> Subject: Re: [PATCH 3/4] mdev: Expose mdev alias in sysfs tree
>
> On Mon, 26 Aug 2019 15:41:18 -0500
> Parav Pandit wrote:
>
> > Expose mdev alias as string in a sysfs tree so that such attribute can
> > be used to generate netdevice name by systemd/udev or can be used to
> > match other kernel objects based on the alias of the mdev.
>
> What about
>
> "Expose the optional alias for an mdev device as a sysfs attribute.
> This way, userspace tools such as udev may make use of the alias, for example
> to create a netdevice name for the mdev."
>
Ok. I will change it.
> >
> > Signed-off-by: Parav Pandit
> > ---
> > drivers/vfio/mdev/mdev_sysfs.c | 13 +
>
> I think the documentation should be updated as well.
>
> > 1 file changed, 13 insertions(+)
> >
> > diff --git a/drivers/vfio/mdev/mdev_sysfs.c
> > b/drivers/vfio/mdev/mdev_sysfs.c index 43afe0e80b76..59f4e3cc5233
> > 100644
> > --- a/drivers/vfio/mdev/mdev_sysfs.c
> > +++ b/drivers/vfio/mdev/mdev_sysfs.c
> > @@ -246,7 +246,20 @@ static ssize_t remove_store(struct device *dev,
> > struct device_attribute *attr,
> >
> > static DEVICE_ATTR_WO(remove);
> >
> > +static ssize_t alias_show(struct device *device,
> > + struct device_attribute *attr, char *buf) {
> > + struct mdev_device *dev = mdev_from_dev(device);
> > +
> > + if (!dev->alias)
> > + return -EOPNOTSUPP;
>
> I'm wondering how to make this consumable by userspace in the easiest way.
> - As you do now (userspace gets an error when trying to read)?
> - Returning an empty value (nothing to see here, move along)?
No. This is confusing, to return empty value, because it says that there is an
alias but it is some weird empty string.
If there is alias, it shows exactly what it is.
If no alias it returns an error code = unsupported -> inline with other widely
used subsystem.
> - Or not creating the attribute at all? That would match what userspace
> sees on older kernels, so it needs to be able to deal with that
New sysfs files can appear. Tool cannot say that I was not expecting this file
here.
User space is supposed to work with the file they are off interest.
Mdev interface has option to specify vendor specific files, though in usual
manner it's not recommended.
So there is no old user space, new kernel issue here.
> anyway.
>
> > +
> > + return sprintf(buf, "%s\n", dev->alias); } static
> > +DEVICE_ATTR_RO(alias);
> > +
> > static const struct attribute *mdev_device_attrs[] = {
> > + _attr_alias.attr,
> > _attr_remove.attr,
> > NULL,
> > };
RE: [PATCH 2/4] mdev: Make mdev alias unique among all mdevs
Hi Mark,
> -Original Message-
> From: Mark Bloch
> Sent: Tuesday, August 27, 2019 4:32 AM
> To: Parav Pandit ; alex.william...@redhat.com; Jiri
> Pirko ; kwankh...@nvidia.com; coh...@redhat.com;
> da...@davemloft.net
> Cc: k...@vger.kernel.org; linux-kernel@vger.kernel.org;
> net...@vger.kernel.org
> Subject: Re: [PATCH 2/4] mdev: Make mdev alias unique among all mdevs
>
>
>
> On 8/26/19 1:41 PM, Parav Pandit wrote:
> > Mdev alias should be unique among all the mdevs, so that when such
> > alias is used by the mdev users to derive other objects, there is no
> > collision in a given system.
> >
> > Signed-off-by: Parav Pandit
> > ---
> > drivers/vfio/mdev/mdev_core.c | 5 +
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/vfio/mdev/mdev_core.c
> > b/drivers/vfio/mdev/mdev_core.c index e825ff38b037..6eb37f0c6369
> > 100644
> > --- a/drivers/vfio/mdev/mdev_core.c
> > +++ b/drivers/vfio/mdev/mdev_core.c
> > @@ -375,6 +375,11 @@ int mdev_device_create(struct kobject *kobj,
> struct device *dev,
> > ret = -EEXIST;
> > goto mdev_fail;
> > }
> > + if (tmp->alias && strcmp(tmp->alias, alias) == 0) {
>
> alias can be NULL here no?
>
If alias is NULL, tmp->alias would also be null because for given parent either
we have alias or we don’t.
So its not possible to have tmp->alias as null and alias as non null.
But it may be good/defensive to add check for both.
> > + mutex_unlock(_list_lock);
> > + ret = -EEXIST;
> > + goto mdev_fail;
> > + }
> > }
> >
> > mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
> >
>
> Mark
RE: [PATCH 1/4] mdev: Introduce sha1 based mdev alias
> -Original Message-
> From: Alex Williamson
> Sent: Tuesday, August 27, 2019 7:15 AM
> To: Parav Pandit
> Cc: Jiri Pirko ; kwankh...@nvidia.com;
> coh...@redhat.com; da...@davemloft.net; k...@vger.kernel.org; linux-
> ker...@vger.kernel.org; net...@vger.kernel.org
> Subject: Re: [PATCH 1/4] mdev: Introduce sha1 based mdev alias
>
> On Mon, 26 Aug 2019 15:41:16 -0500
> Parav Pandit wrote:
>
> > Whenever a parent requests to generate mdev alias, generate a mdev
> > alias.
> > It is an optional attribute that parent can request to generate for
> > each of its child mdev.
> > mdev alias is generated using sha1 from the mdev name.
> >
> > Signed-off-by: Parav Pandit
> > ---
> > drivers/vfio/mdev/mdev_core.c| 98
> +++-
> > drivers/vfio/mdev/mdev_private.h | 5 +-
> > drivers/vfio/mdev/mdev_sysfs.c | 13 +++--
> > include/linux/mdev.h | 4 ++
> > 4 files changed, 111 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/vfio/mdev/mdev_core.c
> > b/drivers/vfio/mdev/mdev_core.c index b558d4cfd082..e825ff38b037
> > 100644
> > --- a/drivers/vfio/mdev/mdev_core.c
> > +++ b/drivers/vfio/mdev/mdev_core.c
> > @@ -10,9 +10,11 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > +#include
> >
> > #include "mdev_private.h"
> >
> > @@ -27,6 +29,8 @@ static struct class_compat *mdev_bus_compat_class;
> > static LIST_HEAD(mdev_list); static DEFINE_MUTEX(mdev_list_lock);
> >
> > +static struct crypto_shash *alias_hash;
> > +
> > struct device *mdev_parent_dev(struct mdev_device *mdev) {
> > return mdev->parent->dev;
> > @@ -164,6 +168,18 @@ int mdev_register_device(struct device *dev, const
> struct mdev_parent_ops *ops)
> > goto add_dev_err;
> > }
> >
> > + if (ops->get_alias_length) {
> > + unsigned int digest_size;
> > + unsigned int aligned_len;
> > +
> > + aligned_len = roundup(ops->get_alias_length(), 2);
> > + digest_size = crypto_shash_digestsize(alias_hash);
> > + if (aligned_len / 2 > digest_size) {
> > + ret = -EINVAL;
> > + goto add_dev_err;
> > + }
> > + }
>
> This looks like a sanity check, it could be done outside of the
> parent_list_lock, even before we get a parent device reference.
>
Yes.
> I think we're using a callback for get_alias_length() rather than a fixed
> field
> to support the mtty module option added in patch 4, right?
Right.
I will move the check outside.
> Its utility is rather limited with no args. I could imagine that if a parent
> wanted to generate an alias that could be incorporated into a string with the
> parent device name that it would be useful to call this with the parent
> device as an arg. I guess we can save that until a user comes along though.
>
Right. We save until user arrives.
I suggest you review the extra complexity I added here for vendor driven alias
length, which I think we should do when an actual user comes along.
> There doesn't seem to be anything serializing use of alias_hash.
>
Each sha1 calculation is happening on the new descriptor allocated and
initialized using crypto_shash_init().
So it appears to me that each hash calculation can occur in parallel on the
individual desc.
> > +
> > parent = kzalloc(sizeof(*parent), GFP_KERNEL);
> > if (!parent) {
> > ret = -ENOMEM;
> > @@ -259,6 +275,7 @@ static void mdev_device_free(struct mdev_device
> *mdev)
> > mutex_unlock(_list_lock);
> >
> > dev_dbg(>dev, "MDEV: destroying\n");
> > + kvfree(mdev->alias);
> > kfree(mdev);
> > }
> >
> > @@ -269,18 +286,86 @@ static void mdev_device_release(struct device
> *dev)
> > mdev_device_free(mdev);
> > }
> >
> > -int mdev_device_create(struct kobject *kobj,
> > - struct device *dev, const guid_t *uuid)
> > +static const char *
> > +generate_alias(const char *uuid, unsigned int max_alias_len) {
> > + struct shash_desc *hash_desc;
> > + unsigned int digest_size;
> > + unsigned char *digest;
> > + unsigned int alias_len;
> > + char *alias;
> > + int ret = 0;
> > +
> > + /* Align to multiple of 2 as bin2hex will generate
> > +* even number of bytes.
> > +*/
>
> Comment style for non-networking code please
RE: [PATCH 3/4] mdev: Expose mdev alias in sysfs tree
> -Original Message-
> From: Alex Williamson
> Sent: Tuesday, August 27, 2019 7:24 AM
> To: Parav Pandit
> Cc: Jiri Pirko ; kwankh...@nvidia.com;
> coh...@redhat.com; da...@davemloft.net; k...@vger.kernel.org; linux-
> ker...@vger.kernel.org; net...@vger.kernel.org
> Subject: Re: [PATCH 3/4] mdev: Expose mdev alias in sysfs tree
>
> On Mon, 26 Aug 2019 15:41:18 -0500
> Parav Pandit wrote:
>
> > Expose mdev alias as string in a sysfs tree so that such attribute can
> > be used to generate netdevice name by systemd/udev or can be used to
> > match other kernel objects based on the alias of the mdev.
> >
> > Signed-off-by: Parav Pandit
> > ---
> > drivers/vfio/mdev/mdev_sysfs.c | 13 +
> > 1 file changed, 13 insertions(+)
> >
> > diff --git a/drivers/vfio/mdev/mdev_sysfs.c
> > b/drivers/vfio/mdev/mdev_sysfs.c index 43afe0e80b76..59f4e3cc5233
> > 100644
> > --- a/drivers/vfio/mdev/mdev_sysfs.c
> > +++ b/drivers/vfio/mdev/mdev_sysfs.c
> > @@ -246,7 +246,20 @@ static ssize_t remove_store(struct device *dev,
> > struct device_attribute *attr,
> >
> > static DEVICE_ATTR_WO(remove);
> >
> > +static ssize_t alias_show(struct device *device,
> > + struct device_attribute *attr, char *buf) {
> > + struct mdev_device *dev = mdev_from_dev(device);
> > +
> > + if (!dev->alias)
> > + return -EOPNOTSUPP;
>
> Wouldn't it be better to not create the alias at all? Thanks,
>
In other subsystem such as netdev sysfs files are always created that returns
either returns EOPNOTSUPP or attribute value.
I guess overhead of create multiple groups or creating individual sysfs files
outweigh the simplify of single group.
I think its ok to keep it simple this way.
> Alex
>
> > +
> > + return sprintf(buf, "%s\n", dev->alias); } static
> > +DEVICE_ATTR_RO(alias);
> > +
> > static const struct attribute *mdev_device_attrs[] = {
> > + _attr_alias.attr,
> > _attr_remove.attr,
> > NULL,
> > };
[PATCH 1/4] mdev: Introduce sha1 based mdev alias
Whenever a parent requests to generate mdev alias, generate a mdev
alias.
It is an optional attribute that parent can request to generate
for each of its child mdev.
mdev alias is generated using sha1 from the mdev name.
Signed-off-by: Parav Pandit
---
drivers/vfio/mdev/mdev_core.c| 98 +++-
drivers/vfio/mdev/mdev_private.h | 5 +-
drivers/vfio/mdev/mdev_sysfs.c | 13 +++--
include/linux/mdev.h | 4 ++
4 files changed, 111 insertions(+), 9 deletions(-)
diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
index b558d4cfd082..e825ff38b037 100644
--- a/drivers/vfio/mdev/mdev_core.c
+++ b/drivers/vfio/mdev/mdev_core.c
@@ -10,9 +10,11 @@
#include
#include
#include
+#include
#include
#include
#include
+#include
#include "mdev_private.h"
@@ -27,6 +29,8 @@ static struct class_compat *mdev_bus_compat_class;
static LIST_HEAD(mdev_list);
static DEFINE_MUTEX(mdev_list_lock);
+static struct crypto_shash *alias_hash;
+
struct device *mdev_parent_dev(struct mdev_device *mdev)
{
return mdev->parent->dev;
@@ -164,6 +168,18 @@ int mdev_register_device(struct device *dev, const struct
mdev_parent_ops *ops)
goto add_dev_err;
}
+ if (ops->get_alias_length) {
+ unsigned int digest_size;
+ unsigned int aligned_len;
+
+ aligned_len = roundup(ops->get_alias_length(), 2);
+ digest_size = crypto_shash_digestsize(alias_hash);
+ if (aligned_len / 2 > digest_size) {
+ ret = -EINVAL;
+ goto add_dev_err;
+ }
+ }
+
parent = kzalloc(sizeof(*parent), GFP_KERNEL);
if (!parent) {
ret = -ENOMEM;
@@ -259,6 +275,7 @@ static void mdev_device_free(struct mdev_device *mdev)
mutex_unlock(_list_lock);
dev_dbg(>dev, "MDEV: destroying\n");
+ kvfree(mdev->alias);
kfree(mdev);
}
@@ -269,18 +286,86 @@ static void mdev_device_release(struct device *dev)
mdev_device_free(mdev);
}
-int mdev_device_create(struct kobject *kobj,
- struct device *dev, const guid_t *uuid)
+static const char *
+generate_alias(const char *uuid, unsigned int max_alias_len)
+{
+ struct shash_desc *hash_desc;
+ unsigned int digest_size;
+ unsigned char *digest;
+ unsigned int alias_len;
+ char *alias;
+ int ret = 0;
+
+ /* Align to multiple of 2 as bin2hex will generate
+* even number of bytes.
+*/
+ alias_len = roundup(max_alias_len, 2);
+ alias = kvzalloc(alias_len + 1, GFP_KERNEL);
+ if (!alias)
+ return NULL;
+
+ /* Allocate and init descriptor */
+ hash_desc = kvzalloc(sizeof(*hash_desc) +
+crypto_shash_descsize(alias_hash),
+GFP_KERNEL);
+ if (!hash_desc)
+ goto desc_err;
+
+ hash_desc->tfm = alias_hash;
+
+ digest_size = crypto_shash_digestsize(alias_hash);
+
+ digest = kvzalloc(digest_size, GFP_KERNEL);
+ if (!digest) {
+ ret = -ENOMEM;
+ goto digest_err;
+ }
+ crypto_shash_init(hash_desc);
+ crypto_shash_update(hash_desc, uuid, UUID_STRING_LEN);
+ crypto_shash_final(hash_desc, digest);
+ bin2hex([0], digest,
+ min_t(unsigned int, digest_size, alias_len / 2));
+ /* When alias length is odd, zero out and additional last byte
+* that bin2hex has copied.
+*/
+ if (max_alias_len % 2)
+ alias[max_alias_len] = 0;
+
+ kvfree(digest);
+ kvfree(hash_desc);
+ return alias;
+
+digest_err:
+ kvfree(hash_desc);
+desc_err:
+ kvfree(alias);
+ return NULL;
+}
+
+int mdev_device_create(struct kobject *kobj, struct device *dev,
+ const char *uuid_str, const guid_t *uuid)
{
int ret;
struct mdev_device *mdev, *tmp;
struct mdev_parent *parent;
struct mdev_type *type = to_mdev_type(kobj);
+ unsigned int alias_len = 0;
+ const char *alias = NULL;
parent = mdev_get_parent(type->parent);
if (!parent)
return -EINVAL;
+ if (parent->ops->get_alias_length)
+ alias_len = parent->ops->get_alias_length();
+ if (alias_len) {
+ alias = generate_alias(uuid_str, alias_len);
+ if (!alias) {
+ ret = -ENOMEM;
+ goto alias_fail;
+ }
+ }
+
mutex_lock(_list_lock);
/* Check for duplicate */
@@ -300,6 +385,8 @@ int mdev_device_create(struct kobject *kobj,
}
guid_copy(>uuid, uuid);
+ mdev->alias = alias;
+ alias = NULL;
list_add(>next, _list);
mutex_unlock(_lis
[PATCH 4/4] mtty: Optionally support mtty alias
Provide a module parameter to set alias length to optionally generate
mdev alias.
Example to request mdev alias.
$ modprobe mtty alias_length=12
Signed-off-by: Parav Pandit
---
samples/vfio-mdev/mtty.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c
index 92e770a06ea2..92208245b057 100644
--- a/samples/vfio-mdev/mtty.c
+++ b/samples/vfio-mdev/mtty.c
@@ -1410,6 +1410,15 @@ static struct attribute_group *mdev_type_groups[] = {
NULL,
};
+static unsigned int mtty_alias_length;
+module_param_named(alias_length, mtty_alias_length, uint, 0444);
+MODULE_PARM_DESC(alias_length, "mdev alias length; default=0");
+
+static unsigned int mtty_get_alias_length(void)
+{
+ return mtty_alias_length;
+}
+
static const struct mdev_parent_ops mdev_fops = {
.owner = THIS_MODULE,
.dev_attr_groups= mtty_dev_groups,
@@ -1422,6 +1431,7 @@ static const struct mdev_parent_ops mdev_fops = {
.read = mtty_read,
.write = mtty_write,
.ioctl = mtty_ioctl,
+ .get_alias_length = mtty_get_alias_length
};
static void mtty_device_release(struct device *dev)
--
2.19.2
[PATCH 2/4] mdev: Make mdev alias unique among all mdevs
Mdev alias should be unique among all the mdevs, so that when such alias
is used by the mdev users to derive other objects, there is no
collision in a given system.
Signed-off-by: Parav Pandit
---
drivers/vfio/mdev/mdev_core.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
index e825ff38b037..6eb37f0c6369 100644
--- a/drivers/vfio/mdev/mdev_core.c
+++ b/drivers/vfio/mdev/mdev_core.c
@@ -375,6 +375,11 @@ int mdev_device_create(struct kobject *kobj, struct device
*dev,
ret = -EEXIST;
goto mdev_fail;
}
+ if (tmp->alias && strcmp(tmp->alias, alias) == 0) {
+ mutex_unlock(_list_lock);
+ ret = -EEXIST;
+ goto mdev_fail;
+ }
}
mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
--
2.19.2
[PATCH 3/4] mdev: Expose mdev alias in sysfs tree
Expose mdev alias as string in a sysfs tree so that such attribute can
be used to generate netdevice name by systemd/udev or can be used to
match other kernel objects based on the alias of the mdev.
Signed-off-by: Parav Pandit
---
drivers/vfio/mdev/mdev_sysfs.c | 13 +
1 file changed, 13 insertions(+)
diff --git a/drivers/vfio/mdev/mdev_sysfs.c b/drivers/vfio/mdev/mdev_sysfs.c
index 43afe0e80b76..59f4e3cc5233 100644
--- a/drivers/vfio/mdev/mdev_sysfs.c
+++ b/drivers/vfio/mdev/mdev_sysfs.c
@@ -246,7 +246,20 @@ static ssize_t remove_store(struct device *dev, struct
device_attribute *attr,
static DEVICE_ATTR_WO(remove);
+static ssize_t alias_show(struct device *device,
+ struct device_attribute *attr, char *buf)
+{
+ struct mdev_device *dev = mdev_from_dev(device);
+
+ if (!dev->alias)
+ return -EOPNOTSUPP;
+
+ return sprintf(buf, "%s\n", dev->alias);
+}
+static DEVICE_ATTR_RO(alias);
+
static const struct attribute *mdev_device_attrs[] = {
+ _attr_alias.attr,
_attr_remove.attr,
NULL,
};
--
2.19.2
[PATCH 0/4] Introduce variable length mdev alias
To have consistent naming for the netdevice of a mdev and to have consistent naming of the devlink port [1] of a mdev, which is formed using phys_port_name of the devlink port, current UUID is not usable because UUID is too long. UUID in string format is 36-characters long and in binary 128-bit. Both formats are not able to fit within 15 characters limit of netdev name. It is desired to have mdev device naming consistent using UUID. So that widely used user space framework such as ovs [2] can make use of mdev representor in similar way as PCIe SR-IOV VF and PF representors. Hence, (a) mdev alias is created which is derived using sha1 from the mdev name. (b) Vendor driver describes how long an alias should be for the child mdev created for a given parent. (c) Mdev aliases are unique at system level. (d) alias is created optionally whenever parent requested. This ensures that non networking mdev parents can function without alias creation overhead. This design is discussed at [3]. An example systemd/udev extension will have, 1. netdev name created using mdev alias available in sysfs. mdev UUID=83b8f4f2-509f-382f-3c1e-e6bfe0fa1001 mdev 12 character alias=cd5b146a80a5 netdev name of this mdev = enmcd5b146a80a5 Here en = Ethernet link m = mediated device 2. devlink port phys_port_name created using mdev alias. devlink phys_port_name=pcd5b146a80a5 This patchset enables mdev core to maintain unique alias for a mdev. Patch-1 Introduces mdev alias using sha1. Patch-2 Ensures that mdev alias is unique in a system. Patch-3 Exposes mdev alias in a sysfs hirerchy. Patch-4 Extends mtty driver to optionally provide alias generation. This also enables to test UUID based sha1 collision and trigger error handling for duplicate sha1 results. In future when networking driver wants to use mdev alias, mdev_alias() API will be added to derive devlink port name. [1] http://man7.org/linux/man-pages/man8/devlink-port.8.html [2] https://docs.openstack.org/os-vif/latest/user/plugins/ovs.html [3] https://patchwork.kernel.org/cover/11084231/ Parav Pandit (4): mdev: Introduce sha1 based mdev alias mdev: Make mdev alias unique among all mdevs mdev: Expose mdev alias in sysfs tree mtty: Optionally support mtty alias drivers/vfio/mdev/mdev_core.c| 103 ++- drivers/vfio/mdev/mdev_private.h | 5 +- drivers/vfio/mdev/mdev_sysfs.c | 26 ++-- include/linux/mdev.h | 4 ++ samples/vfio-mdev/mtty.c | 10 +++ 5 files changed, 139 insertions(+), 9 deletions(-) -- 2.19.2
RE: [PATCH v2 0/2] Simplify mtty driver and mdev core
> -Original Message- > From: Alex Williamson > Sent: Saturday, August 24, 2019 10:29 AM > To: Parav Pandit > Cc: Jiri Pirko ; Jiri Pirko ; David S . > Miller ; Kirti Wankhede ; > Cornelia Huck ; k...@vger.kernel.org; linux- > ker...@vger.kernel.org; cjia ; net...@vger.kernel.org > Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core > > On Sat, 24 Aug 2019 03:56:08 + > Parav Pandit wrote: > > > > -Original Message- > > > From: Alex Williamson > > > Sent: Saturday, August 24, 2019 1:14 AM > > > To: Parav Pandit > > > Cc: Jiri Pirko ; Jiri Pirko ; > > > David S . Miller ; Kirti Wankhede > > > ; Cornelia Huck ; > > > k...@vger.kernel.org; linux- ker...@vger.kernel.org; cjia > > > ; net...@vger.kernel.org > > > Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core > > > > > > On Fri, 23 Aug 2019 18:00:30 + > > > Parav Pandit wrote: > > > > > > > > -Original Message- > > > > > From: Alex Williamson > > > > > Sent: Friday, August 23, 2019 10:47 PM > > > > > To: Parav Pandit > > > > > Cc: Jiri Pirko ; Jiri Pirko > > > > > ; David S . Miller ; > > > > > Kirti Wankhede ; Cornelia Huck > > > > > ; k...@vger.kernel.org; linux- > > > > > ker...@vger.kernel.org; cjia ; > > > > > net...@vger.kernel.org > > > > > Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core > > > > > > > > > > On Fri, 23 Aug 2019 16:14:04 + Parav Pandit > > > > > wrote: > > > > > > > > > > > > > Idea is to have mdev alias as optional. > > > > > > > > Each mdev_parent says whether it wants mdev_core to > > > > > > > > generate an alias or not. So only networking device drivers > would set it to true. > > > > > > > > For rest, alias won't be generated, and won't be compared > > > > > > > > either during creation time. User continue to provide only uuid. > > > > > > > > > > > > > > Ok > > > > > > > > > > > > > > > I am tempted to have alias collision detection only within > > > > > > > > children mdevs of the same parent, but doing so will > > > > > > > > always mandate to prefix in netdev name. And currently we > > > > > > > > are left with only 3 characters to prefix it, so that may not be > good either. > > > > > > > > Hence, I think mdev core wide alias is better with 12 > > > > > > > > characters. > > > > > > > > > > > > > > I suppose it depends on the API, if the vendor driver can > > > > > > > ask the mdev core for an alias as part of the device > > > > > > > creation process, then it could manage the netdev namespace > > > > > > > for all its devices, choosing how many characters to use, > > > > > > > and fail the creation if it can't meet a uniqueness > > > > > > > requirement. IOW, mdev-core would always provide a full > > > > > > > sha1 and therefore gets itself out of the uniqueness/collision > aspects. > > > > > > > > > > > > > This doesn't work. At mdev core level 20 bytes sha1 are > > > > > > unique, so mdev core allowed to create a mdev. > > > > > > > > > > The mdev vendor driver has the opportunity to fail the device > > > > > creation in mdev_parent_ops.create(). > > > > > > > > > That is not helpful for below reasons. > > > > 1. vendor driver doesn't have visibility in other vendor's alias. > > > > 2. Even for single vendor, it needs to maintain global list of > > > > devices to see > > > collision. > > > > 3. multiple vendors needs to implement same scheme. > > > > > > > > Mdev core should be the owner. Shifting ownership from one layer > > > > to a lower layer in vendor driver doesn't solve the problem (if > > > > there is one, which I think doesn't exist). > > > > > > > > > > And then devlink core chooses > > > > > > only 6 bytes (12 characters) and there is collision. Things > > > > > > fall apart. Since mdev provides unique uuid based scheme, it's > > > > >
RE: [PATCH v2 0/2] Simplify mtty driver and mdev core
Hi Alex, > -Original Message- > From: linux-kernel-ow...@vger.kernel.org ow...@vger.kernel.org> On Behalf Of Parav Pandit > Sent: Saturday, August 24, 2019 9:26 AM > To: Alex Williamson > Cc: Jiri Pirko ; Jiri Pirko ; David S . > Miller ; Kirti Wankhede ; > Cornelia Huck ; k...@vger.kernel.org; linux- > ker...@vger.kernel.org; cjia ; net...@vger.kernel.org > Subject: RE: [PATCH v2 0/2] Simplify mtty driver and mdev core > > I don't understand this logic. I'm simply asking that we have a way > > to test the collision behavior without changing the binary. The path > > we're driving towards seems to be making this easier and easier. If > > the vendor can request an alias of a specific length, then a sample > > driver with a module option to set the desired alias length to 1-char makes > it trivially easy to induce a collision. > Sure it is easy to test collision, but my point is - mdev core is not sha1 > test > module. > Hence adding functionality of variable alias length to test collision doesn't > make sense. > When the actual user arrives who needs small alias, we will be able to add > additional pieces very easily. My initial thoughts to add parent_ops to have bool flag to generate alias or not. However, instead of bool, keeping it unsigned int to say, zero to skip alias and non-zero length to convey generate alias. This will serve both the purpose with trivial handling.
RE: [PATCH v2 0/2] Simplify mtty driver and mdev core
> -Original Message- > From: Alex Williamson > Sent: Saturday, August 24, 2019 1:14 AM > To: Parav Pandit > Cc: Jiri Pirko ; Jiri Pirko ; David S . > Miller > ; Kirti Wankhede ; Cornelia > Huck ; k...@vger.kernel.org; linux- > ker...@vger.kernel.org; cjia ; net...@vger.kernel.org > Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core > > On Fri, 23 Aug 2019 18:00:30 + > Parav Pandit wrote: > > > > -Original Message- > > > From: Alex Williamson > > > Sent: Friday, August 23, 2019 10:47 PM > > > To: Parav Pandit > > > Cc: Jiri Pirko ; Jiri Pirko ; > > > David S . Miller ; Kirti Wankhede > > > ; Cornelia Huck ; > > > k...@vger.kernel.org; linux- ker...@vger.kernel.org; cjia > > > ; net...@vger.kernel.org > > > Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core > > > > > > On Fri, 23 Aug 2019 16:14:04 + > > > Parav Pandit wrote: > > > > > > > > > Idea is to have mdev alias as optional. > > > > > > Each mdev_parent says whether it wants mdev_core to generate > > > > > > an alias or not. So only networking device drivers would set it to > > > > > > true. > > > > > > For rest, alias won't be generated, and won't be compared > > > > > > either during creation time. User continue to provide only uuid. > > > > > > > > > > Ok > > > > > > > > > > > I am tempted to have alias collision detection only within > > > > > > children mdevs of the same parent, but doing so will always > > > > > > mandate to prefix in netdev name. And currently we are left > > > > > > with only 3 characters to prefix it, so that may not be good either. > > > > > > Hence, I think mdev core wide alias is better with 12 characters. > > > > > > > > > > I suppose it depends on the API, if the vendor driver can ask > > > > > the mdev core for an alias as part of the device creation > > > > > process, then it could manage the netdev namespace for all its > > > > > devices, choosing how many characters to use, and fail the > > > > > creation if it can't meet a uniqueness requirement. IOW, > > > > > mdev-core would always provide a full > > > > > sha1 and therefore gets itself out of the uniqueness/collision > > > > > aspects. > > > > > > > > > This doesn't work. At mdev core level 20 bytes sha1 are unique, so > > > > mdev core allowed to create a mdev. > > > > > > The mdev vendor driver has the opportunity to fail the device > > > creation in mdev_parent_ops.create(). > > > > > That is not helpful for below reasons. > > 1. vendor driver doesn't have visibility in other vendor's alias. > > 2. Even for single vendor, it needs to maintain global list of devices to > > see > collision. > > 3. multiple vendors needs to implement same scheme. > > > > Mdev core should be the owner. Shifting ownership from one layer to a > > lower layer in vendor driver doesn't solve the problem (if there is > > one, which I think doesn't exist). > > > > > > And then devlink core chooses > > > > only 6 bytes (12 characters) and there is collision. Things fall > > > > apart. Since mdev provides unique uuid based scheme, it's the mdev > > > > core's ownership to provide unique aliases. > > > > > > You're suggesting/contemplating multiple solutions here, 3-char > > > prefix + 12- char sha1 vs + ?-char sha1. Also, the > > > 15-char total limit is imposed by an external subsystem, where the > > > vendor driver is the gateway between that subsystem and mdev. How > > > would mdev integrate with another subsystem that maybe only has > > > 9-chars available? Would the vendor driver API specify "I need an > > > alias" or would it specify "I need an X-char length alias"? > > Yes, Vendor driver should say how long the alias it wants. > > However before we implement that, I suggest let such > > vendor/user/driver arrive which needs that. Such variable length alias > > can be added at that time and even with that alias collision can be > > detected by single mdev module. > > If we agree that different alias lengths are possible, then I would request > that > minimally an mdev sample driver be modified to request an alias with a length > that can be adjusted without r
RE: [PATCH v2 0/2] Simplify mtty driver and mdev core
> -Original Message- > From: Alex Williamson > Sent: Friday, August 23, 2019 10:47 PM > To: Parav Pandit > Cc: Jiri Pirko ; Jiri Pirko ; David S . > Miller > ; Kirti Wankhede ; Cornelia > Huck ; k...@vger.kernel.org; linux- > ker...@vger.kernel.org; cjia ; net...@vger.kernel.org > Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core > > On Fri, 23 Aug 2019 16:14:04 + > Parav Pandit wrote: > > > > > Idea is to have mdev alias as optional. > > > > Each mdev_parent says whether it wants mdev_core to generate an > > > > alias or not. So only networking device drivers would set it to true. > > > > For rest, alias won't be generated, and won't be compared either > > > > during creation time. User continue to provide only uuid. > > > > > > Ok > > > > > > > I am tempted to have alias collision detection only within > > > > children mdevs of the same parent, but doing so will always > > > > mandate to prefix in netdev name. And currently we are left with > > > > only 3 characters to prefix it, so that may not be good either. > > > > Hence, I think mdev core wide alias is better with 12 characters. > > > > > > I suppose it depends on the API, if the vendor driver can ask the > > > mdev core for an alias as part of the device creation process, then > > > it could manage the netdev namespace for all its devices, choosing > > > how many characters to use, and fail the creation if it can't meet a > > > uniqueness requirement. IOW, mdev-core would always provide a full > > > sha1 and therefore gets itself out of the uniqueness/collision aspects. > > > > > This doesn't work. At mdev core level 20 bytes sha1 are unique, so > > mdev core allowed to create a mdev. > > The mdev vendor driver has the opportunity to fail the device creation in > mdev_parent_ops.create(). > That is not helpful for below reasons. 1. vendor driver doesn't have visibility in other vendor's alias. 2. Even for single vendor, it needs to maintain global list of devices to see collision. 3. multiple vendors needs to implement same scheme. Mdev core should be the owner. Shifting ownership from one layer to a lower layer in vendor driver doesn't solve the problem (if there is one, which I think doesn't exist). > > And then devlink core chooses > > only 6 bytes (12 characters) and there is collision. Things fall > > apart. Since mdev provides unique uuid based scheme, it's the mdev > > core's ownership to provide unique aliases. > > You're suggesting/contemplating multiple solutions here, 3-char prefix + 12- > char sha1 vs + ?-char sha1. Also, the 15-char total limit is > imposed by an external subsystem, where the vendor driver is the gateway > between that subsystem and mdev. How would mdev integrate with another > subsystem that maybe only has 9-chars available? Would the vendor driver API > specify "I need an alias" or would it specify "I need an X-char length alias"? Yes, Vendor driver should say how long the alias it wants. However before we implement that, I suggest let such vendor/user/driver arrive which needs that. Such variable length alias can be added at that time and even with that alias collision can be detected by single mdev module. > Does it make sense that mdev-core would fail creation of a device if there's a > collision in the 12-char address space between different subsystems? For > example, does enm0123456789ab really collide with xyz0123456789ab? I think so, because at mdev level its 12-char alias matters. Choosing the prefix not adding prefix is really a user space choice. > So if > mdev were to provided a 40-char sha1, is it possible that the vendor driver > could consume this in its create callback, truncate it to the number of chars > required by the vendor driver's subsystem, and determine whether a collision > exists? We shouldn't shift the problem from mdev to multiple vendor drivers to detect collision. I still think that user providing alias is better because it knows the use-case system in use, and eliminates these collision issue. > > > > > I do not understand how an extra character reduces collision, if > > > > that's what you meant. > > > > > > If the default were for example 3-chars, we might already have > > > device 'abc'. A collision would expose one more char of the new > > > device, so we might add device with alias 'abcd'. I mentioned > > > previously that this leaves an issue for userspace that we can't > > > change the alias of device abc, so without additional information, > > > userspace c
RE: [PATCH v2 0/2] Simplify mtty driver and mdev core
> -Original Message- > From: Alex Williamson > Sent: Friday, August 23, 2019 9:22 PM > To: Parav Pandit > Cc: Jiri Pirko ; Jiri Pirko ; David S . > Miller > ; Kirti Wankhede ; Cornelia > Huck ; k...@vger.kernel.org; linux- > ker...@vger.kernel.org; cjia ; net...@vger.kernel.org > Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core > > On Fri, 23 Aug 2019 14:53:06 + > Parav Pandit wrote: > > > > -Original Message- > > > From: Alex Williamson > > > Sent: Friday, August 23, 2019 7:58 PM > > > To: Parav Pandit > > > Cc: Jiri Pirko ; Jiri Pirko ; > > > David S . Miller ; Kirti Wankhede > > > ; Cornelia Huck ; > > > k...@vger.kernel.org; linux- ker...@vger.kernel.org; cjia > > > ; net...@vger.kernel.org > > > Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core > > > > > > On Fri, 23 Aug 2019 08:14:39 + > > > Parav Pandit wrote: > > > > > > > Hi Alex, > > > > > > > > > > > > > -Original Message- > > > > > From: Jiri Pirko > > > > > Sent: Friday, August 23, 2019 1:42 PM > > > > > To: Parav Pandit > > > > > Cc: Alex Williamson ; Jiri Pirko > > > > > ; David S . Miller ; > > > > > Kirti Wankhede ; Cornelia Huck > > > ; > > > > > k...@vger.kernel.org; linux-kernel@vger.kernel.org; cjia > > > > > ; net...@vger.kernel.org > > > > > Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core > > > > > > > > > > Thu, Aug 22, 2019 at 03:33:30PM CEST, pa...@mellanox.com wrote: > > > > > > > > > > > > > > > > > >> -Original Message- > > > > > >> From: Jiri Pirko > > > > > >> Sent: Thursday, August 22, 2019 5:50 PM > > > > > >> To: Parav Pandit > > > > > >> Cc: Alex Williamson ; Jiri Pirko > > > > > >> ; David S . Miller ; > > > > > >> Kirti Wankhede ; Cornelia Huck > > > > > ; > > > > > >> k...@vger.kernel.org; linux-kernel@vger.kernel.org; cjia > > > > > >> ; net...@vger.kernel.org > > > > > >> Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev > > > > > >> core > > > > > >> > > > > > >> Thu, Aug 22, 2019 at 12:04:02PM CEST, pa...@mellanox.com wrote: > > > > > >> > > > > > > >> > > > > > > >> >> -Original Message- > > > > > >> >> From: Jiri Pirko > > > > > >> >> Sent: Thursday, August 22, 2019 3:28 PM > > > > > >> >> To: Parav Pandit > > > > > >> >> Cc: Alex Williamson ; Jiri > > > > > >> >> Pirko ; David S . Miller > > > > > >> >> ; Kirti Wankhede > > > > > >> >> ; Cornelia Huck > > > > > >> ; > > > > > >> >> k...@vger.kernel.org; linux-kernel@vger.kernel.org; cjia > > > > > >> >> ; net...@vger.kernel.org > > > > > >> >> Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev > > > > > >> >> core > > > > > >> >> > > > > > >> >> Thu, Aug 22, 2019 at 11:42:13AM CEST, pa...@mellanox.com > wrote: > > > > > >> >> > > > > > > >> >> > > > > > > >> >> >> -Original Message- > > > > > >> >> >> From: Jiri Pirko > > > > > >> >> >> Sent: Thursday, August 22, 2019 2:59 PM > > > > > >> >> >> To: Parav Pandit > > > > > >> >> >> Cc: Alex Williamson ; Jiri > > > > > >> >> >> Pirko ; David S . Miller > > > > > >> >> >> ; Kirti Wankhede > > > > > >> >> >> ; Cornelia Huck > > > > > >> >> ; > > > > > >> >> >> k...@vger.kernel.org; linux-kernel@vger.kernel.org; cjia > > > > > >> >> >> ; net...@vger.kernel.org > > > > > >> >> >> Subject: Re: [PATCH v2 0/2] Simplify mtty driver and > > > > > >> >> &g
RE: [PATCH v2 0/2] Simplify mtty driver and mdev core
> -Original Message- > From: Alex Williamson > Sent: Friday, August 23, 2019 7:58 PM > To: Parav Pandit > Cc: Jiri Pirko ; Jiri Pirko ; David S . > Miller > ; Kirti Wankhede ; Cornelia > Huck ; k...@vger.kernel.org; linux- > ker...@vger.kernel.org; cjia ; net...@vger.kernel.org > Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core > > On Fri, 23 Aug 2019 08:14:39 + > Parav Pandit wrote: > > > Hi Alex, > > > > > > > -Original Message- > > > From: Jiri Pirko > > > Sent: Friday, August 23, 2019 1:42 PM > > > To: Parav Pandit > > > Cc: Alex Williamson ; Jiri Pirko > > > ; David S . Miller ; Kirti > > > Wankhede ; Cornelia Huck > ; > > > k...@vger.kernel.org; linux-kernel@vger.kernel.org; cjia > > > ; net...@vger.kernel.org > > > Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core > > > > > > Thu, Aug 22, 2019 at 03:33:30PM CEST, pa...@mellanox.com wrote: > > > > > > > > > > > >> -Original Message- > > > >> From: Jiri Pirko > > > >> Sent: Thursday, August 22, 2019 5:50 PM > > > >> To: Parav Pandit > > > >> Cc: Alex Williamson ; Jiri Pirko > > > >> ; David S . Miller ; > > > >> Kirti Wankhede ; Cornelia Huck > > > ; > > > >> k...@vger.kernel.org; linux-kernel@vger.kernel.org; cjia > > > >> ; net...@vger.kernel.org > > > >> Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core > > > >> > > > >> Thu, Aug 22, 2019 at 12:04:02PM CEST, pa...@mellanox.com wrote: > > > >> > > > > >> > > > > >> >> -Original Message- > > > >> >> From: Jiri Pirko > > > >> >> Sent: Thursday, August 22, 2019 3:28 PM > > > >> >> To: Parav Pandit > > > >> >> Cc: Alex Williamson ; Jiri Pirko > > > >> >> ; David S . Miller ; > > > >> >> Kirti Wankhede ; Cornelia Huck > > > >> ; > > > >> >> k...@vger.kernel.org; linux-kernel@vger.kernel.org; cjia > > > >> >> ; net...@vger.kernel.org > > > >> >> Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core > > > >> >> > > > >> >> Thu, Aug 22, 2019 at 11:42:13AM CEST, pa...@mellanox.com wrote: > > > >> >> > > > > >> >> > > > > >> >> >> -Original Message- > > > >> >> >> From: Jiri Pirko > > > >> >> >> Sent: Thursday, August 22, 2019 2:59 PM > > > >> >> >> To: Parav Pandit > > > >> >> >> Cc: Alex Williamson ; Jiri > > > >> >> >> Pirko ; David S . Miller > > > >> >> >> ; Kirti Wankhede > > > >> >> >> ; Cornelia Huck > > > >> >> ; > > > >> >> >> k...@vger.kernel.org; linux-kernel@vger.kernel.org; cjia > > > >> >> >> ; net...@vger.kernel.org > > > >> >> >> Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev > > > >> >> >> core > > > >> >> >> > > > >> >> >> Wed, Aug 21, 2019 at 08:23:17AM CEST, pa...@mellanox.com > wrote: > > > >> >> >> > > > > >> >> >> > > > > >> >> >> >> -Original Message- > > > >> >> >> >> From: Alex Williamson > > > >> >> >> >> Sent: Wednesday, August 21, 2019 10:56 AM > > > >> >> >> >> To: Parav Pandit > > > >> >> >> >> Cc: Jiri Pirko ; David S . Miller > > > >> >> >> >> ; Kirti Wankhede > > > >> >> >> >> ; Cornelia Huck > > > >> >> >> >> ; k...@vger.kernel.org; > > > >> >> >> >> linux-kernel@vger.kernel.org; cjia ; > > > >> >> >> >> net...@vger.kernel.org > > > >> >> >> >> Subject: Re: [PATCH v2 0/2] Simplify mtty driver and > > > >> >> >> >> mdev core > > > >> >> >> >> > > > >> >> >> >> > > > > Just an exa
RE: [PATCH v2 0/2] Simplify mtty driver and mdev core
Hi Alex, > -Original Message- > From: Jiri Pirko > Sent: Friday, August 23, 2019 1:42 PM > To: Parav Pandit > Cc: Alex Williamson ; Jiri Pirko > ; David S . Miller ; Kirti > Wankhede ; Cornelia Huck ; > k...@vger.kernel.org; linux-kernel@vger.kernel.org; cjia ; > net...@vger.kernel.org > Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core > > Thu, Aug 22, 2019 at 03:33:30PM CEST, pa...@mellanox.com wrote: > > > > > >> -Original Message- > >> From: Jiri Pirko > >> Sent: Thursday, August 22, 2019 5:50 PM > >> To: Parav Pandit > >> Cc: Alex Williamson ; Jiri Pirko > >> ; David S . Miller ; Kirti > >> Wankhede ; Cornelia Huck > ; > >> k...@vger.kernel.org; linux-kernel@vger.kernel.org; cjia > >> ; net...@vger.kernel.org > >> Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core > >> > >> Thu, Aug 22, 2019 at 12:04:02PM CEST, pa...@mellanox.com wrote: > >> > > >> > > >> >> -Original Message- > >> >> From: Jiri Pirko > >> >> Sent: Thursday, August 22, 2019 3:28 PM > >> >> To: Parav Pandit > >> >> Cc: Alex Williamson ; Jiri Pirko > >> >> ; David S . Miller ; Kirti > >> >> Wankhede ; Cornelia Huck > >> ; > >> >> k...@vger.kernel.org; linux-kernel@vger.kernel.org; cjia > >> >> ; net...@vger.kernel.org > >> >> Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core > >> >> > >> >> Thu, Aug 22, 2019 at 11:42:13AM CEST, pa...@mellanox.com wrote: > >> >> > > >> >> > > >> >> >> -Original Message- > >> >> >> From: Jiri Pirko > >> >> >> Sent: Thursday, August 22, 2019 2:59 PM > >> >> >> To: Parav Pandit > >> >> >> Cc: Alex Williamson ; Jiri Pirko > >> >> >> ; David S . Miller ; > >> >> >> Kirti Wankhede ; Cornelia Huck > >> >> ; > >> >> >> k...@vger.kernel.org; linux-kernel@vger.kernel.org; cjia > >> >> >> ; net...@vger.kernel.org > >> >> >> Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core > >> >> >> > >> >> >> Wed, Aug 21, 2019 at 08:23:17AM CEST, pa...@mellanox.com wrote: > >> >> >> > > >> >> >> > > >> >> >> >> -Original Message- > >> >> >> >> From: Alex Williamson > >> >> >> >> Sent: Wednesday, August 21, 2019 10:56 AM > >> >> >> >> To: Parav Pandit > >> >> >> >> Cc: Jiri Pirko ; David S . Miller > >> >> >> >> ; Kirti Wankhede > >> >> >> >> ; Cornelia Huck ; > >> >> >> >> k...@vger.kernel.org; linux-kernel@vger.kernel.org; cjia > >> >> >> >> ; net...@vger.kernel.org > >> >> >> >> Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev > >> >> >> >> core > >> >> >> >> > >> >> >> >> > > > > Just an example of the alias, not proposing how it's set. > >> >> >> >> > > > > In fact, proposing that the user does not set it, > >> >> >> >> > > > > mdev-core provides one > >> >> >> >> > > automatically. > >> >> >> >> > > > > > >> >> >> >> > > > > > > Since there seems to be some prefix overhead, as > >> >> >> >> > > > > > > I ask about above in how many characters we > >> >> >> >> > > > > > > actually have to work with in IFNAMESZ, maybe we > >> >> >> >> > > > > > > start with > >> >> >> >> > > > > > > 8 characters (matching your "index" namespace) > >> >> >> >> > > > > > > and expand as necessary for > >> >> >> disambiguation. > >> >> >> >> > > > > > > If we can eliminate overhead in IFNAMESZ, let's > >> >> >> >> > > > > > > start with > >> 12. > >> >> >> >> > > > > > > Thanks,
RE: [PATCH v2 0/2] Simplify mtty driver and mdev core
> -Original Message- > From: Jiri Pirko > Sent: Thursday, August 22, 2019 5:50 PM > To: Parav Pandit > Cc: Alex Williamson ; Jiri Pirko > ; David S . Miller ; Kirti > Wankhede ; Cornelia Huck ; > k...@vger.kernel.org; linux-kernel@vger.kernel.org; cjia ; > net...@vger.kernel.org > Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core > > Thu, Aug 22, 2019 at 12:04:02PM CEST, pa...@mellanox.com wrote: > > > > > >> -Original Message- > >> From: Jiri Pirko > >> Sent: Thursday, August 22, 2019 3:28 PM > >> To: Parav Pandit > >> Cc: Alex Williamson ; Jiri Pirko > >> ; David S . Miller ; Kirti > >> Wankhede ; Cornelia Huck > ; > >> k...@vger.kernel.org; linux-kernel@vger.kernel.org; cjia > >> ; net...@vger.kernel.org > >> Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core > >> > >> Thu, Aug 22, 2019 at 11:42:13AM CEST, pa...@mellanox.com wrote: > >> > > >> > > >> >> -Original Message- > >> >> From: Jiri Pirko > >> >> Sent: Thursday, August 22, 2019 2:59 PM > >> >> To: Parav Pandit > >> >> Cc: Alex Williamson ; Jiri Pirko > >> >> ; David S . Miller ; Kirti > >> >> Wankhede ; Cornelia Huck > >> ; > >> >> k...@vger.kernel.org; linux-kernel@vger.kernel.org; cjia > >> >> ; net...@vger.kernel.org > >> >> Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core > >> >> > >> >> Wed, Aug 21, 2019 at 08:23:17AM CEST, pa...@mellanox.com wrote: > >> >> > > >> >> > > >> >> >> -Original Message- > >> >> >> From: Alex Williamson > >> >> >> Sent: Wednesday, August 21, 2019 10:56 AM > >> >> >> To: Parav Pandit > >> >> >> Cc: Jiri Pirko ; David S . Miller > >> >> >> ; Kirti Wankhede ; > >> >> >> Cornelia Huck ; k...@vger.kernel.org; > >> >> >> linux-kernel@vger.kernel.org; cjia ; > >> >> >> net...@vger.kernel.org > >> >> >> Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core > >> >> >> > >> >> >> > > > > Just an example of the alias, not proposing how it's set. > >> >> >> > > > > In fact, proposing that the user does not set it, > >> >> >> > > > > mdev-core provides one > >> >> >> > > automatically. > >> >> >> > > > > > >> >> >> > > > > > > Since there seems to be some prefix overhead, as I > >> >> >> > > > > > > ask about above in how many characters we actually > >> >> >> > > > > > > have to work with in IFNAMESZ, maybe we start with > >> >> >> > > > > > > 8 characters (matching your "index" namespace) and > >> >> >> > > > > > > expand as necessary for > >> >> disambiguation. > >> >> >> > > > > > > If we can eliminate overhead in IFNAMESZ, let's start > >> >> >> > > > > > > with > 12. > >> >> >> > > > > > > Thanks, > >> >> >> > > > > > > > >> >> >> > > > > > If user is going to choose the alias, why does it > >> >> >> > > > > > have to be limited to > >> >> >> sha1? > >> >> >> > > > > > Or you just told it as an example? > >> >> >> > > > > > > >> >> >> > > > > > It can be an alpha-numeric string. > >> >> >> > > > > > >> >> >> > > > > No, I'm proposing a different solution where mdev-core > >> >> >> > > > > creates an alias based on an abbreviated sha1. The > >> >> >> > > > > user does not provide the > >> >> >> alias. > >> >> >> > > > > > >> >> >> > > > > > Instead of mdev imposing number of characters on the > >> >> >> > > > > > alias, it should be best > >> >> >> > > > > left to the user. &g
RE: [PATCH v2 0/2] Simplify mtty driver and mdev core
> -Original Message- > From: Jiri Pirko > Sent: Thursday, August 22, 2019 3:28 PM > To: Parav Pandit > Cc: Alex Williamson ; Jiri Pirko > ; David S . Miller ; Kirti > Wankhede ; Cornelia Huck ; > k...@vger.kernel.org; linux-kernel@vger.kernel.org; cjia ; > net...@vger.kernel.org > Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core > > Thu, Aug 22, 2019 at 11:42:13AM CEST, pa...@mellanox.com wrote: > > > > > >> -Original Message- > >> From: Jiri Pirko > >> Sent: Thursday, August 22, 2019 2:59 PM > >> To: Parav Pandit > >> Cc: Alex Williamson ; Jiri Pirko > >> ; David S . Miller ; Kirti > >> Wankhede ; Cornelia Huck > ; > >> k...@vger.kernel.org; linux-kernel@vger.kernel.org; cjia > >> ; net...@vger.kernel.org > >> Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core > >> > >> Wed, Aug 21, 2019 at 08:23:17AM CEST, pa...@mellanox.com wrote: > >> > > >> > > >> >> -Original Message- > >> >> From: Alex Williamson > >> >> Sent: Wednesday, August 21, 2019 10:56 AM > >> >> To: Parav Pandit > >> >> Cc: Jiri Pirko ; David S . Miller > >> >> ; Kirti Wankhede ; > >> >> Cornelia Huck ; k...@vger.kernel.org; > >> >> linux-kernel@vger.kernel.org; cjia ; > >> >> net...@vger.kernel.org > >> >> Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core > >> >> > >> >> > > > > Just an example of the alias, not proposing how it's set. > >> >> > > > > In fact, proposing that the user does not set it, > >> >> > > > > mdev-core provides one > >> >> > > automatically. > >> >> > > > > > >> >> > > > > > > Since there seems to be some prefix overhead, as I ask > >> >> > > > > > > about above in how many characters we actually have to > >> >> > > > > > > work with in IFNAMESZ, maybe we start with 8 > >> >> > > > > > > characters (matching your "index" namespace) and > >> >> > > > > > > expand as necessary for > >> disambiguation. > >> >> > > > > > > If we can eliminate overhead in IFNAMESZ, let's start with > >> >> > > > > > > 12. > >> >> > > > > > > Thanks, > >> >> > > > > > > > >> >> > > > > > If user is going to choose the alias, why does it have > >> >> > > > > > to be limited to > >> >> sha1? > >> >> > > > > > Or you just told it as an example? > >> >> > > > > > > >> >> > > > > > It can be an alpha-numeric string. > >> >> > > > > > >> >> > > > > No, I'm proposing a different solution where mdev-core > >> >> > > > > creates an alias based on an abbreviated sha1. The user > >> >> > > > > does not provide the > >> >> alias. > >> >> > > > > > >> >> > > > > > Instead of mdev imposing number of characters on the > >> >> > > > > > alias, it should be best > >> >> > > > > left to the user. > >> >> > > > > > Because in future if netdev improves on the naming > >> >> > > > > > scheme, mdev will be > >> >> > > > > limiting it, which is not right. > >> >> > > > > > So not restricting alias size seems right to me. > >> >> > > > > > User configuring mdev for networking devices in a given > >> >> > > > > > kernel knows what > >> >> > > > > user is doing. > >> >> > > > > > So user can choose alias name size as it finds suitable. > >> >> > > > > > >> >> > > > > That's not what I'm proposing, please read again. Thanks, > >> >> > > > > >> >> > > > I understood your point. But mdev doesn't know how user is > >> >> > > > going to use > >> >> > > udev/systemd to name the netdev. > >> >> > > > So even if mdev chose to
RE: [PATCH v2 0/2] Simplify mtty driver and mdev core
> -Original Message- > From: Jiri Pirko > Sent: Thursday, August 22, 2019 2:59 PM > To: Parav Pandit > Cc: Alex Williamson ; Jiri Pirko > ; David S . Miller ; Kirti > Wankhede ; Cornelia Huck ; > k...@vger.kernel.org; linux-kernel@vger.kernel.org; cjia ; > net...@vger.kernel.org > Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core > > Wed, Aug 21, 2019 at 08:23:17AM CEST, pa...@mellanox.com wrote: > > > > > >> -Original Message- > >> From: Alex Williamson > >> Sent: Wednesday, August 21, 2019 10:56 AM > >> To: Parav Pandit > >> Cc: Jiri Pirko ; David S . Miller > >> ; Kirti Wankhede ; > >> Cornelia Huck ; k...@vger.kernel.org; > >> linux-kernel@vger.kernel.org; cjia ; > >> net...@vger.kernel.org > >> Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core > >> > >> > > > > Just an example of the alias, not proposing how it's set. In > >> > > > > fact, proposing that the user does not set it, mdev-core > >> > > > > provides one > >> > > automatically. > >> > > > > > >> > > > > > > Since there seems to be some prefix overhead, as I ask > >> > > > > > > about above in how many characters we actually have to > >> > > > > > > work with in IFNAMESZ, maybe we start with 8 characters > >> > > > > > > (matching your "index" namespace) and expand as necessary for > disambiguation. > >> > > > > > > If we can eliminate overhead in IFNAMESZ, let's start with 12. > >> > > > > > > Thanks, > >> > > > > > > > >> > > > > > If user is going to choose the alias, why does it have to > >> > > > > > be limited to > >> sha1? > >> > > > > > Or you just told it as an example? > >> > > > > > > >> > > > > > It can be an alpha-numeric string. > >> > > > > > >> > > > > No, I'm proposing a different solution where mdev-core > >> > > > > creates an alias based on an abbreviated sha1. The user does > >> > > > > not provide the > >> alias. > >> > > > > > >> > > > > > Instead of mdev imposing number of characters on the alias, > >> > > > > > it should be best > >> > > > > left to the user. > >> > > > > > Because in future if netdev improves on the naming scheme, > >> > > > > > mdev will be > >> > > > > limiting it, which is not right. > >> > > > > > So not restricting alias size seems right to me. > >> > > > > > User configuring mdev for networking devices in a given > >> > > > > > kernel knows what > >> > > > > user is doing. > >> > > > > > So user can choose alias name size as it finds suitable. > >> > > > > > >> > > > > That's not what I'm proposing, please read again. Thanks, > >> > > > > >> > > > I understood your point. But mdev doesn't know how user is > >> > > > going to use > >> > > udev/systemd to name the netdev. > >> > > > So even if mdev chose to pick 12 characters, it could result in > >> > > > collision. > >> > > > Hence the proposal to provide the alias by the user, as user > >> > > > know the best > >> > > policy for its use case in the environment its using. > >> > > > So 12 character sha1 method will still work by user. > >> > > > >> > > Haven't you already provided examples where certain drivers or > >> > > subsystems have unique netdev prefixes? If mdev provides a > >> > > unique alias within the subsystem, couldn't we simply define a > >> > > netdev prefix for the mdev subsystem and avoid all other > >> > > collisions? I'm not in favor of the user providing both a uuid > >> > > and an alias/instance. Thanks, > >> > > > >> > For a given prefix, say ens2f0, can two UUID->sha1 first 9 > >> > characters have > >> collision? > >> > >> I think it would be a mistake to waste so many chars on a prefix, but > >> 9 characters of sha1 likely wouldn't have a
RE: [PATCH v2 0/2] Simplify mtty driver and mdev core
> -Original Message- > From: Alex Williamson > Sent: Wednesday, August 21, 2019 10:56 AM > To: Parav Pandit > Cc: Jiri Pirko ; David S . Miller ; > Kirti Wankhede ; Cornelia Huck > ; k...@vger.kernel.org; linux-kernel@vger.kernel.org; > cjia ; net...@vger.kernel.org > Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core > > > > > > Just an example of the alias, not proposing how it's set. In > > > > > fact, proposing that the user does not set it, mdev-core > > > > > provides one > > > automatically. > > > > > > > > > > > > Since there seems to be some prefix overhead, as I ask about > > > > > > > above in how many characters we actually have to work with > > > > > > > in IFNAMESZ, maybe we start with 8 characters (matching your > > > > > > > "index" namespace) and expand as necessary for disambiguation. > > > > > > > If we can eliminate overhead in IFNAMESZ, let's start with 12. > > > > > > > Thanks, > > > > > > > > > > > > > If user is going to choose the alias, why does it have to be > > > > > > limited to > sha1? > > > > > > Or you just told it as an example? > > > > > > > > > > > > It can be an alpha-numeric string. > > > > > > > > > > No, I'm proposing a different solution where mdev-core creates > > > > > an alias based on an abbreviated sha1. The user does not provide the > alias. > > > > > > > > > > > Instead of mdev imposing number of characters on the alias, it > > > > > > should be best > > > > > left to the user. > > > > > > Because in future if netdev improves on the naming scheme, > > > > > > mdev will be > > > > > limiting it, which is not right. > > > > > > So not restricting alias size seems right to me. > > > > > > User configuring mdev for networking devices in a given kernel > > > > > > knows what > > > > > user is doing. > > > > > > So user can choose alias name size as it finds suitable. > > > > > > > > > > That's not what I'm proposing, please read again. Thanks, > > > > > > > > I understood your point. But mdev doesn't know how user is going > > > > to use > > > udev/systemd to name the netdev. > > > > So even if mdev chose to pick 12 characters, it could result in > > > > collision. > > > > Hence the proposal to provide the alias by the user, as user know > > > > the best > > > policy for its use case in the environment its using. > > > > So 12 character sha1 method will still work by user. > > > > > > Haven't you already provided examples where certain drivers or > > > subsystems have unique netdev prefixes? If mdev provides a unique > > > alias within the subsystem, couldn't we simply define a netdev > > > prefix for the mdev subsystem and avoid all other collisions? I'm > > > not in favor of the user providing both a uuid and an > > > alias/instance. Thanks, > > > > > For a given prefix, say ens2f0, can two UUID->sha1 first 9 characters have > collision? > > I think it would be a mistake to waste so many chars on a prefix, but 9 > characters of sha1 likely wouldn't have a collision before we have 10s of > thousands of devices. Thanks, > > Alex Jiri, Dave, Are you ok with it for devlink/netdev part? Mdev core will create an alias from a UUID. This will be supplied during devlink port attr set such as, devlink_port_attrs_mdev_set(struct devlink_port *port, const char *mdev_alias); This alias is used to generate representor netdev's phys_port_name. This alias from the mdev device's sysfs will be used by the udev/systemd to generate predicable netdev's name. Example: enm I took Ethernet mdev as an example. New prefix 'm' stands for mediated device. Remaining 12 characters are first 12 chars of the mdev alias.
RE: [PATCH v2 0/2] Simplify mtty driver and mdev core
> -Original Message- > From: Alex Williamson > Sent: Wednesday, August 21, 2019 10:27 AM > To: Parav Pandit > Cc: Jiri Pirko ; David S . Miller ; > Kirti Wankhede ; Cornelia Huck > ; k...@vger.kernel.org; linux-kernel@vger.kernel.org; > cjia ; net...@vger.kernel.org > Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core > > On Wed, 21 Aug 2019 04:40:15 + > Parav Pandit wrote: > > > > -Original Message- > > > From: Alex Williamson > > > Sent: Wednesday, August 21, 2019 9:51 AM > > > To: Parav Pandit > > > Cc: Jiri Pirko ; David S . Miller > > > ; Kirti Wankhede ; > > > Cornelia Huck ; k...@vger.kernel.org; > > > linux-kernel@vger.kernel.org; cjia ; > > > net...@vger.kernel.org > > > Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core > > > > > > On Wed, 21 Aug 2019 03:42:25 + > > > Parav Pandit wrote: > > > > > > > > -Original Message- > > > > > From: Alex Williamson > > > > > Sent: Tuesday, August 20, 2019 10:49 PM > > > > > To: Parav Pandit > > > > > Cc: Jiri Pirko ; David S . Miller > > > > > ; Kirti Wankhede ; > > > > > Cornelia Huck ; k...@vger.kernel.org; > > > > > linux-kernel@vger.kernel.org; cjia ; > > > > > net...@vger.kernel.org > > > > > Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core > > > > > > > > > > On Tue, 20 Aug 2019 08:58:02 + Parav Pandit > > > > > wrote: > > > > > > > > > > > + Dave. > > > > > > > > > > > > Hi Jiri, Dave, Alex, Kirti, Cornelia, > > > > > > > > > > > > Please provide your feedback on it, how shall we proceed? > > > > > > > > > > > > Short summary of requirements. > > > > > > For a given mdev (mediated device [1]), there is one > > > > > > representor netdevice and devlink port in switchdev mode > > > > > > (similar to SR-IOV VF), And there is one netdevice for the actual > > > > > > mdev > when mdev is probed. > > > > > > > > > > > > (a) representor netdev and devlink port should be able derive > > > > > > phys_port_name(). So that representor netdev name can be built > > > > > > deterministically across reboots. > > > > > > > > > > > > (b) for mdev's netdevice, mdev's device should have an attribute. > > > > > > This attribute can be used by udev rules/systemd or something > > > > > > else to rename netdev name deterministically. > > > > > > > > > > > > (c) IFNAMSIZ of 16 bytes is too small to fit whole UUID. > > > > > > A simple grep IFNAMSIZ in stack hints hundreds of users of > > > > > > IFNAMSIZ in drivers, uapi, netlink, boot config area and more. > > > > > > Changing IFNAMSIZ for a mdev bus doesn't really look > > > > > > reasonable option > > > to me. > > > > > > > > > > How many characters do we really have to work with? Your > > > > > examples below prepend various characters, ex. option-1 results > > > > > in ens2f0_m10 or enm10. Do the extra 8 or 3 characters in these count > against IFNAMSIZ? > > > > > > > > > Maximum 15. Last is null termination. > > > > Some udev rules setting by user prefix the PF netdev interface. I > > > > took such > > > example below where ens2f0 netdev named is prefixed. > > > > Some prefer not to prefix. > > > > > > > > > > Hence, I would like to discuss below options. > > > > > > > > > > > > Option-1: mdev index > > > > > > Introduce an optional mdev index/handle as u32 during mdev > > > > > > create time. User passes mdev index/handle as input. > > > > > > > > > > > > phys_port_name=mIndex=m%u > > > > > > mdev_index will be available in sysfs as mdev attribute for > > > > > > udev to name the mdev's netdev. > > > > > > > > > > > > example mdev create command: > > > > > > UUID=$(uuidgen) > > > > > > echo $UUID index=10 > > > > > > > /sys/class/net/ens2f0/mdev_supported_types/mlx5_core_mdev/cr > > > > > &g
RE: [PATCH v2 0/2] Simplify mtty driver and mdev core
> -Original Message- > From: Alex Williamson > Sent: Wednesday, August 21, 2019 9:51 AM > To: Parav Pandit > Cc: Jiri Pirko ; David S . Miller ; > Kirti Wankhede ; Cornelia Huck > ; k...@vger.kernel.org; linux-kernel@vger.kernel.org; > cjia ; net...@vger.kernel.org > Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core > > On Wed, 21 Aug 2019 03:42:25 + > Parav Pandit wrote: > > > > -Original Message- > > > From: Alex Williamson > > > Sent: Tuesday, August 20, 2019 10:49 PM > > > To: Parav Pandit > > > Cc: Jiri Pirko ; David S . Miller > > > ; Kirti Wankhede ; > > > Cornelia Huck ; k...@vger.kernel.org; > > > linux-kernel@vger.kernel.org; cjia ; > > > net...@vger.kernel.org > > > Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core > > > > > > On Tue, 20 Aug 2019 08:58:02 + > > > Parav Pandit wrote: > > > > > > > + Dave. > > > > > > > > Hi Jiri, Dave, Alex, Kirti, Cornelia, > > > > > > > > Please provide your feedback on it, how shall we proceed? > > > > > > > > Short summary of requirements. > > > > For a given mdev (mediated device [1]), there is one representor > > > > netdevice and devlink port in switchdev mode (similar to SR-IOV > > > > VF), And there is one netdevice for the actual mdev when mdev is probed. > > > > > > > > (a) representor netdev and devlink port should be able derive > > > > phys_port_name(). So that representor netdev name can be built > > > > deterministically across reboots. > > > > > > > > (b) for mdev's netdevice, mdev's device should have an attribute. > > > > This attribute can be used by udev rules/systemd or something else > > > > to rename netdev name deterministically. > > > > > > > > (c) IFNAMSIZ of 16 bytes is too small to fit whole UUID. > > > > A simple grep IFNAMSIZ in stack hints hundreds of users of > > > > IFNAMSIZ in drivers, uapi, netlink, boot config area and more. > > > > Changing IFNAMSIZ for a mdev bus doesn't really look reasonable option > to me. > > > > > > How many characters do we really have to work with? Your examples > > > below prepend various characters, ex. option-1 results in ens2f0_m10 > > > or enm10. Do the extra 8 or 3 characters in these count against IFNAMSIZ? > > > > > Maximum 15. Last is null termination. > > Some udev rules setting by user prefix the PF netdev interface. I took such > example below where ens2f0 netdev named is prefixed. > > Some prefer not to prefix. > > > > > > Hence, I would like to discuss below options. > > > > > > > > Option-1: mdev index > > > > Introduce an optional mdev index/handle as u32 during mdev create > > > > time. User passes mdev index/handle as input. > > > > > > > > phys_port_name=mIndex=m%u > > > > mdev_index will be available in sysfs as mdev attribute for udev > > > > to name the mdev's netdev. > > > > > > > > example mdev create command: > > > > UUID=$(uuidgen) > > > > echo $UUID index=10 > > > > > /sys/class/net/ens2f0/mdev_supported_types/mlx5_core_mdev/create > > > > > > Nit, IIRC previous discussions of additional parameters used comma > > > separators, ex. echo $UUID,index=10 >... > > > > > Yes, ok. > > > > > > > example netdevs: > > > > repnetdev=ens2f0_m10/*ens2f0 is parent PF's netdevice */ > > > > > > Is the parent really relevant in the name? > > No. I just picked one udev example who prefixed the parent netdev name. > > But there are users who do not prefix it. > > > > > Tools like mdevctl are meant to > > > provide persistence, creating the same mdev devices on the same > > > parent, but that's simply the easiest policy decision. We can also > > > imagine that multiple parent devices might support a specified mdev > > > type and policies factoring in proximity, load-balancing, power > > > consumption, etc might be weighed such that we really don't want to > > > promote userspace creating dependencies on the parent association. > > > > > > > mdev_netdev=enm10 > > > > > > > > Pros: > > > > 1. mdevctl and any other existing tools are unaffected. > > > > 2. netdev stack, ovs and other switching platforms are u
RE: [PATCH v2 0/2] Simplify mtty driver and mdev core
> -Original Message- > From: Cornelia Huck > Sent: Tuesday, August 20, 2019 11:25 PM > To: Alex Williamson > Cc: Parav Pandit ; Jiri Pirko ; > David S . Miller ; Kirti Wankhede > ; k...@vger.kernel.org; linux- > ker...@vger.kernel.org; cjia ; net...@vger.kernel.org > Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core > > On Tue, 20 Aug 2019 11:19:04 -0600 > Alex Williamson wrote: > > > What about an alias based on the uuid? For example, we use 160-bit > > sha1s daily with git (uuids are only 128-bit), but we generally don't > > reference git commits with the full 20 character string. Generally 12 > > characters is recommended to avoid ambiguity. Could mdev > > automatically create an abbreviated sha1 alias for the device? If so, > > how many characters should we use and what do we do on collision? The > > colliding device could add enough alias characters to disambiguate (we > > likely couldn't re-alias the existing device to disambiguate, but I'm > > not sure it matters, userspace has sysfs to associate aliases). Ex. > > > > UUID=$(uuidgen) > > ALIAS=$(echo $UUID | sha1sum | colrm 13) > > > > Since there seems to be some prefix overhead, as I ask about above in > > how many characters we actually have to work with in IFNAMESZ, maybe > > we start with 8 characters (matching your "index" namespace) and > > expand as necessary for disambiguation. If we can eliminate overhead > > in IFNAMESZ, let's start with 12. Thanks, > > > > Alex > > I really like that idea, and it seems the best option proposed yet, as we > don't > need to create a secondary identifier. User setting this alias at mdev creation time and exposed via sysfs as read only attribute works. Exposing that as const char *mdev_alias(struct mdev_device *dev) to vendor drivers..
RE: [PATCH v2 0/2] Simplify mtty driver and mdev core
> -Original Message- > From: Alex Williamson > Sent: Tuesday, August 20, 2019 10:49 PM > To: Parav Pandit > Cc: Jiri Pirko ; David S . Miller ; > Kirti Wankhede ; Cornelia Huck > ; k...@vger.kernel.org; linux-kernel@vger.kernel.org; > cjia ; net...@vger.kernel.org > Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core > > On Tue, 20 Aug 2019 08:58:02 + > Parav Pandit wrote: > > > + Dave. > > > > Hi Jiri, Dave, Alex, Kirti, Cornelia, > > > > Please provide your feedback on it, how shall we proceed? > > > > Short summary of requirements. > > For a given mdev (mediated device [1]), there is one representor > > netdevice and devlink port in switchdev mode (similar to SR-IOV VF), > > And there is one netdevice for the actual mdev when mdev is probed. > > > > (a) representor netdev and devlink port should be able derive > > phys_port_name(). So that representor netdev name can be built > > deterministically across reboots. > > > > (b) for mdev's netdevice, mdev's device should have an attribute. > > This attribute can be used by udev rules/systemd or something else to > > rename netdev name deterministically. > > > > (c) IFNAMSIZ of 16 bytes is too small to fit whole UUID. > > A simple grep IFNAMSIZ in stack hints hundreds of users of IFNAMSIZ in > > drivers, uapi, netlink, boot config area and more. Changing IFNAMSIZ > > for a mdev bus doesn't really look reasonable option to me. > > How many characters do we really have to work with? Your examples below > prepend various characters, ex. option-1 results in ens2f0_m10 or enm10. Do > the extra 8 or 3 characters in these count against IFNAMSIZ? > Maximum 15. Last is null termination. Some udev rules setting by user prefix the PF netdev interface. I took such example below where ens2f0 netdev named is prefixed. Some prefer not to prefix. > > Hence, I would like to discuss below options. > > > > Option-1: mdev index > > Introduce an optional mdev index/handle as u32 during mdev create > > time. User passes mdev index/handle as input. > > > > phys_port_name=mIndex=m%u > > mdev_index will be available in sysfs as mdev attribute for udev to > > name the mdev's netdev. > > > > example mdev create command: > > UUID=$(uuidgen) > > echo $UUID index=10 > > > /sys/class/net/ens2f0/mdev_supported_types/mlx5_core_mdev/create > > Nit, IIRC previous discussions of additional parameters used comma separators, > ex. echo $UUID,index=10 >... > Yes, ok. > > > example netdevs: > > repnetdev=ens2f0_m10/*ens2f0 is parent PF's netdevice */ > > Is the parent really relevant in the name? No. I just picked one udev example who prefixed the parent netdev name. But there are users who do not prefix it. > Tools like mdevctl are meant to > provide persistence, creating the same mdev devices on the same parent, but > that's simply the easiest policy decision. We can also imagine that multiple > parent devices might support a specified mdev type and policies factoring in > proximity, load-balancing, power consumption, etc might be weighed such that > we really don't want to promote userspace creating dependencies on the > parent association. > > > mdev_netdev=enm10 > > > > Pros: > > 1. mdevctl and any other existing tools are unaffected. > > 2. netdev stack, ovs and other switching platforms are unaffected. > > 3. achieves unique phys_port_name for representor netdev 4. achieves > > unique mdev eth netdev name for the mdev using udev/systemd extension. > > 5. Aligns well with mdev and netdev subsystem and similar to existing > > sriov bdf's. > > A user provided index seems strange to me. It's not really an index, just a > user > specified instance number. Presumably you have the user providing this > because if it really were an index, then the value depends on the creation > order > and persistence is lost. Now the user needs to both avoid uuid collision as > well > as "index" number collision. The uuid namespace is large enough to mostly > ignore this, but this is not. This seems like a burden. > I liked the term 'instance number', which is lot better way to say than index/handle. Yes, user needs to avoid both the collision. UUID collision should not occur in most cases, they way UUID are generated. So practically users needs to pick unique 'instance number', similar to how it picks unique netdev names. Burden to user comes from the requirement to get uniqueness. > > Option-2: shorter mdev name > > Extend mdev to have shorter mdev device name in addition to UUID. > > such a
RE: [PATCH v2 0/2] Simplify mtty driver and mdev core
> -Original Message- > From: Cornelia Huck > Sent: Tuesday, August 20, 2019 10:01 PM > > > > Option-1: mdev index > > > > Introduce an optional mdev index/handle as u32 during mdev create > time. > > > > User passes mdev index/handle as input. > > > > > > > > phys_port_name=mIndex=m%u > > > > mdev_index will be available in sysfs as mdev attribute for udev > > > > to name the > > > mdev's netdev. > > > > > > > > example mdev create command: > > > > UUID=$(uuidgen) > > > > echo $UUID index=10 > > > > > /sys/class/net/ens2f0/mdev_supported_types/mlx5_core_mdev/create > > > > example netdevs: > > > > repnetdev=ens2f0_m10/*ens2f0 is parent PF's netdevice */ > > > > mdev_netdev=enm10 > > > > > > > > Pros: > > > > 1. mdevctl and any other existing tools are unaffected. > > > > 2. netdev stack, ovs and other switching platforms are unaffected. > > > > 3. achieves unique phys_port_name for representor netdev 4. > > > > achieves unique mdev eth netdev name for the mdev using udev/systemd > extension. > > > > 5. Aligns well with mdev and netdev subsystem and similar to > > > > existing sriov > > > bdf's. > > > > > > > > Option-2: shorter mdev name > > > > Extend mdev to have shorter mdev device name in addition to UUID. > > > > such as 'foo', 'bar'. > > > > Mdev will continue to have UUID. > > I fail to understand how 'uses uuid' and 'allow shorter device name' > are supposed to play together? > Each mdev will have uuid as today. Instead of naming device based on UUID, name it based on explicit name given by the user. Again, I want to repeat, this name parameter is optional. > > > > phys_port_name=mdev_name > > > > > > > > Pros: > > > > 1. All same as option-1, except mdevctl needs upgrade for newer usage. > > > > It is common practice to upgrade iproute2 package along with the kernel. > > > > Similar practice to be done with mdevctl. > > > > 2. Newer users of mdevctl who wants to work with non_UUID names, > > > > will use > > > newer mdevctl/tools. > > > > Cons: > > > > 1. Dual naming scheme of mdev might affect some of the existing tools. > > > > It's unclear how/if it actually affects. > > > > mdevctl [2] is very recently developed and can be enhanced for > > > > dual naming > > > scheme. > > The main problem is not tools we know about (i.e. mdevctl), but those we don't > know about. > Well, if it not part of the distros, there is very little can do about it by kernel. I tried mdevctl with mdev named using non UUID and it were able to list them. > IOW, this (and the IFNAMESIZ change, which seems even worse) are the > options I would not want at all. > Ok. > > > > > > > > Option-3: mdev uuid alias > > > > Instead of shorter mdev name or mdev index, have alpha-numeric > > > > name > > > alias. > > > > Alias is an optional mdev sysfs attribute such as 'foo', 'bar'. > > > > example mdev create command: > > > > UUID=$(uuidgen) > > > > echo $UUID alias=foo > > > > > /sys/class/net/ens2f0/mdev_supported_types/mlx5_core_mdev/create > > > > example netdevs: > > > > examle netdevs: > > > > repnetdev = ens2f0_mfoo > > > > mdev_netdev=enmfoo > > > > > > > > Pros: > > > > 1. All same as option-1. > > > > 2. Doesn't affect existing mdev naming scheme. > > > > Cons: > > > > 1. Index scheme of option-1 is better which can number large > > > > number of > > > mdevs with fewer characters, simplifying the management tool. > > > > > > I believe that Alex pointed out another "Cons" to all three options, > > > which is that it forces user-space to resolve potential race > > > conditions when creating an index or short name or alias. > > > > > This race condition exists for at least two subsystems that I know of, i.e. > netdev and rdma. > > If a device with a given name exists, subsystem returns error. > > When user space gets error code EEXIST, and it can picks up different > identifier(s). > > If you decouple device creation and setting the alias/index, you make the > issue > visible and thus much more manageable. > I thought about it. It has two issues. 1. user should be able to set this only once. Repeatedly setting it requires changing/notifying it. 2. setting alias translating in creating devlink port doesn't sound correct. Because if user attempts to reset to different value, it required unregistration, reregistration. All of such race conditions handling it not worth it. So setting the index, I liked Alex's term more 'instance number', at instance creation time is lot more simple. > > > > > Also, what happens if `index=10` is not provided on the command-line? > > > Does that make the device unusable for your purpose? > > Yes, it is unusable to an extent. > > Currently we have DEVLINK_PORT_FLAVOUR_PCI_VF in > > include/uapi/linux/devlink.h Similar to it, we need to have > DEVLINK_PORT_FLAVOUR_MDEV for mdev eswitch ports. > > This port flavour needs to generate phys_port_name(). This should be user > parameter driven. > > Because representor netdevice name is generated based on this parameter. >
RE: [PATCH v2 0/2] Simplify mtty driver and mdev core
> -Original Message- > From: Christophe de Dinechin > Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core > > > Parav Pandit writes: > > > + Dave. > > > > Hi Jiri, Dave, Alex, Kirti, Cornelia, > > > > Please provide your feedback on it, how shall we proceed? > > > > Hence, I would like to discuss below options. > > > > Option-1: mdev index > > Introduce an optional mdev index/handle as u32 during mdev create time. > > User passes mdev index/handle as input. > > > > phys_port_name=mIndex=m%u > > mdev_index will be available in sysfs as mdev attribute for udev to name the > mdev's netdev. > > > > example mdev create command: > > UUID=$(uuidgen) > > echo $UUID index=10 > > > /sys/class/net/ens2f0/mdev_supported_types/mlx5_core_mdev/create > > example netdevs: > > repnetdev=ens2f0_m10/*ens2f0 is parent PF's netdevice */ > > mdev_netdev=enm10 > > > > Pros: > > 1. mdevctl and any other existing tools are unaffected. > > 2. netdev stack, ovs and other switching platforms are unaffected. > > 3. achieves unique phys_port_name for representor netdev 4. achieves > > unique mdev eth netdev name for the mdev using udev/systemd extension. > > 5. Aligns well with mdev and netdev subsystem and similar to existing sriov > bdf's. > > > > Option-2: shorter mdev name > > Extend mdev to have shorter mdev device name in addition to UUID. > > such as 'foo', 'bar'. > > Mdev will continue to have UUID. > > phys_port_name=mdev_name > > > > Pros: > > 1. All same as option-1, except mdevctl needs upgrade for newer usage. > > It is common practice to upgrade iproute2 package along with the kernel. > > Similar practice to be done with mdevctl. > > 2. Newer users of mdevctl who wants to work with non_UUID names, will use > newer mdevctl/tools. > > Cons: > > 1. Dual naming scheme of mdev might affect some of the existing tools. > > It's unclear how/if it actually affects. > > mdevctl [2] is very recently developed and can be enhanced for dual naming > scheme. > > > > Option-3: mdev uuid alias > > Instead of shorter mdev name or mdev index, have alpha-numeric name > alias. > > Alias is an optional mdev sysfs attribute such as 'foo', 'bar'. > > example mdev create command: > > UUID=$(uuidgen) > > echo $UUID alias=foo > > > /sys/class/net/ens2f0/mdev_supported_types/mlx5_core_mdev/create > > example netdevs: > > examle netdevs: > > repnetdev = ens2f0_mfoo > > mdev_netdev=enmfoo > > > > Pros: > > 1. All same as option-1. > > 2. Doesn't affect existing mdev naming scheme. > > Cons: > > 1. Index scheme of option-1 is better which can number large number of > mdevs with fewer characters, simplifying the management tool. > > I believe that Alex pointed out another "Cons" to all three options, which is > that > it forces user-space to resolve potential race conditions when creating an > index > or short name or alias. > This race condition exists for at least two subsystems that I know of, i.e. netdev and rdma. If a device with a given name exists, subsystem returns error. When user space gets error code EEXIST, and it can picks up different identifier(s). > Also, what happens if `index=10` is not provided on the command-line? > Does that make the device unusable for your purpose? Yes, it is unusable to an extent. Currently we have DEVLINK_PORT_FLAVOUR_PCI_VF in include/uapi/linux/devlink.h Similar to it, we need to have DEVLINK_PORT_FLAVOUR_MDEV for mdev eswitch ports. This port flavour needs to generate phys_port_name(). This should be user parameter driven. Because representor netdevice name is generated based on this parameter.
RE: [PATCH v2 0/2] Simplify mtty driver and mdev core
+ Dave. Hi Jiri, Dave, Alex, Kirti, Cornelia, Please provide your feedback on it, how shall we proceed? Short summary of requirements. For a given mdev (mediated device [1]), there is one representor netdevice and devlink port in switchdev mode (similar to SR-IOV VF), And there is one netdevice for the actual mdev when mdev is probed. (a) representor netdev and devlink port should be able derive phys_port_name(). So that representor netdev name can be built deterministically across reboots. (b) for mdev's netdevice, mdev's device should have an attribute. This attribute can be used by udev rules/systemd or something else to rename netdev name deterministically. (c) IFNAMSIZ of 16 bytes is too small to fit whole UUID. A simple grep IFNAMSIZ in stack hints hundreds of users of IFNAMSIZ in drivers, uapi, netlink, boot config area and more. Changing IFNAMSIZ for a mdev bus doesn't really look reasonable option to me. Hence, I would like to discuss below options. Option-1: mdev index Introduce an optional mdev index/handle as u32 during mdev create time. User passes mdev index/handle as input. phys_port_name=mIndex=m%u mdev_index will be available in sysfs as mdev attribute for udev to name the mdev's netdev. example mdev create command: UUID=$(uuidgen) echo $UUID index=10 > /sys/class/net/ens2f0/mdev_supported_types/mlx5_core_mdev/create example netdevs: repnetdev=ens2f0_m10/*ens2f0 is parent PF's netdevice */ mdev_netdev=enm10 Pros: 1. mdevctl and any other existing tools are unaffected. 2. netdev stack, ovs and other switching platforms are unaffected. 3. achieves unique phys_port_name for representor netdev 4. achieves unique mdev eth netdev name for the mdev using udev/systemd extension. 5. Aligns well with mdev and netdev subsystem and similar to existing sriov bdf's. Option-2: shorter mdev name Extend mdev to have shorter mdev device name in addition to UUID. such as 'foo', 'bar'. Mdev will continue to have UUID. phys_port_name=mdev_name Pros: 1. All same as option-1, except mdevctl needs upgrade for newer usage. It is common practice to upgrade iproute2 package along with the kernel. Similar practice to be done with mdevctl. 2. Newer users of mdevctl who wants to work with non_UUID names, will use newer mdevctl/tools. Cons: 1. Dual naming scheme of mdev might affect some of the existing tools. It's unclear how/if it actually affects. mdevctl [2] is very recently developed and can be enhanced for dual naming scheme. Option-3: mdev uuid alias Instead of shorter mdev name or mdev index, have alpha-numeric name alias. Alias is an optional mdev sysfs attribute such as 'foo', 'bar'. example mdev create command: UUID=$(uuidgen) echo $UUID alias=foo > /sys/class/net/ens2f0/mdev_supported_types/mlx5_core_mdev/create example netdevs: examle netdevs: repnetdev = ens2f0_mfoo mdev_netdev=enmfoo Pros: 1. All same as option-1. 2. Doesn't affect existing mdev naming scheme. Cons: 1. Index scheme of option-1 is better which can number large number of mdevs with fewer characters, simplifying the management tool. Option-4: extend IFNAMESZ to be 64 bytes Extended IFNAMESZ from 16 to 64 bytes phys_port_name=mdev_UUID_string mdev_netdev_name=enmUUID Pros: 1. Doesn't require mdev extension Cons: 1. netdev stack, driver, uapi, user space, boot config wide changes 2. Possible user space extensions who assumed name size being 16 characters 3. Single device type demands namesize change for all netdev types [1] https://www.kernel.org/doc/Documentation/vfio-mediated-device.txt [2] https://github.com/mdevctl/mdevctl Regards, Parav Pandit > -Original Message- > From: linux-kernel-ow...@vger.kernel.org ow...@vger.kernel.org> On Behalf Of Parav Pandit > Sent: Wednesday, August 14, 2019 9:51 PM > To: Alex Williamson > Cc: Cornelia Huck ; Kirti Wankhede > ; k...@vger.kernel.org; linux- > ker...@vger.kernel.org; c...@nvidia.com; Jiri Pirko ; > net...@vger.kernel.org > Subject: RE: [PATCH v2 0/2] Simplify mtty driver and mdev core > > > > > -Original Message- > > From: Alex Williamson > > Sent: Wednesday, August 14, 2019 8:28 PM > > To: Parav Pandit > > Cc: Cornelia Huck ; Kirti Wankhede > > ; k...@vger.kernel.org; linux- > > ker...@vger.kernel.org; c...@nvidia.com; Jiri Pirko > > ; net...@vger.kernel.org > > Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core > > > > On Wed, 14 Aug 2019 13:45:49 + > > Parav Pandit wrote: > > > > > > -Original Message- > > > > From: Cornelia Huck > > > > Sent: Wednesday, August 14, 2019 6:39 PM > > > > To: Parav Pandit > > > > Cc: Alex Williamson ; Kirti Wankhede > > > > ; k...@vger.kernel.org; linux- > > > > ker...@vger.kernel.org; c...@nvidia.com; Jiri Pirko > > > > ; net...@vger.kernel.org > &
RE: [PATCH v2 0/2] Simplify mtty driver and mdev core
> -Original Message- > From: Alex Williamson > Sent: Wednesday, August 14, 2019 8:28 PM > To: Parav Pandit > Cc: Cornelia Huck ; Kirti Wankhede > ; k...@vger.kernel.org; linux- > ker...@vger.kernel.org; c...@nvidia.com; Jiri Pirko ; > net...@vger.kernel.org > Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core > > On Wed, 14 Aug 2019 13:45:49 + > Parav Pandit wrote: > > > > -Original Message- > > > From: Cornelia Huck > > > Sent: Wednesday, August 14, 2019 6:39 PM > > > To: Parav Pandit > > > Cc: Alex Williamson ; Kirti Wankhede > > > ; k...@vger.kernel.org; linux- > > > ker...@vger.kernel.org; c...@nvidia.com; Jiri Pirko > > > ; net...@vger.kernel.org > > > Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core > > > > > > On Wed, 14 Aug 2019 12:27:01 + > > > Parav Pandit wrote: > > > > > > > + Jiri, + netdev > > > > To get perspective on the ndo->phys_port_name for the representor > > > > netdev > > > of mdev. > > > > > > > > Hi Cornelia, > > > > > > > > > -Original Message- > > > > > From: Cornelia Huck > > > > > Sent: Wednesday, August 14, 2019 1:32 PM > > > > > To: Parav Pandit > > > > > Cc: Alex Williamson ; Kirti Wankhede > > > > > ; k...@vger.kernel.org; linux- > > > > > ker...@vger.kernel.org; c...@nvidia.com > > > > > Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core > > > > > > > > > > On Wed, 14 Aug 2019 05:54:36 + Parav Pandit > > > > > wrote: > > > > > > > > > > > > > I get that part. I prefer to remove the UUID itself from > > > > > > > > the structure and therefore removing this API makes lot more > sense? > > > > > > > > > > > > > > Mdev and support tools around mdev are based on UUIDs > > > > > > > because it's > > > > > defined > > > > > > > in the documentation. > > > > > > When we introduce newer device naming scheme, it will update > > > > > > the > > > > > documentation also. > > > > > > May be that is the time to move to .rst format too. > > > > > > > > > > You are aware that there are existing tools that expect a uuid > > > > > naming scheme, right? > > > > > > > > > Yes, Alex mentioned too. > > > > The good tool that I am aware of is [1], which is 4 months old. > > > > Not sure if it is > > > part of any distros yet. > > > > > > > > README also says, that it is in 'early in development. So we have > > > > scope to > > > improve it for non UUID names, but lets discuss that more below. > > > > > > The up-to-date reference for mdevctl is > > > https://github.com/mdevctl/mdevctl. There is currently an effort to > > > get this packaged in Fedora. > > > > > Awesome. > > > > > > > > > > > > > > > > > > > I don't think it's as simple as saying "voila, UUID > > > > > > > dependencies are removed, users are free to use arbitrary > > > > > > > strings". We'd need to create some kind of naming policy, > > > > > > > what characters are allows so that we can potentially expand > > > > > > > the creation parameters as has been proposed a couple times, > > > > > > > how do we deal with collisions and races, and why should we > > > > > > > make such a change when a UUID is a perfectly reasonable > > > > > > > devices name. Thanks, > > > > > > > > > > > > > Sure, we should define a policy on device naming to be more relaxed. > > > > > > We have enough examples in-kernel. > > > > > > Few that I am aware of are netdev (vxlan, macvlan, ipvlan, lot > > > > > > more), rdma > > > > > etc which has arbitrary device names and ID based device names. > > > > > > > > > > > > Collisions and race is already taken care today in the mdev core. > > > > > > Same > > > > > unique device names continue. > > > > > > > > > > I'm still completely missing a rationale _why_ uuids are > >
RE: [PATCH v2 0/2] Simplify mtty driver and mdev core
> -Original Message- > From: Cornelia Huck > Sent: Wednesday, August 14, 2019 6:39 PM > To: Parav Pandit > Cc: Alex Williamson ; Kirti Wankhede > ; k...@vger.kernel.org; linux- > ker...@vger.kernel.org; c...@nvidia.com; Jiri Pirko ; > net...@vger.kernel.org > Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core > > On Wed, 14 Aug 2019 12:27:01 + > Parav Pandit wrote: > > > + Jiri, + netdev > > To get perspective on the ndo->phys_port_name for the representor netdev > of mdev. > > > > Hi Cornelia, > > > > > -Original Message- > > > From: Cornelia Huck > > > Sent: Wednesday, August 14, 2019 1:32 PM > > > To: Parav Pandit > > > Cc: Alex Williamson ; Kirti Wankhede > > > ; k...@vger.kernel.org; linux- > > > ker...@vger.kernel.org; c...@nvidia.com > > > Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core > > > > > > On Wed, 14 Aug 2019 05:54:36 + > > > Parav Pandit wrote: > > > > > > > > > I get that part. I prefer to remove the UUID itself from the > > > > > > structure and therefore removing this API makes lot more sense? > > > > > > > > > > Mdev and support tools around mdev are based on UUIDs because > > > > > it's > > > defined > > > > > in the documentation. > > > > When we introduce newer device naming scheme, it will update the > > > documentation also. > > > > May be that is the time to move to .rst format too. > > > > > > You are aware that there are existing tools that expect a uuid > > > naming scheme, right? > > > > > Yes, Alex mentioned too. > > The good tool that I am aware of is [1], which is 4 months old. Not sure if > > it is > part of any distros yet. > > > > README also says, that it is in 'early in development. So we have scope to > improve it for non UUID names, but lets discuss that more below. > > The up-to-date reference for mdevctl is > https://github.com/mdevctl/mdevctl. There is currently an effort to get this > packaged in Fedora. > Awesome. > > > > > > > > > > > I don't think it's as simple as saying "voila, UUID dependencies > > > > > are removed, users are free to use arbitrary strings". We'd > > > > > need to create some kind of naming policy, what characters are > > > > > allows so that we can potentially expand the creation parameters > > > > > as has been proposed a couple times, how do we deal with > > > > > collisions and races, and why should we make such a change when > > > > > a UUID is a perfectly reasonable devices name. Thanks, > > > > > > > > > Sure, we should define a policy on device naming to be more relaxed. > > > > We have enough examples in-kernel. > > > > Few that I am aware of are netdev (vxlan, macvlan, ipvlan, lot > > > > more), rdma > > > etc which has arbitrary device names and ID based device names. > > > > > > > > Collisions and race is already taken care today in the mdev core. > > > > Same > > > unique device names continue. > > > > > > I'm still completely missing a rationale _why_ uuids are supposedly > > > bad/restricting/etc. > > There is nothing bad about uuid based naming. > > Its just too long name to derive phys_port_name of a netdev. > > In details below. > > > > For a given mdev of networking type, we would like to have > > (a) representor netdevice [2] > > (b) associated devlink port [3] > > > > Currently these representor netdevice exist only for the PCIe SR-IOV VFs. > > It is further getting extended for mdev without SR-IOV. > > > > Each of the devlink port is attached to representor netdevice [4]. > > > > This netdevice phys_port_name should be a unique derived from some > property of mdev. > > Udev/systemd uses phys_port_name to derive unique representor netdev > name. > > This netdev name is further use by orchestration and switching software in > user space. > > One such distro supported switching software is ovs [4], which relies on the > persistent device name of the representor netdevice. > > Ok, let me rephrase this to check that I understand this correctly. I'm not > sure > about some of the terms you use here (even after looking at the linked > doc/code), but that's probably still ok. > > We want to derive an unique (and probably persistent?) netdev name so t
RE: [PATCH v2 0/2] Simplify mtty driver and mdev core
+ Jiri, + netdev To get perspective on the ndo->phys_port_name for the representor netdev of mdev. Hi Cornelia, > -Original Message- > From: Cornelia Huck > Sent: Wednesday, August 14, 2019 1:32 PM > To: Parav Pandit > Cc: Alex Williamson ; Kirti Wankhede > ; k...@vger.kernel.org; linux- > ker...@vger.kernel.org; c...@nvidia.com > Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core > > On Wed, 14 Aug 2019 05:54:36 + > Parav Pandit wrote: > > > > > I get that part. I prefer to remove the UUID itself from the > > > > structure and therefore removing this API makes lot more sense? > > > > > > Mdev and support tools around mdev are based on UUIDs because it's > defined > > > in the documentation. > > When we introduce newer device naming scheme, it will update the > documentation also. > > May be that is the time to move to .rst format too. > > You are aware that there are existing tools that expect a uuid naming scheme, > right? > Yes, Alex mentioned too. The good tool that I am aware of is [1], which is 4 months old. Not sure if it is part of any distros yet. README also says, that it is in 'early in development. So we have scope to improve it for non UUID names, but lets discuss that more below. > > > > > I don't think it's as simple as saying "voila, UUID dependencies are > > > removed, users are free to use arbitrary strings". We'd need to > > > create some kind of naming policy, what characters are allows so > > > that we can potentially expand the creation parameters as has been > > > proposed a couple times, how do we deal with collisions and races, > > > and why should we make such a change when a UUID is a perfectly > > > reasonable devices name. Thanks, > > > > > Sure, we should define a policy on device naming to be more relaxed. > > We have enough examples in-kernel. > > Few that I am aware of are netdev (vxlan, macvlan, ipvlan, lot more), rdma > etc which has arbitrary device names and ID based device names. > > > > Collisions and race is already taken care today in the mdev core. Same > unique device names continue. > > I'm still completely missing a rationale _why_ uuids are supposedly > bad/restricting/etc. There is nothing bad about uuid based naming. Its just too long name to derive phys_port_name of a netdev. In details below. For a given mdev of networking type, we would like to have (a) representor netdevice [2] (b) associated devlink port [3] Currently these representor netdevice exist only for the PCIe SR-IOV VFs. It is further getting extended for mdev without SR-IOV. Each of the devlink port is attached to representor netdevice [4]. This netdevice phys_port_name should be a unique derived from some property of mdev. Udev/systemd uses phys_port_name to derive unique representor netdev name. This netdev name is further use by orchestration and switching software in user space. One such distro supported switching software is ovs [4], which relies on the persistent device name of the representor netdevice. phys_port_name has limitation to be only 15 characters long. UUID doesn't fit in phys_port_name. Longer UUID names are creating snow ball effect, not just in networking stack but many user space tools too. (as opposed to recently introduced mdevctl, are they more mdev tools which has dependency on UUID name?) Instead of mdev subsystem creating such effect, one option we are considering is to have shorter mdev names. (Similar to netdev, rdma, nvme devices). Such as mdev1, mdev2000 etc. Second option I was considering is to have an optional alias for UUID based mdev. This name alias is given at time of mdev creation. Devlink port's phys_port_name is derived out of this shorter mdev name alias. This way, mdev remains to be UUID based with optional extension. However, I prefer first option to relax mdev naming scheme. > We want to uniquely identify a device, across different > types of vendor drivers. An uuid is a unique identifier and even a > well-defined > one. Tools (e.g. mdevctl) are relying on it for mdev devices today. > > What is the problem you're trying to solve? Unique device naming is still achieved without UUID scheme by various subsystems in kernel using alpha-numeric string. Having such string based continue to provide unique names. I hope I described the problem and two solutions above. [1] https://github.com/awilliam/mdevctl [2] https://elixir.bootlin.com/linux/v5.3-rc4/source/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c [3] http://man7.org/linux/man-pages/man8/devlink-port.8.html [4] https://elixir.bootlin.com/linux/v5.3-rc4/source/net/core/devlink.c#L6921 [5] https://www.openvswitch.org/
RE: [PATCH v2 0/2] Simplify mtty driver and mdev core
> -Original Message- > From: Alex Williamson > Sent: Tuesday, August 13, 2019 10:42 PM > To: Parav Pandit > Cc: Kirti Wankhede ; k...@vger.kernel.org; linux- > ker...@vger.kernel.org; coh...@redhat.com; c...@nvidia.com > Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core > > On Tue, 13 Aug 2019 16:28:53 + > Parav Pandit wrote: > > > > -Original Message- > > > From: Alex Williamson > > > Sent: Tuesday, August 13, 2019 8:23 PM > > > To: Parav Pandit > > > Cc: Kirti Wankhede ; k...@vger.kernel.org; > > > linux- ker...@vger.kernel.org; coh...@redhat.com; c...@nvidia.com > > > Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core > > > > > > On Tue, 13 Aug 2019 14:40:02 + > > > Parav Pandit wrote: > > > > > > > > -Original Message- > > > > > From: Kirti Wankhede > > > > > Sent: Monday, August 12, 2019 5:06 PM > > > > > To: Alex Williamson ; Parav Pandit > > > > > > > > > > Cc: k...@vger.kernel.org; linux-kernel@vger.kernel.org; > > > > > coh...@redhat.com; c...@nvidia.com > > > > > Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core > > > > > > > > > > > > > > > > > > > > On 8/9/2019 4:32 AM, Alex Williamson wrote: > > > > > > On Thu, 8 Aug 2019 09:12:53 -0500 Parav Pandit > > > > > > wrote: > > > > > > > > > > > >> Currently mtty sample driver uses mdev state and UUID in > > > > > >> convoluated way to generate an interrupt. > > > > > >> It uses several translations from mdev_state to mdev_device > > > > > >> to mdev > > > uuid. > > > > > >> After which it does linear search of long uuid comparision to > > > > > >> find out mdev_state in mtty_trigger_interrupt(). > > > > > >> mdev_state is already available while generating interrupt > > > > > >> from which all such translations are done to reach back to > mdev_state. > > > > > >> > > > > > >> This translations are done during interrupt generation path. > > > > > >> This is unnecessary and reduandant. > > > > > > > > > > > > Is the interrupt handling efficiency of this particular sample > > > > > > driver really relevant, or is its purpose more to illustrate > > > > > > the API and provide a proof of concept? If we go to the > > > > > > trouble to optimize the sample driver and remove this > > > > > > interface from the API, what > > > do we lose? > > > > > > > > > > > > This interface was added via commit: > > > > > > > > > > > > 99e3123e3d72 vfio-mdev: Make mdev_device private and abstract > > > > > > interfaces > > > > > > > > > > > > Where the goal was to create a more formal interface and > > > > > > abstract driver access to the struct mdev_device. In part > > > > > > this served to make out-of-tree mdev vendor drivers more > > > > > > supportable; the object is considered opaque and access is > > > > > > provided via an API rather than through direct structure fields. > > > > > > > > > > > > I believe that the NVIDIA GRID mdev driver does make use of > > > > > > this interface and it's likely included in the sample driver > > > > > > specifically so that there is an in-kernel user for it (ie. > > > > > > specifically to avoid it being removed so casually). An > > > > > > interesting feature of the NVIDIA mdev driver is that I > > > > > > believe it has > > > portions that run in userspace. > > > > > > As we know, mdevs are named with a UUID, so I can imagine > > > > > > there are some efficiencies to be gained in having direct > > > > > > access to the UUID for a device when interacting with > > > > > > userspace, rather than repeatedly parsing it from a device name. > > > > > > > > > > That's right. > > > > > > > > > > > Is that really something we want to make more difficult in > > > > > > order to optimize a sample driver? Knowing that an mdev > > > > >
