KVM Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/2] Simplify mtty driver and mdev core
@ 2019-08-02  6:59 Parav Pandit
  2019-08-02  6:59 ` [PATCH 1/2] vfio-mdev/mtty: Simplify interrupt generation Parav Pandit
                   ` (3 more replies)
  0 siblings, 4 replies; 38+ messages in thread
From: Parav Pandit @ 2019-08-02  6:59 UTC (permalink / raw)
  To: kvm, wankhede, linux-kernel; +Cc: parav, alex.williamson, cohuck, cjia

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.

Hence,
Patch-1 simplifies mtty sample driver to directly use mdev_state.

Patch-2, Since no production driver uses mdev_uuid(), and mdev's name
is already available using core kernel dev_name(), simplifies and removes
redandant mdev_uuid() exported symbol.

Parav Pandit (2):
  vfio-mdev/mtty: Simplify interrupt generation
  vfio/mdev: Removed unused and redundant API for mdev name

 drivers/vfio/mdev/mdev_core.c |  6 ------
 include/linux/mdev.h          |  1 -
 samples/vfio-mdev/mtty.c      | 39 +++++++----------------------------
 3 files changed, 8 insertions(+), 38 deletions(-)

-- 
2.21.0.777.g83232e3864


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [PATCH 1/2] vfio-mdev/mtty: Simplify interrupt generation
  2019-08-02  6:59 [PATCH 0/2] Simplify mtty driver and mdev core Parav Pandit
@ 2019-08-02  6:59 ` Parav Pandit
  2019-08-06  8:15   ` Cornelia Huck
  2019-08-02  6:59 ` [PATCH 2/2] vfio/mdev: Removed unused and redundant API for mdev name Parav Pandit
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 38+ messages in thread
From: Parav Pandit @ 2019-08-02  6:59 UTC (permalink / raw)
  To: kvm, wankhede, linux-kernel; +Cc: parav, alex.williamson, cohuck, cjia

While generating interrupt, mdev_state is already available for which
interrupt is generated.
Instead of doing indirect way from state->device->uuid-> to searching
state linearly in linked list on every interrupt generation,
directly use the available state.

Hence, simplify the code to use mdev_state and remove unused helper
function with that.

Signed-off-by: Parav Pandit <parav@mellanox.com>
---
 samples/vfio-mdev/mtty.c | 39 ++++++++-------------------------------
 1 file changed, 8 insertions(+), 31 deletions(-)

diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c
index 92e770a06ea2..ce84a300a4da 100644
--- a/samples/vfio-mdev/mtty.c
+++ b/samples/vfio-mdev/mtty.c
@@ -152,20 +152,9 @@ static const struct file_operations vd_fops = {
 
 /* function prototypes */
 
-static int mtty_trigger_interrupt(const guid_t *uuid);
+static int mtty_trigger_interrupt(struct mdev_state *mdev_state);
 
 /* Helper functions */
-static struct mdev_state *find_mdev_state_by_uuid(const guid_t *uuid)
-{
-	struct mdev_state *mds;
-
-	list_for_each_entry(mds, &mdev_devices_list, next) {
-		if (guid_equal(mdev_uuid(mds->mdev), uuid))
-			return mds;
-	}
-
-	return NULL;
-}
 
 static void dump_buffer(u8 *buf, uint32_t count)
 {
@@ -337,8 +326,7 @@ static void handle_bar_write(unsigned int index, struct mdev_state *mdev_state,
 				pr_err("Serial port %d: Fifo level trigger\n",
 					index);
 #endif
-				mtty_trigger_interrupt(
-						mdev_uuid(mdev_state->mdev));
+				mtty_trigger_interrupt(mdev_state);
 			}
 		} else {
 #if defined(DEBUG_INTR)
@@ -352,8 +340,7 @@ static void handle_bar_write(unsigned int index, struct mdev_state *mdev_state,
 			 */
 			if (mdev_state->s[index].uart_reg[UART_IER] &
 								UART_IER_RLSI)
-				mtty_trigger_interrupt(
-						mdev_uuid(mdev_state->mdev));
+				mtty_trigger_interrupt(mdev_state);
 		}
 		mutex_unlock(&mdev_state->rxtx_lock);
 		break;
@@ -372,8 +359,7 @@ static void handle_bar_write(unsigned int index, struct mdev_state *mdev_state,
 				pr_err("Serial port %d: IER_THRI write\n",
 					index);
 #endif
-				mtty_trigger_interrupt(
-						mdev_uuid(mdev_state->mdev));
+				mtty_trigger_interrupt(mdev_state);
 			}
 
 			mutex_unlock(&mdev_state->rxtx_lock);
@@ -444,7 +430,7 @@ static void handle_bar_write(unsigned int index, struct mdev_state *mdev_state,
 #if defined(DEBUG_INTR)
 			pr_err("Serial port %d: MCR_OUT2 write\n", index);
 #endif
-			mtty_trigger_interrupt(mdev_uuid(mdev_state->mdev));
+			mtty_trigger_interrupt(mdev_state);
 		}
 
 		if ((mdev_state->s[index].uart_reg[UART_IER] & UART_IER_MSI) &&
@@ -452,7 +438,7 @@ static void handle_bar_write(unsigned int index, struct mdev_state *mdev_state,
 #if defined(DEBUG_INTR)
 			pr_err("Serial port %d: MCR RTS/DTR write\n", index);
 #endif
-			mtty_trigger_interrupt(mdev_uuid(mdev_state->mdev));
+			mtty_trigger_interrupt(mdev_state);
 		}
 		break;
 
@@ -503,8 +489,7 @@ static void handle_bar_read(unsigned int index, struct mdev_state *mdev_state,
 #endif
 			if (mdev_state->s[index].uart_reg[UART_IER] &
 							 UART_IER_THRI)
-				mtty_trigger_interrupt(
-					mdev_uuid(mdev_state->mdev));
+				mtty_trigger_interrupt(mdev_state);
 		}
 		mutex_unlock(&mdev_state->rxtx_lock);
 
@@ -1028,17 +1013,9 @@ static int mtty_set_irqs(struct mdev_device *mdev, uint32_t flags,
 	return ret;
 }
 
-static int mtty_trigger_interrupt(const guid_t *uuid)
+static int mtty_trigger_interrupt(struct mdev_state *mdev_state)
 {
 	int ret = -1;
-	struct mdev_state *mdev_state;
-
-	mdev_state = find_mdev_state_by_uuid(uuid);
-
-	if (!mdev_state) {
-		pr_info("%s: mdev not found\n", __func__);
-		return -EINVAL;
-	}
 
 	if ((mdev_state->irq_index == VFIO_PCI_MSI_IRQ_INDEX) &&
 	    (!mdev_state->msi_evtfd))
-- 
2.21.0.777.g83232e3864


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [PATCH 2/2] vfio/mdev: Removed unused and redundant API for mdev name
  2019-08-02  6:59 [PATCH 0/2] Simplify mtty driver and mdev core Parav Pandit
  2019-08-02  6:59 ` [PATCH 1/2] vfio-mdev/mtty: Simplify interrupt generation Parav Pandit
@ 2019-08-02  6:59 ` Parav Pandit
  2019-08-06  8:29   ` Cornelia Huck
  2019-08-06 14:18 ` [PATCH v1 0/2] Simplify mtty driver and mdev core Parav Pandit
  2019-08-08 14:12 ` [PATCH v2 0/2] Simplify mtty driver and mdev core Parav Pandit
  3 siblings, 1 reply; 38+ messages in thread
From: Parav Pandit @ 2019-08-02  6:59 UTC (permalink / raw)
  To: kvm, wankhede, linux-kernel; +Cc: parav, alex.williamson, cohuck, cjia

There is no single production driver who is interested in mdev device
name.
Additionally mdev device name is already available using core kernel
API dev_name().

Hence removed unused exported symbol.

Signed-off-by: Parav Pandit <parav@mellanox.com>
---
 drivers/vfio/mdev/mdev_core.c | 6 ------
 include/linux/mdev.h          | 1 -
 2 files changed, 7 deletions(-)

diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
index b558d4cfd082..c2b809cbe59f 100644
--- a/drivers/vfio/mdev/mdev_core.c
+++ b/drivers/vfio/mdev/mdev_core.c
@@ -57,12 +57,6 @@ struct mdev_device *mdev_from_dev(struct device *dev)
 }
 EXPORT_SYMBOL(mdev_from_dev);
 
-const guid_t *mdev_uuid(struct mdev_device *mdev)
-{
-	return &mdev->uuid;
-}
-EXPORT_SYMBOL(mdev_uuid);
-
 /* Should be called holding parent_list_lock */
 static struct mdev_parent *__find_parent_device(struct device *dev)
 {
diff --git a/include/linux/mdev.h b/include/linux/mdev.h
index 0ce30ca78db0..375a5830c3d8 100644
--- a/include/linux/mdev.h
+++ b/include/linux/mdev.h
@@ -131,7 +131,6 @@ struct mdev_driver {
 
 void *mdev_get_drvdata(struct mdev_device *mdev);
 void mdev_set_drvdata(struct mdev_device *mdev, void *data);
-const guid_t *mdev_uuid(struct mdev_device *mdev);
 
 extern struct bus_type mdev_bus_type;
 
-- 
2.21.0.777.g83232e3864


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 1/2] vfio-mdev/mtty: Simplify interrupt generation
  2019-08-02  6:59 ` [PATCH 1/2] vfio-mdev/mtty: Simplify interrupt generation Parav Pandit
@ 2019-08-06  8:15   ` Cornelia Huck
  0 siblings, 0 replies; 38+ messages in thread
From: Cornelia Huck @ 2019-08-06  8:15 UTC (permalink / raw)
  To: Parav Pandit; +Cc: kvm, wankhede, linux-kernel, alex.williamson, cjia

On Fri,  2 Aug 2019 01:59:04 -0500
Parav Pandit <parav@mellanox.com> wrote:

> While generating interrupt, mdev_state is already available for which
> interrupt is generated.
> Instead of doing indirect way from state->device->uuid-> to searching
> state linearly in linked list on every interrupt generation,
> directly use the available state.
> 
> Hence, simplify the code to use mdev_state and remove unused helper
> function with that.
> 
> Signed-off-by: Parav Pandit <parav@mellanox.com>
> ---
>  samples/vfio-mdev/mtty.c | 39 ++++++++-------------------------------
>  1 file changed, 8 insertions(+), 31 deletions(-)

This is sample code, so no high impact; but it makes sense to set a
good example.

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 2/2] vfio/mdev: Removed unused and redundant API for mdev name
  2019-08-02  6:59 ` [PATCH 2/2] vfio/mdev: Removed unused and redundant API for mdev name Parav Pandit
@ 2019-08-06  8:29   ` Cornelia Huck
  2019-08-06 13:12     ` Parav Pandit
  0 siblings, 1 reply; 38+ messages in thread
From: Cornelia Huck @ 2019-08-06  8:29 UTC (permalink / raw)
  To: Parav Pandit; +Cc: kvm, wankhede, linux-kernel, alex.williamson, cjia

On Fri,  2 Aug 2019 01:59:05 -0500
Parav Pandit <parav@mellanox.com> wrote:

> There is no single production driver who is interested in mdev device
> name.
> Additionally mdev device name is already available using core kernel
> API dev_name().

The patch description is a bit confusing: You talk about removing an
api to access the device name, but what you are actually removing is
the api to access the device's uuid. That uuid is, of course, used to
generate the device name, but the two are not the same. Using
dev_name() gives you a string containing the uuid, not the uuid.

> 
> Hence removed unused exported symbol.

I'm not really against removing this api if no driver has interest in
the device's uuid (and I'm currently not seeing why they would need it;
we can easily add it back, should the need arise); but this needs a
different description.

> 
> Signed-off-by: Parav Pandit <parav@mellanox.com>
> ---
>  drivers/vfio/mdev/mdev_core.c | 6 ------
>  include/linux/mdev.h          | 1 -
>  2 files changed, 7 deletions(-)
> 
> diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
> index b558d4cfd082..c2b809cbe59f 100644
> --- a/drivers/vfio/mdev/mdev_core.c
> +++ b/drivers/vfio/mdev/mdev_core.c
> @@ -57,12 +57,6 @@ struct mdev_device *mdev_from_dev(struct device *dev)
>  }
>  EXPORT_SYMBOL(mdev_from_dev);
>  
> -const guid_t *mdev_uuid(struct mdev_device *mdev)
> -{
> -	return &mdev->uuid;
> -}
> -EXPORT_SYMBOL(mdev_uuid);
> -
>  /* Should be called holding parent_list_lock */
>  static struct mdev_parent *__find_parent_device(struct device *dev)
>  {
> diff --git a/include/linux/mdev.h b/include/linux/mdev.h
> index 0ce30ca78db0..375a5830c3d8 100644
> --- a/include/linux/mdev.h
> +++ b/include/linux/mdev.h
> @@ -131,7 +131,6 @@ struct mdev_driver {
>  
>  void *mdev_get_drvdata(struct mdev_device *mdev);
>  void mdev_set_drvdata(struct mdev_device *mdev, void *data);
> -const guid_t *mdev_uuid(struct mdev_device *mdev);
>  
>  extern struct bus_type mdev_bus_type;
>  


^ permalink raw reply	[flat|nested] 38+ messages in thread

* RE: [PATCH 2/2] vfio/mdev: Removed unused and redundant API for mdev name
  2019-08-06  8:29   ` Cornelia Huck
@ 2019-08-06 13:12     ` Parav Pandit
  0 siblings, 0 replies; 38+ messages in thread
From: Parav Pandit @ 2019-08-06 13:12 UTC (permalink / raw)
  To: Cornelia Huck; +Cc: kvm, wankhede, linux-kernel, alex.williamson, cjia



> -----Original Message-----
> From: Cornelia Huck <cohuck@redhat.com>
> Sent: Tuesday, August 6, 2019 1:59 PM
> To: Parav Pandit <parav@mellanox.com>
> Cc: kvm@vger.kernel.org; wankhede@nvidia.com; linux-
> kernel@vger.kernel.org; alex.williamson@redhat.com; cjia@nvidia.com
> Subject: Re: [PATCH 2/2] vfio/mdev: Removed unused and redundant API for
> mdev name
> 
> On Fri,  2 Aug 2019 01:59:05 -0500
> Parav Pandit <parav@mellanox.com> wrote:
> 
> > There is no single production driver who is interested in mdev device
> > name.
> > Additionally mdev device name is already available using core kernel
> > API dev_name().
> 
> The patch description is a bit confusing: You talk about removing an api to
> access the device name, but what you are actually removing is the api to access
> the device's uuid. That uuid is, of course, used to generate the device name, but
> the two are not the same. Using
> dev_name() gives you a string containing the uuid, not the uuid.
> 
> >
> > Hence removed unused exported symbol.
> 
> I'm not really against removing this api if no driver has interest in the device's
> uuid (and I'm currently not seeing why they would need it; we can easily add it
> back, should the need arise); but this needs a different description.
> 

Ok. I understand that uuid and dev_name() are not same.
I will update the commit description.
Sending v1.

> >
> > Signed-off-by: Parav Pandit <parav@mellanox.com>
> > ---
> >  drivers/vfio/mdev/mdev_core.c | 6 ------
> >  include/linux/mdev.h          | 1 -
> >  2 files changed, 7 deletions(-)
> >
> > diff --git a/drivers/vfio/mdev/mdev_core.c
> > b/drivers/vfio/mdev/mdev_core.c index b558d4cfd082..c2b809cbe59f
> > 100644
> > --- a/drivers/vfio/mdev/mdev_core.c
> > +++ b/drivers/vfio/mdev/mdev_core.c
> > @@ -57,12 +57,6 @@ struct mdev_device *mdev_from_dev(struct device
> > *dev)  }  EXPORT_SYMBOL(mdev_from_dev);
> >
> > -const guid_t *mdev_uuid(struct mdev_device *mdev) -{
> > -	return &mdev->uuid;
> > -}
> > -EXPORT_SYMBOL(mdev_uuid);
> > -
> >  /* Should be called holding parent_list_lock */  static struct
> > mdev_parent *__find_parent_device(struct device *dev)  { diff --git
> > a/include/linux/mdev.h b/include/linux/mdev.h index
> > 0ce30ca78db0..375a5830c3d8 100644
> > --- a/include/linux/mdev.h
> > +++ b/include/linux/mdev.h
> > @@ -131,7 +131,6 @@ struct mdev_driver {
> >
> >  void *mdev_get_drvdata(struct mdev_device *mdev);  void
> > mdev_set_drvdata(struct mdev_device *mdev, void *data); -const guid_t
> > *mdev_uuid(struct mdev_device *mdev);
> >
> >  extern struct bus_type mdev_bus_type;
> >


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [PATCH v1 0/2] Simplify mtty driver and mdev core
  2019-08-02  6:59 [PATCH 0/2] Simplify mtty driver and mdev core Parav Pandit
  2019-08-02  6:59 ` [PATCH 1/2] vfio-mdev/mtty: Simplify interrupt generation Parav Pandit
  2019-08-02  6:59 ` [PATCH 2/2] vfio/mdev: Removed unused and redundant API for mdev name Parav Pandit
@ 2019-08-06 14:18 ` Parav Pandit
  2019-08-06 14:18   ` [PATCH v1 1/2] vfio-mdev/mtty: Simplify interrupt generation Parav Pandit
  2019-08-06 14:18   ` [PATCH v1 2/2] vfio/mdev: Removed unused and redundant API for mdev UUID Parav Pandit
  2019-08-08 14:12 ` [PATCH v2 0/2] Simplify mtty driver and mdev core Parav Pandit
  3 siblings, 2 replies; 38+ messages in thread
From: Parav Pandit @ 2019-08-06 14:18 UTC (permalink / raw)
  To: kvm, wankhede, linux-kernel; +Cc: parav, alex.williamson, cohuck, cjia

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.

Hence,
Patch-1 simplifies mtty sample driver to directly use mdev_state.

Patch-2, Since no production driver uses mdev_uuid() and mdev's name
(derived from UUID) is already available using core kernel dev_name(),
this patch simplifies and removes redandant mdev_uuid() exported symbol.

Parav Pandit (2):
  vfio-mdev/mtty: Simplify interrupt generation
  vfio/mdev: Removed unused and redundant API for mdev UUID

 drivers/vfio/mdev/mdev_core.c |  6 ------
 include/linux/mdev.h          |  1 -
 samples/vfio-mdev/mtty.c      | 39 +++++++----------------------------
 3 files changed, 8 insertions(+), 38 deletions(-)

-- 
2.21.0.777.g83232e3864


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [PATCH v1 1/2] vfio-mdev/mtty: Simplify interrupt generation
  2019-08-06 14:18 ` [PATCH v1 0/2] Simplify mtty driver and mdev core Parav Pandit
@ 2019-08-06 14:18   ` Parav Pandit
  2019-08-06 14:18   ` [PATCH v1 2/2] vfio/mdev: Removed unused and redundant API for mdev UUID Parav Pandit
  1 sibling, 0 replies; 38+ messages in thread
From: Parav Pandit @ 2019-08-06 14:18 UTC (permalink / raw)
  To: kvm, wankhede, linux-kernel; +Cc: parav, alex.williamson, cohuck, cjia

While generating interrupt, mdev_state is already available for which
interrupt is generated.
Instead of doing indirect way from state->device->uuid-> to searching
state linearly in linked list on every interrupt generation,
directly use the available state.

Hence, simplify the code to use mdev_state and remove unused helper
function with that.

Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Parav Pandit <parav@mellanox.com>
---
 samples/vfio-mdev/mtty.c | 39 ++++++++-------------------------------
 1 file changed, 8 insertions(+), 31 deletions(-)

diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c
index 92e770a06ea2..ce84a300a4da 100644
--- a/samples/vfio-mdev/mtty.c
+++ b/samples/vfio-mdev/mtty.c
@@ -152,20 +152,9 @@ static const struct file_operations vd_fops = {
 
 /* function prototypes */
 
-static int mtty_trigger_interrupt(const guid_t *uuid);
+static int mtty_trigger_interrupt(struct mdev_state *mdev_state);
 
 /* Helper functions */
-static struct mdev_state *find_mdev_state_by_uuid(const guid_t *uuid)
-{
-	struct mdev_state *mds;
-
-	list_for_each_entry(mds, &mdev_devices_list, next) {
-		if (guid_equal(mdev_uuid(mds->mdev), uuid))
-			return mds;
-	}
-
-	return NULL;
-}
 
 static void dump_buffer(u8 *buf, uint32_t count)
 {
@@ -337,8 +326,7 @@ static void handle_bar_write(unsigned int index, struct mdev_state *mdev_state,
 				pr_err("Serial port %d: Fifo level trigger\n",
 					index);
 #endif
-				mtty_trigger_interrupt(
-						mdev_uuid(mdev_state->mdev));
+				mtty_trigger_interrupt(mdev_state);
 			}
 		} else {
 #if defined(DEBUG_INTR)
@@ -352,8 +340,7 @@ static void handle_bar_write(unsigned int index, struct mdev_state *mdev_state,
 			 */
 			if (mdev_state->s[index].uart_reg[UART_IER] &
 								UART_IER_RLSI)
-				mtty_trigger_interrupt(
-						mdev_uuid(mdev_state->mdev));
+				mtty_trigger_interrupt(mdev_state);
 		}
 		mutex_unlock(&mdev_state->rxtx_lock);
 		break;
@@ -372,8 +359,7 @@ static void handle_bar_write(unsigned int index, struct mdev_state *mdev_state,
 				pr_err("Serial port %d: IER_THRI write\n",
 					index);
 #endif
-				mtty_trigger_interrupt(
-						mdev_uuid(mdev_state->mdev));
+				mtty_trigger_interrupt(mdev_state);
 			}
 
 			mutex_unlock(&mdev_state->rxtx_lock);
@@ -444,7 +430,7 @@ static void handle_bar_write(unsigned int index, struct mdev_state *mdev_state,
 #if defined(DEBUG_INTR)
 			pr_err("Serial port %d: MCR_OUT2 write\n", index);
 #endif
-			mtty_trigger_interrupt(mdev_uuid(mdev_state->mdev));
+			mtty_trigger_interrupt(mdev_state);
 		}
 
 		if ((mdev_state->s[index].uart_reg[UART_IER] & UART_IER_MSI) &&
@@ -452,7 +438,7 @@ static void handle_bar_write(unsigned int index, struct mdev_state *mdev_state,
 #if defined(DEBUG_INTR)
 			pr_err("Serial port %d: MCR RTS/DTR write\n", index);
 #endif
-			mtty_trigger_interrupt(mdev_uuid(mdev_state->mdev));
+			mtty_trigger_interrupt(mdev_state);
 		}
 		break;
 
@@ -503,8 +489,7 @@ static void handle_bar_read(unsigned int index, struct mdev_state *mdev_state,
 #endif
 			if (mdev_state->s[index].uart_reg[UART_IER] &
 							 UART_IER_THRI)
-				mtty_trigger_interrupt(
-					mdev_uuid(mdev_state->mdev));
+				mtty_trigger_interrupt(mdev_state);
 		}
 		mutex_unlock(&mdev_state->rxtx_lock);
 
@@ -1028,17 +1013,9 @@ static int mtty_set_irqs(struct mdev_device *mdev, uint32_t flags,
 	return ret;
 }
 
-static int mtty_trigger_interrupt(const guid_t *uuid)
+static int mtty_trigger_interrupt(struct mdev_state *mdev_state)
 {
 	int ret = -1;
-	struct mdev_state *mdev_state;
-
-	mdev_state = find_mdev_state_by_uuid(uuid);
-
-	if (!mdev_state) {
-		pr_info("%s: mdev not found\n", __func__);
-		return -EINVAL;
-	}
 
 	if ((mdev_state->irq_index == VFIO_PCI_MSI_IRQ_INDEX) &&
 	    (!mdev_state->msi_evtfd))
-- 
2.21.0.777.g83232e3864


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [PATCH v1 2/2] vfio/mdev: Removed unused and redundant API for mdev UUID
  2019-08-06 14:18 ` [PATCH v1 0/2] Simplify mtty driver and mdev core Parav Pandit
  2019-08-06 14:18   ` [PATCH v1 1/2] vfio-mdev/mtty: Simplify interrupt generation Parav Pandit
@ 2019-08-06 14:18   ` Parav Pandit
  2019-08-07  9:28     ` Cornelia Huck
  1 sibling, 1 reply; 38+ messages in thread
From: Parav Pandit @ 2019-08-06 14:18 UTC (permalink / raw)
  To: kvm, wankhede, linux-kernel; +Cc: parav, alex.williamson, cohuck, cjia

There is no single production driver who is interested in mdev device
uuid. Currently UUID is mainly used to derive a device name.
Additionally mdev device name is already available using core kernel
API dev_name().

Hence removed unused exported symbol.

Signed-off-by: Parav Pandit <parav@mellanox.com>
---
Changelog:
v0->v1:
 - Updated commit log to address comments from Cornelia
---
 drivers/vfio/mdev/mdev_core.c | 6 ------
 include/linux/mdev.h          | 1 -
 2 files changed, 7 deletions(-)

diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
index b558d4cfd082..c2b809cbe59f 100644
--- a/drivers/vfio/mdev/mdev_core.c
+++ b/drivers/vfio/mdev/mdev_core.c
@@ -57,12 +57,6 @@ struct mdev_device *mdev_from_dev(struct device *dev)
 }
 EXPORT_SYMBOL(mdev_from_dev);
 
-const guid_t *mdev_uuid(struct mdev_device *mdev)
-{
-	return &mdev->uuid;
-}
-EXPORT_SYMBOL(mdev_uuid);
-
 /* Should be called holding parent_list_lock */
 static struct mdev_parent *__find_parent_device(struct device *dev)
 {
diff --git a/include/linux/mdev.h b/include/linux/mdev.h
index 0ce30ca78db0..375a5830c3d8 100644
--- a/include/linux/mdev.h
+++ b/include/linux/mdev.h
@@ -131,7 +131,6 @@ struct mdev_driver {
 
 void *mdev_get_drvdata(struct mdev_device *mdev);
 void mdev_set_drvdata(struct mdev_device *mdev, void *data);
-const guid_t *mdev_uuid(struct mdev_device *mdev);
 
 extern struct bus_type mdev_bus_type;
 
-- 
2.21.0.777.g83232e3864


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v1 2/2] vfio/mdev: Removed unused and redundant API for mdev UUID
  2019-08-06 14:18   ` [PATCH v1 2/2] vfio/mdev: Removed unused and redundant API for mdev UUID Parav Pandit
@ 2019-08-07  9:28     ` Cornelia Huck
  2019-08-07 16:33       ` Parav Pandit
  0 siblings, 1 reply; 38+ messages in thread
From: Cornelia Huck @ 2019-08-07  9:28 UTC (permalink / raw)
  To: Parav Pandit; +Cc: kvm, wankhede, linux-kernel, alex.williamson, cjia

On Tue,  6 Aug 2019 09:18:26 -0500
Parav Pandit <parav@mellanox.com> wrote:

> There is no single production driver who is interested in mdev device
> uuid. Currently UUID is mainly used to derive a device name.
> Additionally mdev device name is already available using core kernel
> API dev_name().

Well, the mdev code actually uses the uuid to check for duplicates
before registration with the driver core would fail... I'd just drop
the two sentences talking about the device name, IMHO they don't really
add useful information; but I'll leave that decision to the maintainers.

> 
> Hence removed unused exported symbol.
> 
> Signed-off-by: Parav Pandit <parav@mellanox.com>
> ---
> Changelog:
> v0->v1:
>  - Updated commit log to address comments from Cornelia
> ---
>  drivers/vfio/mdev/mdev_core.c | 6 ------
>  include/linux/mdev.h          | 1 -
>  2 files changed, 7 deletions(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

^ permalink raw reply	[flat|nested] 38+ messages in thread

* RE: [PATCH v1 2/2] vfio/mdev: Removed unused and redundant API for mdev UUID
  2019-08-07  9:28     ` Cornelia Huck
@ 2019-08-07 16:33       ` Parav Pandit
  2019-08-08  8:29         ` Cornelia Huck
  0 siblings, 1 reply; 38+ messages in thread
From: Parav Pandit @ 2019-08-07 16:33 UTC (permalink / raw)
  To: Cornelia Huck; +Cc: kvm, wankhede, linux-kernel, alex.williamson, cjia



> -----Original Message-----
> From: Cornelia Huck <cohuck@redhat.com>
> Sent: Wednesday, August 7, 2019 2:58 PM
> To: Parav Pandit <parav@mellanox.com>
> Cc: kvm@vger.kernel.org; wankhede@nvidia.com; linux-
> kernel@vger.kernel.org; alex.williamson@redhat.com; cjia@nvidia.com
> Subject: Re: [PATCH v1 2/2] vfio/mdev: Removed unused and redundant API for
> mdev UUID
> 
> On Tue,  6 Aug 2019 09:18:26 -0500
> Parav Pandit <parav@mellanox.com> wrote:
> 
> > There is no single production driver who is interested in mdev device
> > uuid. Currently UUID is mainly used to derive a device name.
> > Additionally mdev device name is already available using core kernel
> > API dev_name().
> 
> Well, the mdev code actually uses the uuid to check for duplicates before
> registration with the driver core would fail... I'd just drop the two sentences
Yes, it does the check. But its mainly used to derive a device name.
And to ensure that there are no two devices with duplicate name, it compares with the uuid.

Even this 16 bytes storage is redundant.
Subsequently, I will submit a patch to get rid of storing this 16 bytes of UUID too.
Because for duplicate name check, device name itself is pretty good enough.

Since I ran out of time and rc-4 is going on, I differed the 3rd simplification patch.

Commit message actually came from the thoughts of 3rd patch, but I see that without it, its not so intuitive.

> talking about the device name, IMHO they don't really add useful information;
> but I'll leave that decision to the maintainers.
> 
> >
> > Hence removed unused exported symbol.
> >
> > Signed-off-by: Parav Pandit <parav@mellanox.com>
> > ---
> > Changelog:
> > v0->v1:
> >  - Updated commit log to address comments from Cornelia
> > ---
> >  drivers/vfio/mdev/mdev_core.c | 6 ------
> >  include/linux/mdev.h          | 1 -
> >  2 files changed, 7 deletions(-)
> 
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Thanks for the review.

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v1 2/2] vfio/mdev: Removed unused and redundant API for mdev UUID
  2019-08-07 16:33       ` Parav Pandit
@ 2019-08-08  8:29         ` Cornelia Huck
  2019-08-08 14:01           ` Parav Pandit
  0 siblings, 1 reply; 38+ messages in thread
From: Cornelia Huck @ 2019-08-08  8:29 UTC (permalink / raw)
  To: Parav Pandit; +Cc: kvm, wankhede, linux-kernel, alex.williamson, cjia

On Wed, 7 Aug 2019 16:33:11 +0000
Parav Pandit <parav@mellanox.com> wrote:

> > -----Original Message-----
> > From: Cornelia Huck <cohuck@redhat.com>
> > Sent: Wednesday, August 7, 2019 2:58 PM
> > To: Parav Pandit <parav@mellanox.com>
> > Cc: kvm@vger.kernel.org; wankhede@nvidia.com; linux-
> > kernel@vger.kernel.org; alex.williamson@redhat.com; cjia@nvidia.com
> > Subject: Re: [PATCH v1 2/2] vfio/mdev: Removed unused and redundant API for
> > mdev UUID
> > 
> > On Tue,  6 Aug 2019 09:18:26 -0500
> > Parav Pandit <parav@mellanox.com> wrote:
> >   
> > > There is no single production driver who is interested in mdev device
> > > uuid. Currently UUID is mainly used to derive a device name.
> > > Additionally mdev device name is already available using core kernel
> > > API dev_name().  
> > 
> > Well, the mdev code actually uses the uuid to check for duplicates before
> > registration with the driver core would fail... I'd just drop the two sentences  
> Yes, it does the check. But its mainly used to derive a device name.
> And to ensure that there are no two devices with duplicate name, it compares with the uuid.
> 
> Even this 16 bytes storage is redundant.
> Subsequently, I will submit a patch to get rid of storing this 16 bytes of UUID too.
> Because for duplicate name check, device name itself is pretty good enough.
> 
> Since I ran out of time and rc-4 is going on, I differed the 3rd simplification patch.

I'm not sure why we'd want to ditch the uuid; it's not like it is
taking up huge amounts of space... and I see the device name being
derived from the unique identifier that is the uuid, and not as the
unique identifier itself.

> 
> Commit message actually came from the thoughts of 3rd patch, but I see that without it, its not so intuitive.
> 
> > talking about the device name, IMHO they don't really add useful information;
> > but I'll leave that decision to the maintainers.
> >   
> > >
> > > Hence removed unused exported symbol.
> > >
> > > Signed-off-by: Parav Pandit <parav@mellanox.com>
> > > ---
> > > Changelog:
> > > v0->v1:
> > >  - Updated commit log to address comments from Cornelia
> > > ---
> > >  drivers/vfio/mdev/mdev_core.c | 6 ------
> > >  include/linux/mdev.h          | 1 -
> > >  2 files changed, 7 deletions(-)  
> > 
> > Reviewed-by: Cornelia Huck <cohuck@redhat.com>  
> Thanks for the review.


^ permalink raw reply	[flat|nested] 38+ messages in thread

* RE: [PATCH v1 2/2] vfio/mdev: Removed unused and redundant API for mdev UUID
  2019-08-08  8:29         ` Cornelia Huck
@ 2019-08-08 14:01           ` Parav Pandit
  0 siblings, 0 replies; 38+ messages in thread
From: Parav Pandit @ 2019-08-08 14:01 UTC (permalink / raw)
  To: Cornelia Huck; +Cc: kvm, wankhede, linux-kernel, alex.williamson, cjia



> -----Original Message-----
> From: Cornelia Huck <cohuck@redhat.com>
> Sent: Thursday, August 8, 2019 2:00 PM
> To: Parav Pandit <parav@mellanox.com>
> Cc: kvm@vger.kernel.org; wankhede@nvidia.com; linux-
> kernel@vger.kernel.org; alex.williamson@redhat.com; cjia@nvidia.com
> Subject: Re: [PATCH v1 2/2] vfio/mdev: Removed unused and redundant API
> for mdev UUID
> 
> On Wed, 7 Aug 2019 16:33:11 +0000
> Parav Pandit <parav@mellanox.com> wrote:
> 
> > > -----Original Message-----
> > > From: Cornelia Huck <cohuck@redhat.com>
> > > Sent: Wednesday, August 7, 2019 2:58 PM
> > > To: Parav Pandit <parav@mellanox.com>
> > > Cc: kvm@vger.kernel.org; wankhede@nvidia.com; linux-
> > > kernel@vger.kernel.org; alex.williamson@redhat.com; cjia@nvidia.com
> > > Subject: Re: [PATCH v1 2/2] vfio/mdev: Removed unused and redundant
> > > API for mdev UUID
> > >
> > > On Tue,  6 Aug 2019 09:18:26 -0500
> > > Parav Pandit <parav@mellanox.com> wrote:
> > >
> > > > There is no single production driver who is interested in mdev
> > > > device uuid. Currently UUID is mainly used to derive a device name.
> > > > Additionally mdev device name is already available using core
> > > > kernel API dev_name().
> > >
> > > Well, the mdev code actually uses the uuid to check for duplicates
> > > before registration with the driver core would fail... I'd just drop
> > > the two sentences
> > Yes, it does the check. But its mainly used to derive a device name.
> > And to ensure that there are no two devices with duplicate name, it
> compares with the uuid.
> >
> > Even this 16 bytes storage is redundant.
> > Subsequently, I will submit a patch to get rid of storing this 16 bytes of
> UUID too.
> > Because for duplicate name check, device name itself is pretty good
> enough.
> >
> > Since I ran out of time and rc-4 is going on, I differed the 3rd simplification
> patch.
> 
> I'm not sure why we'd want to ditch the uuid; it's not like it is taking up huge
> amounts of space... and I see the device name being derived from the
> unique identifier that is the uuid, and not as the unique identifier itself.
>
Its just extra storage where ID is already present in device name.
Its redundant. Same functionality can be achieved without its storage, so it's better to simplify.
Anyways, will handle it right after this two patches.

I realized that I had typo in the email of Kirti. So resending it with corrected email.

> >
> > Commit message actually came from the thoughts of 3rd patch, but I see
> that without it, its not so intuitive.
> >
> > > talking about the device name, IMHO they don't really add useful
> > > information; but I'll leave that decision to the maintainers.
> > >
> > > >
> > > > Hence removed unused exported symbol.
> > > >
> > > > Signed-off-by: Parav Pandit <parav@mellanox.com>
> > > > ---
> > > > Changelog:
> > > > v0->v1:
> > > >  - Updated commit log to address comments from Cornelia
> > > > ---
> > > >  drivers/vfio/mdev/mdev_core.c | 6 ------
> > > >  include/linux/mdev.h          | 1 -
> > > >  2 files changed, 7 deletions(-)
> > >
> > > Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> > Thanks for the review.


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [PATCH v2 0/2] Simplify mtty driver and mdev core
  2019-08-02  6:59 [PATCH 0/2] Simplify mtty driver and mdev core Parav Pandit
                   ` (2 preceding siblings ...)
  2019-08-06 14:18 ` [PATCH v1 0/2] Simplify mtty driver and mdev core Parav Pandit
@ 2019-08-08 14:12 ` Parav Pandit
  2019-08-08 14:12   ` [PATCH v2 1/2] vfio-mdev/mtty: Simplify interrupt generation Parav Pandit
                     ` (2 more replies)
  3 siblings, 3 replies; 38+ messages in thread
From: Parav Pandit @ 2019-08-08 14:12 UTC (permalink / raw)
  To: kvm, kwankhede, linux-kernel; +Cc: parav, alex.williamson, cohuck, cjia

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.

Hence,
Patch-1 simplifies mtty sample driver to directly use mdev_state.

Patch-2, Since no production driver uses mdev_uuid(), simplifies and
removes redandant mdev_uuid() exported symbol.

---
Changelog:
v1->v2:
 - Corrected email of Kirti
 - Updated cover letter commit log to address comment from Cornelia
 - Added Reviewed-by tag
v0->v1:
 - Updated commit log

Parav Pandit (2):
  vfio-mdev/mtty: Simplify interrupt generation
  vfio/mdev: Removed unused and redundant API for mdev UUID

 drivers/vfio/mdev/mdev_core.c |  6 ------
 include/linux/mdev.h          |  1 -
 samples/vfio-mdev/mtty.c      | 39 +++++++----------------------------
 3 files changed, 8 insertions(+), 38 deletions(-)

-- 
2.21.0.777.g83232e3864


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [PATCH v2 1/2] vfio-mdev/mtty: Simplify interrupt generation
  2019-08-08 14:12 ` [PATCH v2 0/2] Simplify mtty driver and mdev core Parav Pandit
@ 2019-08-08 14:12   ` Parav Pandit
  2019-08-13 16:39     ` Christoph Hellwig
  2019-08-08 14:12   ` [PATCH v2 2/2] vfio/mdev: Removed unused and redundant API for mdev UUID Parav Pandit
  2019-08-08 23:02   ` [PATCH v2 0/2] Simplify mtty driver and mdev core Alex Williamson
  2 siblings, 1 reply; 38+ messages in thread
From: Parav Pandit @ 2019-08-08 14:12 UTC (permalink / raw)
  To: kvm, kwankhede, linux-kernel; +Cc: parav, alex.williamson, cohuck, cjia

While generating interrupt, mdev_state is already available for which
interrupt is generated.
Instead of doing indirect way from state->device->uuid-> to searching
state linearly in linked list on every interrupt generation,
directly use the available state.

Hence, simplify the code to use mdev_state and remove unused helper
function with that.

Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Parav Pandit <parav@mellanox.com>
---
 samples/vfio-mdev/mtty.c | 39 ++++++++-------------------------------
 1 file changed, 8 insertions(+), 31 deletions(-)

diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c
index 92e770a06ea2..ce84a300a4da 100644
--- a/samples/vfio-mdev/mtty.c
+++ b/samples/vfio-mdev/mtty.c
@@ -152,20 +152,9 @@ static const struct file_operations vd_fops = {
 
 /* function prototypes */
 
-static int mtty_trigger_interrupt(const guid_t *uuid);
+static int mtty_trigger_interrupt(struct mdev_state *mdev_state);
 
 /* Helper functions */
-static struct mdev_state *find_mdev_state_by_uuid(const guid_t *uuid)
-{
-	struct mdev_state *mds;
-
-	list_for_each_entry(mds, &mdev_devices_list, next) {
-		if (guid_equal(mdev_uuid(mds->mdev), uuid))
-			return mds;
-	}
-
-	return NULL;
-}
 
 static void dump_buffer(u8 *buf, uint32_t count)
 {
@@ -337,8 +326,7 @@ static void handle_bar_write(unsigned int index, struct mdev_state *mdev_state,
 				pr_err("Serial port %d: Fifo level trigger\n",
 					index);
 #endif
-				mtty_trigger_interrupt(
-						mdev_uuid(mdev_state->mdev));
+				mtty_trigger_interrupt(mdev_state);
 			}
 		} else {
 #if defined(DEBUG_INTR)
@@ -352,8 +340,7 @@ static void handle_bar_write(unsigned int index, struct mdev_state *mdev_state,
 			 */
 			if (mdev_state->s[index].uart_reg[UART_IER] &
 								UART_IER_RLSI)
-				mtty_trigger_interrupt(
-						mdev_uuid(mdev_state->mdev));
+				mtty_trigger_interrupt(mdev_state);
 		}
 		mutex_unlock(&mdev_state->rxtx_lock);
 		break;
@@ -372,8 +359,7 @@ static void handle_bar_write(unsigned int index, struct mdev_state *mdev_state,
 				pr_err("Serial port %d: IER_THRI write\n",
 					index);
 #endif
-				mtty_trigger_interrupt(
-						mdev_uuid(mdev_state->mdev));
+				mtty_trigger_interrupt(mdev_state);
 			}
 
 			mutex_unlock(&mdev_state->rxtx_lock);
@@ -444,7 +430,7 @@ static void handle_bar_write(unsigned int index, struct mdev_state *mdev_state,
 #if defined(DEBUG_INTR)
 			pr_err("Serial port %d: MCR_OUT2 write\n", index);
 #endif
-			mtty_trigger_interrupt(mdev_uuid(mdev_state->mdev));
+			mtty_trigger_interrupt(mdev_state);
 		}
 
 		if ((mdev_state->s[index].uart_reg[UART_IER] & UART_IER_MSI) &&
@@ -452,7 +438,7 @@ static void handle_bar_write(unsigned int index, struct mdev_state *mdev_state,
 #if defined(DEBUG_INTR)
 			pr_err("Serial port %d: MCR RTS/DTR write\n", index);
 #endif
-			mtty_trigger_interrupt(mdev_uuid(mdev_state->mdev));
+			mtty_trigger_interrupt(mdev_state);
 		}
 		break;
 
@@ -503,8 +489,7 @@ static void handle_bar_read(unsigned int index, struct mdev_state *mdev_state,
 #endif
 			if (mdev_state->s[index].uart_reg[UART_IER] &
 							 UART_IER_THRI)
-				mtty_trigger_interrupt(
-					mdev_uuid(mdev_state->mdev));
+				mtty_trigger_interrupt(mdev_state);
 		}
 		mutex_unlock(&mdev_state->rxtx_lock);
 
@@ -1028,17 +1013,9 @@ static int mtty_set_irqs(struct mdev_device *mdev, uint32_t flags,
 	return ret;
 }
 
-static int mtty_trigger_interrupt(const guid_t *uuid)
+static int mtty_trigger_interrupt(struct mdev_state *mdev_state)
 {
 	int ret = -1;
-	struct mdev_state *mdev_state;
-
-	mdev_state = find_mdev_state_by_uuid(uuid);
-
-	if (!mdev_state) {
-		pr_info("%s: mdev not found\n", __func__);
-		return -EINVAL;
-	}
 
 	if ((mdev_state->irq_index == VFIO_PCI_MSI_IRQ_INDEX) &&
 	    (!mdev_state->msi_evtfd))
-- 
2.21.0.777.g83232e3864


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [PATCH v2 2/2] vfio/mdev: Removed unused and redundant API for mdev UUID
  2019-08-08 14:12 ` [PATCH v2 0/2] Simplify mtty driver and mdev core Parav Pandit
  2019-08-08 14:12   ` [PATCH v2 1/2] vfio-mdev/mtty: Simplify interrupt generation Parav Pandit
@ 2019-08-08 14:12   ` Parav Pandit
  2019-08-13 16:39     ` Christoph Hellwig
  2019-08-16 15:22     ` Cornelia Huck
  2019-08-08 23:02   ` [PATCH v2 0/2] Simplify mtty driver and mdev core Alex Williamson
  2 siblings, 2 replies; 38+ messages in thread
From: Parav Pandit @ 2019-08-08 14:12 UTC (permalink / raw)
  To: kvm, kwankhede, linux-kernel; +Cc: parav, alex.williamson, cohuck, cjia

There is no single production driver who is interested in mdev device
uuid. Currently UUID is mainly used to derive a device name.
Additionally mdev device name is already available using core kernel
API dev_name().

Hence removed unused exported symbol.

Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Parav Pandit <parav@mellanox.com>
---
Changelog:
v0->v1:
 - Updated commit log to address comments from Cornelia
---
 drivers/vfio/mdev/mdev_core.c | 6 ------
 include/linux/mdev.h          | 1 -
 2 files changed, 7 deletions(-)

diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
index b558d4cfd082..c2b809cbe59f 100644
--- a/drivers/vfio/mdev/mdev_core.c
+++ b/drivers/vfio/mdev/mdev_core.c
@@ -57,12 +57,6 @@ struct mdev_device *mdev_from_dev(struct device *dev)
 }
 EXPORT_SYMBOL(mdev_from_dev);
 
-const guid_t *mdev_uuid(struct mdev_device *mdev)
-{
-	return &mdev->uuid;
-}
-EXPORT_SYMBOL(mdev_uuid);
-
 /* Should be called holding parent_list_lock */
 static struct mdev_parent *__find_parent_device(struct device *dev)
 {
diff --git a/include/linux/mdev.h b/include/linux/mdev.h
index 0ce30ca78db0..375a5830c3d8 100644
--- a/include/linux/mdev.h
+++ b/include/linux/mdev.h
@@ -131,7 +131,6 @@ struct mdev_driver {
 
 void *mdev_get_drvdata(struct mdev_device *mdev);
 void mdev_set_drvdata(struct mdev_device *mdev, void *data);
-const guid_t *mdev_uuid(struct mdev_device *mdev);
 
 extern struct bus_type mdev_bus_type;
 
-- 
2.21.0.777.g83232e3864


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 0/2] Simplify mtty driver and mdev core
  2019-08-08 14:12 ` [PATCH v2 0/2] Simplify mtty driver and mdev core Parav Pandit
  2019-08-08 14:12   ` [PATCH v2 1/2] vfio-mdev/mtty: Simplify interrupt generation Parav Pandit
  2019-08-08 14:12   ` [PATCH v2 2/2] vfio/mdev: Removed unused and redundant API for mdev UUID Parav Pandit
@ 2019-08-08 23:02   ` Alex Williamson
  2019-08-09  8:07     ` Cornelia Huck
                       ` (2 more replies)
  2 siblings, 3 replies; 38+ messages in thread
From: Alex Williamson @ 2019-08-08 23:02 UTC (permalink / raw)
  To: Parav Pandit; +Cc: kvm, kwankhede, linux-kernel, cohuck, cjia

On Thu,  8 Aug 2019 09:12:53 -0500
Parav Pandit <parav@mellanox.com> 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.  Is that really something we want to make more
difficult in order to optimize a sample driver?  Knowing that an mdev
device uses a UUID for it's name, as tools like libvirt and mdevctl
expect, is it really worthwhile to remove such a trivial API?

> Hence,
> Patch-1 simplifies mtty sample driver to directly use mdev_state.
> 
> Patch-2, Since no production driver uses mdev_uuid(), simplifies and
> removes redandant mdev_uuid() exported symbol.

s/no production driver/no in-kernel production driver/

I'd be interested to hear how the NVIDIA folks make use of this API
interface.  Thanks,

Alex

> ---
> Changelog:
> v1->v2:
>  - Corrected email of Kirti
>  - Updated cover letter commit log to address comment from Cornelia
>  - Added Reviewed-by tag
> v0->v1:
>  - Updated commit log
> 
> Parav Pandit (2):
>   vfio-mdev/mtty: Simplify interrupt generation
>   vfio/mdev: Removed unused and redundant API for mdev UUID
> 
>  drivers/vfio/mdev/mdev_core.c |  6 ------
>  include/linux/mdev.h          |  1 -
>  samples/vfio-mdev/mtty.c      | 39 +++++++----------------------------
>  3 files changed, 8 insertions(+), 38 deletions(-)
> 


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 0/2] Simplify mtty driver and mdev core
  2019-08-08 23:02   ` [PATCH v2 0/2] Simplify mtty driver and mdev core Alex Williamson
@ 2019-08-09  8:07     ` Cornelia Huck
  2019-08-12 11:35     ` Kirti Wankhede
  2019-08-13 14:48     ` Parav Pandit
  2 siblings, 0 replies; 38+ messages in thread
From: Cornelia Huck @ 2019-08-09  8:07 UTC (permalink / raw)
  To: Alex Williamson; +Cc: Parav Pandit, kvm, kwankhede, linux-kernel, cjia

On Thu, 8 Aug 2019 17:02:47 -0600
Alex Williamson <alex.williamson@redhat.com> wrote:

> On Thu,  8 Aug 2019 09:12:53 -0500
> Parav Pandit <parav@mellanox.com> 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?

Not sure how useful the sample driver is as a template; blindly copying
their interrupt handling is probably not a good idea.

> 
> 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.  Is that really something we want to make more
> difficult in order to optimize a sample driver?  Knowing that an mdev
> device uses a UUID for it's name, as tools like libvirt and mdevctl
> expect, is it really worthwhile to remove such a trivial API?

Ripping out the uuid is a bad idea, I agree. The device name simply is
no good replacement for that.

If there's a good use case for using the uuid in a vendor driver, let's
keep the accessor. But then we probably should either leave the sample
driver alone, or add a more compelling use of the api there.

> 
> > Hence,
> > Patch-1 simplifies mtty sample driver to directly use mdev_state.
> > 
> > Patch-2, Since no production driver uses mdev_uuid(), simplifies and
> > removes redandant mdev_uuid() exported symbol.  
> 
> s/no production driver/no in-kernel production driver/
> 
> I'd be interested to hear how the NVIDIA folks make use of this API
> interface.  Thanks,
> 
> Alex
> 
> > ---
> > Changelog:
> > v1->v2:
> >  - Corrected email of Kirti
> >  - Updated cover letter commit log to address comment from Cornelia
> >  - Added Reviewed-by tag
> > v0->v1:
> >  - Updated commit log
> > 
> > Parav Pandit (2):
> >   vfio-mdev/mtty: Simplify interrupt generation
> >   vfio/mdev: Removed unused and redundant API for mdev UUID
> > 
> >  drivers/vfio/mdev/mdev_core.c |  6 ------
> >  include/linux/mdev.h          |  1 -
> >  samples/vfio-mdev/mtty.c      | 39 +++++++----------------------------
> >  3 files changed, 8 insertions(+), 38 deletions(-)
> >   
> 


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 0/2] Simplify mtty driver and mdev core
  2019-08-08 23:02   ` [PATCH v2 0/2] Simplify mtty driver and mdev core Alex Williamson
  2019-08-09  8:07     ` Cornelia Huck
@ 2019-08-12 11:35     ` Kirti Wankhede
  2019-08-13 14:40       ` Parav Pandit
  2019-08-13 14:48     ` Parav Pandit
  2 siblings, 1 reply; 38+ messages in thread
From: Kirti Wankhede @ 2019-08-12 11:35 UTC (permalink / raw)
  To: Alex Williamson, Parav Pandit; +Cc: kvm, linux-kernel, cohuck, cjia



On 8/9/2019 4:32 AM, Alex Williamson wrote:
> On Thu,  8 Aug 2019 09:12:53 -0500
> Parav Pandit <parav@mellanox.com> 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
> device uses a UUID for it's name, as tools like libvirt and mdevctl
> expect, is it really worthwhile to remove such a trivial API?
> 
>> Hence,
>> Patch-1 simplifies mtty sample driver to directly use mdev_state.
>>
>> Patch-2, Since no production driver uses mdev_uuid(), simplifies and
>> removes redandant mdev_uuid() exported symbol.
> 
> s/no production driver/no in-kernel production driver/
> 
> I'd be interested to hear how the NVIDIA folks make use of this API
> interface.  Thanks,
> 

Yes, NVIDIA mdev driver do use this interface. I don't agree on removing
mdev_uuid() interface.

Thanks,
Kirti


> Alex
> 
>> ---
>> Changelog:
>> v1->v2:
>>  - Corrected email of Kirti
>>  - Updated cover letter commit log to address comment from Cornelia
>>  - Added Reviewed-by tag
>> v0->v1:
>>  - Updated commit log
>>
>> Parav Pandit (2):
>>   vfio-mdev/mtty: Simplify interrupt generation
>>   vfio/mdev: Removed unused and redundant API for mdev UUID
>>
>>  drivers/vfio/mdev/mdev_core.c |  6 ------
>>  include/linux/mdev.h          |  1 -
>>  samples/vfio-mdev/mtty.c      | 39 +++++++----------------------------
>>  3 files changed, 8 insertions(+), 38 deletions(-)
>>
> 

^ permalink raw reply	[flat|nested] 38+ messages in thread

* RE: [PATCH v2 0/2] Simplify mtty driver and mdev core
  2019-08-12 11:35     ` Kirti Wankhede
@ 2019-08-13 14:40       ` Parav Pandit
  2019-08-13 14:52         ` Alex Williamson
  2019-08-13 16:37         ` Christoph Hellwig
  0 siblings, 2 replies; 38+ messages in thread
From: Parav Pandit @ 2019-08-13 14:40 UTC (permalink / raw)
  To: Kirti Wankhede, Alex Williamson; +Cc: kvm, linux-kernel, cohuck, cjia



> -----Original Message-----
> From: Kirti Wankhede <kwankhede@nvidia.com>
> Sent: Monday, August 12, 2019 5:06 PM
> To: Alex Williamson <alex.williamson@redhat.com>; Parav Pandit
> <parav@mellanox.com>
> Cc: kvm@vger.kernel.org; linux-kernel@vger.kernel.org; cohuck@redhat.com;
> cjia@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 <parav@mellanox.com> 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 device uses a UUID for
> > it's name, as tools like libvirt and mdevctl expect, is it really
> > worthwhile to remove such a trivial API?
> >
> >> Hence,
> >> Patch-1 simplifies mtty sample driver to directly use mdev_state.
> >>
> >> Patch-2, Since no production driver uses mdev_uuid(), simplifies and
> >> removes redandant mdev_uuid() exported symbol.
> >
> > s/no production driver/no in-kernel production driver/
> >
> > I'd be interested to hear how the NVIDIA folks make use of this API
> > interface.  Thanks,
> >
> 
> Yes, NVIDIA mdev driver do use this interface. I don't agree on removing
> mdev_uuid() interface.
> 
We need to ask Greg or Linus on the kernel policy on whether an API should exist without in-kernel driver.
We don't add such API in netdev, rdma and possibly other subsystem.
Where can we find this mdev driver in-tree?

^ permalink raw reply	[flat|nested] 38+ messages in thread

* RE: [PATCH v2 0/2] Simplify mtty driver and mdev core
  2019-08-08 23:02   ` [PATCH v2 0/2] Simplify mtty driver and mdev core Alex Williamson
  2019-08-09  8:07     ` Cornelia Huck
  2019-08-12 11:35     ` Kirti Wankhede
@ 2019-08-13 14:48     ` Parav Pandit
  2 siblings, 0 replies; 38+ messages in thread
From: Parav Pandit @ 2019-08-13 14:48 UTC (permalink / raw)
  To: Alex Williamson; +Cc: kvm, kwankhede, linux-kernel, cohuck, cjia

Hi Alex,


> -----Original Message-----
> From: Alex Williamson <alex.williamson@redhat.com>
> Sent: Friday, August 9, 2019 4:33 AM
> To: Parav Pandit <parav@mellanox.com>
> Cc: kvm@vger.kernel.org; kwankhede@nvidia.com; linux-
> kernel@vger.kernel.org; cohuck@redhat.com; cjia@nvidia.com
> Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core
> 
> On Thu,  8 Aug 2019 09:12:53 -0500
> Parav Pandit <parav@mellanox.com> 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.
> 
This is not the common practice in the kernel to provide exported symbol for every single field of the structure.

> 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.  
Can you please point to the kernel code that accesses the UUID?

> Is that really something we want to make more difficult in
> order to optimize a sample driver?  Knowing that an mdev device uses a UUID
> for it's name, as tools like libvirt and mdevctl expect, is it really worthwhile to
> remove such a trivial API?
> 
Yes. it is worthwhile to not keep any dead code in the kernel when there is no in-kernel driver using it.
Did I miss a caller?
Sample driver is setting wrong example of how/when uuid is used.
There has be better example to show how/when/why to use it.
Out of tree driver doesn't qualify API addition to my understanding.
I like to listen to Greg and others for an API inclusion without user as I haven't come across such practice in other subsystems such as nvme, netdev, rdma.

> > Hence,
> > Patch-1 simplifies mtty sample driver to directly use mdev_state.
> >
> > Patch-2, Since no production driver uses mdev_uuid(), simplifies and
> > removes redandant mdev_uuid() exported symbol.
> 
> s/no production driver/no in-kernel production driver/
> 
> I'd be interested to hear how the NVIDIA folks make use of this API interface.
> Thanks,
> 
> Alex
> 
> > ---
> > Changelog:
> > v1->v2:
> >  - Corrected email of Kirti
> >  - Updated cover letter commit log to address comment from Cornelia
> >  - Added Reviewed-by tag
> > v0->v1:
> >  - Updated commit log
> >
> > Parav Pandit (2):
> >   vfio-mdev/mtty: Simplify interrupt generation
> >   vfio/mdev: Removed unused and redundant API for mdev UUID
> >
> >  drivers/vfio/mdev/mdev_core.c |  6 ------
> >  include/linux/mdev.h          |  1 -
> >  samples/vfio-mdev/mtty.c      | 39 +++++++----------------------------
> >  3 files changed, 8 insertions(+), 38 deletions(-)
> >


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 0/2] Simplify mtty driver and mdev core
  2019-08-13 14:40       ` Parav Pandit
@ 2019-08-13 14:52         ` Alex Williamson
  2019-08-13 16:28           ` Parav Pandit
  2019-08-13 16:37         ` Christoph Hellwig
  1 sibling, 1 reply; 38+ messages in thread
From: Alex Williamson @ 2019-08-13 14:52 UTC (permalink / raw)
  To: Parav Pandit; +Cc: Kirti Wankhede, kvm, linux-kernel, cohuck, cjia

On Tue, 13 Aug 2019 14:40:02 +0000
Parav Pandit <parav@mellanox.com> wrote:

> > -----Original Message-----
> > From: Kirti Wankhede <kwankhede@nvidia.com>
> > Sent: Monday, August 12, 2019 5:06 PM
> > To: Alex Williamson <alex.williamson@redhat.com>; Parav Pandit
> > <parav@mellanox.com>
> > Cc: kvm@vger.kernel.org; linux-kernel@vger.kernel.org; cohuck@redhat.com;
> > cjia@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 <parav@mellanox.com> 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 device uses a UUID for
> > > it's name, as tools like libvirt and mdevctl expect, is it really
> > > worthwhile to remove such a trivial API?
> > >  
> > >> Hence,
> > >> Patch-1 simplifies mtty sample driver to directly use mdev_state.
> > >>
> > >> Patch-2, Since no production driver uses mdev_uuid(), simplifies and
> > >> removes redandant mdev_uuid() exported symbol.  
> > >
> > > s/no production driver/no in-kernel production driver/
> > >
> > > I'd be interested to hear how the NVIDIA folks make use of this API
> > > interface.  Thanks,
> > >  
> > 
> > Yes, NVIDIA mdev driver do use this interface. I don't agree on removing
> > mdev_uuid() interface.
> >   
> We need to ask Greg or Linus on the kernel policy on whether an API
> should exist without in-kernel driver. We don't add such API in
> netdev, rdma and possibly other subsystem. Where can we find this
> mdev driver in-tree?

We probably would not have added the API only for an out of tree
driver, but we do have a sample driver that uses it, even if it's
rather convoluted.  The sample driver is showing an example of using the
API, which is rather its purpose more so than absolutely efficient
interrupt handling.  Also, let's not overstate what this particular
API callback provides, it's simply access to the uuid of the device,
which is a fundamental property of a mediated device.  This API was
added simply to provide data abstraction, allowing the struct
mdev_device to be opaque to vendor drivers.  Thanks,

Alex

^ permalink raw reply	[flat|nested] 38+ messages in thread

* RE: [PATCH v2 0/2] Simplify mtty driver and mdev core
  2019-08-13 14:52         ` Alex Williamson
@ 2019-08-13 16:28           ` Parav Pandit
  2019-08-13 16:34             ` Cornelia Huck
  2019-08-13 17:11             ` Alex Williamson
  0 siblings, 2 replies; 38+ messages in thread
From: Parav Pandit @ 2019-08-13 16:28 UTC (permalink / raw)
  To: Alex Williamson; +Cc: Kirti Wankhede, kvm, linux-kernel, cohuck, cjia



> -----Original Message-----
> From: Alex Williamson <alex.williamson@redhat.com>
> Sent: Tuesday, August 13, 2019 8:23 PM
> To: Parav Pandit <parav@mellanox.com>
> Cc: Kirti Wankhede <kwankhede@nvidia.com>; kvm@vger.kernel.org; linux-
> kernel@vger.kernel.org; cohuck@redhat.com; cjia@nvidia.com
> Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core
> 
> On Tue, 13 Aug 2019 14:40:02 +0000
> Parav Pandit <parav@mellanox.com> wrote:
> 
> > > -----Original Message-----
> > > From: Kirti Wankhede <kwankhede@nvidia.com>
> > > Sent: Monday, August 12, 2019 5:06 PM
> > > To: Alex Williamson <alex.williamson@redhat.com>; Parav Pandit
> > > <parav@mellanox.com>
> > > Cc: kvm@vger.kernel.org; linux-kernel@vger.kernel.org;
> > > cohuck@redhat.com; cjia@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
> > > > <parav@mellanox.com> 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 device uses a
> > > > UUID for it's name, as tools like libvirt and mdevctl expect, is
> > > > it really worthwhile to remove such a trivial API?
> > > >
> > > >> Hence,
> > > >> Patch-1 simplifies mtty sample driver to directly use mdev_state.
> > > >>
> > > >> Patch-2, Since no production driver uses mdev_uuid(), simplifies
> > > >> and removes redandant mdev_uuid() exported symbol.
> > > >
> > > > s/no production driver/no in-kernel production driver/
> > > >
> > > > I'd be interested to hear how the NVIDIA folks make use of this
> > > > API interface.  Thanks,
> > > >
> > >
> > > Yes, NVIDIA mdev driver do use this interface. I don't agree on
> > > removing
> > > mdev_uuid() interface.
> > >
> > We need to ask Greg or Linus on the kernel policy on whether an API
> > should exist without in-kernel driver. We don't add such API in
> > netdev, rdma and possibly other subsystem. Where can we find this mdev
> > driver in-tree?
> 
> We probably would not have added the API only for an out of tree driver, but
> we do have a sample driver that uses it, even if it's rather convoluted.  The
> sample driver is showing an example of using the API, which is rather its
> purpose more so than absolutely efficient interrupt handling.  
For showing API use, it doesn't have to convoluted that too in interrupt handling code.
It could be just dev_info(" UUID print..)
But the whole point is to have useful API that non sample driver need to use.
And there is none.
In bigger objective, I wanted to discuss post this cleanup patch, is to expand mdev to have more user friendly device names.

Before we reach there, I should include a patch that eliminates storing UUID itself in the mdev_device.

> Also, let's not
> overstate what this particular API callback provides, it's simply access to the
> uuid of the device, which is a fundamental property of a mediated device.
This fundamental property is available in form of device name already.

> API was added simply to provide data abstraction, allowing the struct
> mdev_device to be opaque to vendor drivers.  Thanks,
> 
I get that part. I prefer to remove the UUID itself from the structure and therefore removing this API makes lot more sense?

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 0/2] Simplify mtty driver and mdev core
  2019-08-13 16:28           ` Parav Pandit
@ 2019-08-13 16:34             ` Cornelia Huck
  2019-08-13 17:11             ` Alex Williamson
  1 sibling, 0 replies; 38+ messages in thread
From: Cornelia Huck @ 2019-08-13 16:34 UTC (permalink / raw)
  To: Parav Pandit; +Cc: Alex Williamson, Kirti Wankhede, kvm, linux-kernel, cjia

On Tue, 13 Aug 2019 16:28:53 +0000
Parav Pandit <parav@mellanox.com> wrote:

> In bigger objective, I wanted to discuss post this cleanup patch, is to expand mdev to have more user friendly device names.

Uh, what is unfriendly about uuids?

> 
> Before we reach there, I should include a patch that eliminates storing UUID itself in the mdev_device.

I do not think that's a great idea. A uuid is, well, a unique
identifier. What's so bad about it that it should be eliminated?

> 
> > Also, let's not
> > overstate what this particular API callback provides, it's simply access to the
> > uuid of the device, which is a fundamental property of a mediated device.  
> This fundamental property is available in form of device name already.

Let me reiterate that the device name is a string containing a
formatted uuid, not a uuid.

> 
> > API was added simply to provide data abstraction, allowing the struct
> > mdev_device to be opaque to vendor drivers.  Thanks,
> >   
> I get that part. I prefer to remove the UUID itself from the structure and therefore removing this API makes lot more sense?

What I don't get is why you want to eliminate the uuid in the first
place? Again, what's so bad about it?

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 0/2] Simplify mtty driver and mdev core
  2019-08-13 14:40       ` Parav Pandit
  2019-08-13 14:52         ` Alex Williamson
@ 2019-08-13 16:37         ` Christoph Hellwig
  2019-08-13 17:40           ` Greg Kroah-Hartman
  1 sibling, 1 reply; 38+ messages in thread
From: Christoph Hellwig @ 2019-08-13 16:37 UTC (permalink / raw)
  To: Parav Pandit
  Cc: Kirti Wankhede, Alex Williamson, kvm, linux-kernel, cohuck, cjia,
	Greg Kroah-Hartman

On Tue, Aug 13, 2019 at 02:40:02PM +0000, Parav Pandit wrote:
> We need to ask Greg or Linus on the kernel policy on whether an API should exist without in-kernel driver.
> We don't add such API in netdev, rdma and possibly other subsystem.
> Where can we find this mdev driver in-tree?

The clear policy is that we don't keep such symbols around.  Been
there done that only recently again.

The other interesting thing is the amount of code nvidia and partner 
developers have pushed into the kernel tree for exclusive use of their
driver it should be clearly established by now that it is a derived
work, but that is for a different discussion.

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 1/2] vfio-mdev/mtty: Simplify interrupt generation
  2019-08-08 14:12   ` [PATCH v2 1/2] vfio-mdev/mtty: Simplify interrupt generation Parav Pandit
@ 2019-08-13 16:39     ` Christoph Hellwig
  0 siblings, 0 replies; 38+ messages in thread
From: Christoph Hellwig @ 2019-08-13 16:39 UTC (permalink / raw)
  To: Parav Pandit; +Cc: kvm, kwankhede, linux-kernel, alex.williamson, cohuck, cjia

On Thu, Aug 08, 2019 at 09:12:54AM -0500, Parav Pandit wrote:
> While generating interrupt, mdev_state is already available for which
> interrupt is generated.
> Instead of doing indirect way from state->device->uuid-> to searching
> state linearly in linked list on every interrupt generation,
> directly use the available state.
> 
> Hence, simplify the code to use mdev_state and remove unused helper
> function with that.
> 
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> Signed-off-by: Parav Pandit <parav@mellanox.com>

Looks good, the old code is rather backwards and a bad example to
copy:

Reviewed-by: Christoph Hellwig <hch@lst.de>

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 2/2] vfio/mdev: Removed unused and redundant API for mdev UUID
  2019-08-08 14:12   ` [PATCH v2 2/2] vfio/mdev: Removed unused and redundant API for mdev UUID Parav Pandit
@ 2019-08-13 16:39     ` Christoph Hellwig
  2019-08-16 15:22     ` Cornelia Huck
  1 sibling, 0 replies; 38+ messages in thread
From: Christoph Hellwig @ 2019-08-13 16:39 UTC (permalink / raw)
  To: Parav Pandit; +Cc: kvm, kwankhede, linux-kernel, alex.williamson, cohuck, cjia

On Thu, Aug 08, 2019 at 09:12:55AM -0500, Parav Pandit wrote:
> There is no single production driver who is interested in mdev device
> uuid. Currently UUID is mainly used to derive a device name.
> Additionally mdev device name is already available using core kernel
> API dev_name().
> 
> Hence removed unused exported symbol.
> 
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> Signed-off-by: Parav Pandit <parav@mellanox.com>

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 0/2] Simplify mtty driver and mdev core
  2019-08-13 16:28           ` Parav Pandit
  2019-08-13 16:34             ` Cornelia Huck
@ 2019-08-13 17:11             ` Alex Williamson
  2019-08-14  5:54               ` Parav Pandit
  1 sibling, 1 reply; 38+ messages in thread
From: Alex Williamson @ 2019-08-13 17:11 UTC (permalink / raw)
  To: Parav Pandit; +Cc: Kirti Wankhede, kvm, linux-kernel, cohuck, cjia

On Tue, 13 Aug 2019 16:28:53 +0000
Parav Pandit <parav@mellanox.com> wrote:

> > -----Original Message-----
> > From: Alex Williamson <alex.williamson@redhat.com>
> > Sent: Tuesday, August 13, 2019 8:23 PM
> > To: Parav Pandit <parav@mellanox.com>
> > Cc: Kirti Wankhede <kwankhede@nvidia.com>; kvm@vger.kernel.org; linux-
> > kernel@vger.kernel.org; cohuck@redhat.com; cjia@nvidia.com
> > Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core
> > 
> > On Tue, 13 Aug 2019 14:40:02 +0000
> > Parav Pandit <parav@mellanox.com> wrote:
> >   
> > > > -----Original Message-----
> > > > From: Kirti Wankhede <kwankhede@nvidia.com>
> > > > Sent: Monday, August 12, 2019 5:06 PM
> > > > To: Alex Williamson <alex.williamson@redhat.com>; Parav Pandit
> > > > <parav@mellanox.com>
> > > > Cc: kvm@vger.kernel.org; linux-kernel@vger.kernel.org;
> > > > cohuck@redhat.com; cjia@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
> > > > > <parav@mellanox.com> 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 device uses a
> > > > > UUID for it's name, as tools like libvirt and mdevctl expect, is
> > > > > it really worthwhile to remove such a trivial API?
> > > > >  
> > > > >> Hence,
> > > > >> Patch-1 simplifies mtty sample driver to directly use mdev_state.
> > > > >>
> > > > >> Patch-2, Since no production driver uses mdev_uuid(), simplifies
> > > > >> and removes redandant mdev_uuid() exported symbol.  
> > > > >
> > > > > s/no production driver/no in-kernel production driver/
> > > > >
> > > > > I'd be interested to hear how the NVIDIA folks make use of this
> > > > > API interface.  Thanks,
> > > > >  
> > > >
> > > > Yes, NVIDIA mdev driver do use this interface. I don't agree on
> > > > removing
> > > > mdev_uuid() interface.
> > > >  
> > > We need to ask Greg or Linus on the kernel policy on whether an API
> > > should exist without in-kernel driver. We don't add such API in
> > > netdev, rdma and possibly other subsystem. Where can we find this mdev
> > > driver in-tree?  
> > 
> > We probably would not have added the API only for an out of tree driver, but
> > we do have a sample driver that uses it, even if it's rather convoluted.  The
> > sample driver is showing an example of using the API, which is rather its
> > purpose more so than absolutely efficient interrupt handling.    
> For showing API use, it doesn't have to convoluted that too in interrupt handling code.
> It could be just dev_info(" UUID print..)

I was thinking we could have the mtty driver expose a vendor sysfs
attribute providing the UUID if you insist on cleaning up the
interrupt path.

> But the whole point is to have useful API that non sample driver need to use.
> And there is none.

Kirti has already indicated this API was useful and it's not a burden
to maintain it.  The trouble is that we don't have any in-kernel mdev
drivers sophisticated enough to have the same kernel/user split as the
NVIDIA driver, but that's a feature that I believe we wish to continue
to support.  The kernel exposes the device by UUID, userspace
references the device by UUID, so it seems intuitive to provide a core
API to retrieve the UUID for a device without parsing it from a device
name string.  We can continue to add trivial sample driver use cases or
we can just agree that this is a useful interface for UUID based device.

> In bigger objective, I wanted to discuss post this cleanup patch, is
> to expand mdev to have more user friendly device names.

"Friendly" is a matter of opinion.  UUIDs provide us with consistent
names, effectively avoids name collisions which in turn effectively
avoids races in device creation, they're easy to deal with, and they're
well known.  Naming things is hard. Dealing with arbitrary user
generated names or defining a policy around acceptable naming is hard.

> Before we reach there, I should include a patch that eliminates
> storing UUID itself in the mdev_device.
> 
> > Also, let's not
> > overstate what this particular API callback provides, it's simply
> > access to the uuid of the device, which is a fundamental property
> > of a mediated device.  
> This fundamental property is available in form of device name already.

So you're wanting to optimize a sample driver interrupt handler in
order to eliminate an API which makes the UUID available without
parsing it from a string?  And the complexity grows when you later
propose that the string is now arbitrary?  It doesn't seem like a good
replacement.

> > API was added simply to provide data abstraction, allowing the
> > struct mdev_device to be opaque to vendor drivers.  Thanks,
> >   
> 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.  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,

Alex

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 0/2] Simplify mtty driver and mdev core
  2019-08-13 16:37         ` Christoph Hellwig
@ 2019-08-13 17:40           ` Greg Kroah-Hartman
  2019-08-14  5:30             ` Parav Pandit
  0 siblings, 1 reply; 38+ messages in thread
From: Greg Kroah-Hartman @ 2019-08-13 17:40 UTC (permalink / raw)
  To: Christoph Hellwig, Parav Pandit
  Cc: Kirti Wankhede, Alex Williamson, kvm, linux-kernel, cohuck, cjia

On Tue, Aug 13, 2019 at 09:37:21AM -0700, Christoph Hellwig wrote:
> On Tue, Aug 13, 2019 at 02:40:02PM +0000, Parav Pandit wrote:
> > We need to ask Greg or Linus on the kernel policy on whether an API should exist without in-kernel driver.

I "love" it when people try to ask a question of me and they don't
actually cc: me.  That means they really do not want the answer (or they
already know it...)  Thanks Christoph for adding me here.

The policy is that the api should not exist at all, everyone knows this,
why is this even a question?

> > We don't add such API in netdev, rdma and possibly other subsystem.
> > Where can we find this mdev driver in-tree?
> 
> The clear policy is that we don't keep such symbols around.  Been
> there done that only recently again.

Agreed.  If anyone knows of anything else that isn't being used, we will
be glad to free up the space by cleaning it up.

> The other interesting thing is the amount of code nvidia and partner 
> developers have pushed into the kernel tree for exclusive use of their
> driver it should be clearly established by now that it is a derived
> work, but that is for a different discussion.

That's a discussion the lawyers on their side keep wanting us to ignore,
it's as if they think we are stupid and they are "pulling one over on
us."  ugh...

thanks,

greg "not a lawyer, but spends lots of time with them" k-h

^ permalink raw reply	[flat|nested] 38+ messages in thread

* RE: [PATCH v2 0/2] Simplify mtty driver and mdev core
  2019-08-13 17:40           ` Greg Kroah-Hartman
@ 2019-08-14  5:30             ` Parav Pandit
  0 siblings, 0 replies; 38+ messages in thread
From: Parav Pandit @ 2019-08-14  5:30 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Christoph Hellwig
  Cc: Kirti Wankhede, Alex Williamson, kvm, linux-kernel, cohuck, cjia

Hi Christoph, Greg,

> -----Original Message-----
> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Sent: Tuesday, August 13, 2019 11:10 PM
> To: Christoph Hellwig <hch@infradead.org>; Parav Pandit
> <parav@mellanox.com>
> Cc: Kirti Wankhede <kwankhede@nvidia.com>; Alex Williamson
> <alex.williamson@redhat.com>; kvm@vger.kernel.org; linux-
> kernel@vger.kernel.org; cohuck@redhat.com; cjia@nvidia.com
> Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core
> 
> On Tue, Aug 13, 2019 at 09:37:21AM -0700, Christoph Hellwig wrote:
> > On Tue, Aug 13, 2019 at 02:40:02PM +0000, Parav Pandit wrote:
> > > We need to ask Greg or Linus on the kernel policy on whether an API should
> exist without in-kernel driver.
> 
> I "love" it when people try to ask a question of me and they don't actually cc:
> me.  That means they really do not want the answer (or they already know it...)
> Thanks Christoph for adding me here.
> 
I pretty much knew your answer and I was just hinting Kirti that if you ask Greg you would get the same answer.
So we better cleanup without reaching out to you. :-)

> The policy is that the api should not exist at all, everyone knows this, why is this
> even a question?
> 
Yes, I am aware of this. Few subsystems in which I worked, it has followed this policy cautiously.
But when I heard different policy for mdev, I asked others wisdom.

> > > We don't add such API in netdev, rdma and possibly other subsystem.
> > > Where can we find this mdev driver in-tree?
> >
> > The clear policy is that we don't keep such symbols around.  Been
> > there done that only recently again.
> 
> Agreed.  If anyone knows of anything else that isn't being used, we will be glad
> to free up the space by cleaning it up.
> 
Ok. so this small patchset makes sense.
Thanks for the ack and direction Christoph, Greg.

^ permalink raw reply	[flat|nested] 38+ messages in thread

* RE: [PATCH v2 0/2] Simplify mtty driver and mdev core
  2019-08-13 17:11             ` Alex Williamson
@ 2019-08-14  5:54               ` Parav Pandit
  2019-08-14  8:01                 ` Cornelia Huck
  0 siblings, 1 reply; 38+ messages in thread
From: Parav Pandit @ 2019-08-14  5:54 UTC (permalink / raw)
  To: Alex Williamson; +Cc: Kirti Wankhede, kvm, linux-kernel, cohuck, cjia



> -----Original Message-----
> From: Alex Williamson <alex.williamson@redhat.com>
> Sent: Tuesday, August 13, 2019 10:42 PM
> To: Parav Pandit <parav@mellanox.com>
> Cc: Kirti Wankhede <kwankhede@nvidia.com>; kvm@vger.kernel.org; linux-
> kernel@vger.kernel.org; cohuck@redhat.com; cjia@nvidia.com
> Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core
> 
> On Tue, 13 Aug 2019 16:28:53 +0000
> Parav Pandit <parav@mellanox.com> wrote:
> 
> > > -----Original Message-----
> > > From: Alex Williamson <alex.williamson@redhat.com>
> > > Sent: Tuesday, August 13, 2019 8:23 PM
> > > To: Parav Pandit <parav@mellanox.com>
> > > Cc: Kirti Wankhede <kwankhede@nvidia.com>; kvm@vger.kernel.org;
> > > linux- kernel@vger.kernel.org; cohuck@redhat.com; cjia@nvidia.com
> > > Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core
> > >
> > > On Tue, 13 Aug 2019 14:40:02 +0000
> > > Parav Pandit <parav@mellanox.com> wrote:
> > >
> > > > > -----Original Message-----
> > > > > From: Kirti Wankhede <kwankhede@nvidia.com>
> > > > > Sent: Monday, August 12, 2019 5:06 PM
> > > > > To: Alex Williamson <alex.williamson@redhat.com>; Parav Pandit
> > > > > <parav@mellanox.com>
> > > > > Cc: kvm@vger.kernel.org; linux-kernel@vger.kernel.org;
> > > > > cohuck@redhat.com; cjia@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
> > > > > > <parav@mellanox.com> 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
> > > > > > device uses a UUID for it's name, as tools like libvirt and
> > > > > > mdevctl expect, is it really worthwhile to remove such a trivial API?
> > > > > >
> > > > > >> Hence,
> > > > > >> Patch-1 simplifies mtty sample driver to directly use mdev_state.
> > > > > >>
> > > > > >> Patch-2, Since no production driver uses mdev_uuid(),
> > > > > >> simplifies and removes redandant mdev_uuid() exported symbol.
> > > > > >
> > > > > > s/no production driver/no in-kernel production driver/
> > > > > >
> > > > > > I'd be interested to hear how the NVIDIA folks make use of
> > > > > > this API interface.  Thanks,
> > > > > >
> > > > >
> > > > > Yes, NVIDIA mdev driver do use this interface. I don't agree on
> > > > > removing
> > > > > mdev_uuid() interface.
> > > > >
> > > > We need to ask Greg or Linus on the kernel policy on whether an
> > > > API should exist without in-kernel driver. We don't add such API
> > > > in netdev, rdma and possibly other subsystem. Where can we find
> > > > this mdev driver in-tree?
> > >
> > > We probably would not have added the API only for an out of tree
> > > driver, but we do have a sample driver that uses it, even if it's
> > > rather convoluted.  The sample driver is showing an example of using the
> API, which is rather its
> > > purpose more so than absolutely efficient interrupt handling.
> > For showing API use, it doesn't have to convoluted that too in interrupt
> handling code.
> > It could be just dev_info(" UUID print..)
> 
> I was thinking we could have the mtty driver expose a vendor sysfs attribute
> providing the UUID if you insist on cleaning up the interrupt path.
> 
A vendor driver can add its own sysfs file per device.
A while back I refactored rdma system to simplify all of such sysfs entries for several drivers.
We had hurdle when we introduced namespaces to it.
So I do not recommend adding it in the vendor driver.
Rather, mdev code can add sysfs entry per device exposing its UUID.
This will be unified across the vendors.
And for below discussion, if the device is not based on UUID, this sysfs entry won't exist.
More below.

> > But the whole point is to have useful API that non sample driver need to use.
> > And there is none.
> 
> Kirti has already indicated this API was useful and it's not a burden to maintain
> it.  The trouble is that we don't have any in-kernel mdev drivers sophisticated
> enough to have the same kernel/user split as the NVIDIA driver, but that's a
> feature that I believe we wish to continue to support.  The kernel exposes the
> device by UUID, userspace references the device by UUID, so it seems intuitive
> to provide a core API to retrieve the UUID for a device without parsing it from a
> device name string.  We can continue to add trivial sample driver use cases or
But mdev or any vendor drivers are not parsing it anywhere today.
That is why I was asking for an example production driver that explains why/how it uses it.

> we can just agree that this is a useful interface for UUID based device.
> 
> > In bigger objective, I wanted to discuss post this cleanup patch, is
> > to expand mdev to have more user friendly device names.
> 
> "Friendly" is a matter of opinion.  UUIDs provide us with consistent names,
> effectively avoids name collisions which in turn effectively avoids races in
> device creation, they're easy to deal with, and they're well known.  Naming
> things is hard. Dealing with arbitrary user generated names or defining a policy
> around acceptable naming is hard.
> 
> > Before we reach there, I should include a patch that eliminates
> > storing UUID itself in the mdev_device.
> >
> > > Also, let's not
> > > overstate what this particular API callback provides, it's simply
> > > access to the uuid of the device, which is a fundamental property of
> > > a mediated device.
> > This fundamental property is available in form of device name already.
> 
> So you're wanting to optimize a sample driver interrupt handler in order to
> eliminate an API which makes the UUID available without parsing it from a
> string?
Yes. because none production driver needs this and it can be derived from the device name.

> And the complexity grows when you later propose that the string is now
> arbitrary?  It doesn't seem like a good replacement.
> 
Well sample driver shouldn't use the API the way it uses in convoluted approach.
Just a UUID print during create() call using mdev_uuid() is good enough for sake of showing example of an API.

If we want to uniquely identify mdev using UUID, but not derive their names from UUID, by having additional parameter for its name,
It makes sense to me.
In this approach, mdev_uuid() can be added/kept if a vendor driver is interested in the UUID.
At present there is none.

> > > API was added simply to provide data abstraction, allowing the
> > > struct mdev_device to be opaque to vendor drivers.  Thanks,
> > >
> > 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.

> 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.

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 0/2] Simplify mtty driver and mdev core
  2019-08-14  5:54               ` Parav Pandit
@ 2019-08-14  8:01                 ` Cornelia Huck
  2019-08-14 12:27                   ` Parav Pandit
  0 siblings, 1 reply; 38+ messages in thread
From: Cornelia Huck @ 2019-08-14  8:01 UTC (permalink / raw)
  To: Parav Pandit; +Cc: Alex Williamson, Kirti Wankhede, kvm, linux-kernel, cjia

On Wed, 14 Aug 2019 05:54:36 +0000
Parav Pandit <parav@mellanox.com> 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?

> 
> > 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. 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?

^ permalink raw reply	[flat|nested] 38+ messages in thread

* RE: [PATCH v2 0/2] Simplify mtty driver and mdev core
  2019-08-14  8:01                 ` Cornelia Huck
@ 2019-08-14 12:27                   ` Parav Pandit
  2019-08-14 13:09                     ` Cornelia Huck
  0 siblings, 1 reply; 38+ messages in thread
From: Parav Pandit @ 2019-08-14 12:27 UTC (permalink / raw)
  To: Cornelia Huck
  Cc: Alex Williamson, Kirti Wankhede, kvm, linux-kernel, cjia,
	Jiri Pirko, netdev

+ Jiri, + netdev 
To get perspective on the ndo->phys_port_name for the representor netdev of mdev.

Hi Cornelia,

> -----Original Message-----
> From: Cornelia Huck <cohuck@redhat.com>
> Sent: Wednesday, August 14, 2019 1:32 PM
> To: Parav Pandit <parav@mellanox.com>
> Cc: Alex Williamson <alex.williamson@redhat.com>; Kirti Wankhede
> <kwankhede@nvidia.com>; kvm@vger.kernel.org; linux-
> kernel@vger.kernel.org; cjia@nvidia.com
> Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core
> 
> On Wed, 14 Aug 2019 05:54:36 +0000
> Parav Pandit <parav@mellanox.com> 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/


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 0/2] Simplify mtty driver and mdev core
  2019-08-14 12:27                   ` Parav Pandit
@ 2019-08-14 13:09                     ` Cornelia Huck
  2019-08-14 13:45                       ` Parav Pandit
  0 siblings, 1 reply; 38+ messages in thread
From: Cornelia Huck @ 2019-08-14 13:09 UTC (permalink / raw)
  To: Parav Pandit
  Cc: Alex Williamson, Kirti Wankhede, kvm, linux-kernel, cjia,
	Jiri Pirko, netdev

On Wed, 14 Aug 2019 12:27:01 +0000
Parav Pandit <parav@mellanox.com> 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 <cohuck@redhat.com>
> > Sent: Wednesday, August 14, 2019 1:32 PM
> > To: Parav Pandit <parav@mellanox.com>
> > Cc: Alex Williamson <alex.williamson@redhat.com>; Kirti Wankhede
> > <kwankhede@nvidia.com>; kvm@vger.kernel.org; linux-
> > kernel@vger.kernel.org; cjia@nvidia.com
> > Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core
> > 
> > On Wed, 14 Aug 2019 05:54:36 +0000
> > Parav Pandit <parav@mellanox.com> 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.

> 
> > >  
> > > > 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
that userspace can refer to a representor netdevice. Makes sense.
For generating that name, udev uses the phys_port_name (which
represents the devlink port, IIUC). Also makes sense.

> 
> phys_port_name has limitation to be only 15 characters long.
> UUID doesn't fit in phys_port_name.

Understood. But why do we need to derive the phys_port_name from the
mdev device name? This netdevice use case seems to be just one use case
for using mdev devices? If this is a specialized mdev type for this
setup, why not just expose a shorter identifier via an extra attribute?

> Longer UUID names are creating snow ball effect, not just in networking stack but many user space tools too.

This snowball effect mainly comes from the device name ->
phys_port_name setup, IIUC.

> (as opposed to recently introduced mdevctl, are they more mdev tools which has dependency on UUID name?)

I am aware that people have written scripts etc. to manage their mdevs.
Given that the mdev infrastructure has been around for quite some time,
I'd say the chance of some of those scripts relying on uuid names is
non-zero.

> 
> 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.

Actually, I think that second option makes much more sense, as you
avoid potentially breaking existing tooling.

> 
> > 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/
> 


^ permalink raw reply	[flat|nested] 38+ messages in thread

* RE: [PATCH v2 0/2] Simplify mtty driver and mdev core
  2019-08-14 13:09                     ` Cornelia Huck
@ 2019-08-14 13:45                       ` Parav Pandit
  2019-08-14 14:57                         ` Alex Williamson
  0 siblings, 1 reply; 38+ messages in thread
From: Parav Pandit @ 2019-08-14 13:45 UTC (permalink / raw)
  To: Cornelia Huck
  Cc: Alex Williamson, Kirti Wankhede, kvm, linux-kernel, cjia,
	Jiri Pirko, netdev



> -----Original Message-----
> From: Cornelia Huck <cohuck@redhat.com>
> Sent: Wednesday, August 14, 2019 6:39 PM
> To: Parav Pandit <parav@mellanox.com>
> Cc: Alex Williamson <alex.williamson@redhat.com>; Kirti Wankhede
> <kwankhede@nvidia.com>; kvm@vger.kernel.org; linux-
> kernel@vger.kernel.org; cjia@nvidia.com; Jiri Pirko <jiri@mellanox.com>;
> netdev@vger.kernel.org
> Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core
> 
> On Wed, 14 Aug 2019 12:27:01 +0000
> Parav Pandit <parav@mellanox.com> 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 <cohuck@redhat.com>
> > > Sent: Wednesday, August 14, 2019 1:32 PM
> > > To: Parav Pandit <parav@mellanox.com>
> > > Cc: Alex Williamson <alex.williamson@redhat.com>; Kirti Wankhede
> > > <kwankhede@nvidia.com>; kvm@vger.kernel.org; linux-
> > > kernel@vger.kernel.org; cjia@nvidia.com
> > > Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core
> > >
> > > On Wed, 14 Aug 2019 05:54:36 +0000
> > > Parav Pandit <parav@mellanox.com> 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 that
> userspace can refer to a representor netdevice. Makes sense.
> For generating that name, udev uses the phys_port_name (which represents
> the devlink port, IIUC). Also makes sense.
> 
You understood it correctly.

> >
> > phys_port_name has limitation to be only 15 characters long.
> > UUID doesn't fit in phys_port_name.
> 
> Understood. But why do we need to derive the phys_port_name from the mdev
> device name? This netdevice use case seems to be just one use case for using
> mdev devices? If this is a specialized mdev type for this setup, why not just
> expose a shorter identifier via an extra attribute?
> 
Representor netdev, represents mdev's switch port (like PCI SRIOV VF's switch port).
So user must be able to relate this two objects in similar manner as SRIOV VFs.
Phys_port_name is derived from the PCI PF and VF numbering scheme.
Similarly mdev's such port should be derived from mdev's id/name/attribute.

> > Longer UUID names are creating snow ball effect, not just in networking stack
> but many user space tools too.
> 
> This snowball effect mainly comes from the device name -> phys_port_name
> setup, IIUC.
> 
Right.

> > (as opposed to recently introduced mdevctl, are they more mdev tools
> > which has dependency on UUID name?)
> 
> I am aware that people have written scripts etc. to manage their mdevs.
> Given that the mdev infrastructure has been around for quite some time, I'd
> say the chance of some of those scripts relying on uuid names is non-zero.
> 
Ok. but those scripts have never managed networking devices.
So those scripts won't break because they will always create mdev devices using UUID.
When they use these new networking devices, they need more things than their scripts.
So user space upgrade for such mixed mode case is reasonable.

> >
> > 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.
> 
> Actually, I think that second option makes much more sense, as you avoid
> potentially breaking existing tooling.
Let's first understand of what exactly will break with existing tool if they see non_uuid based device.
> 
Existing tooling continue to work with UUID devices.
Do you have example of what can break if they see non_uuid based device name?
I think you are clear, but to be sure, UUID based creation will continue to be there. Optionally mdev will be created with alpha-numeric string, if we don't it as additional attribute.

> >
> > > 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#L6
> > 921
> > [5] https://www.openvswitch.org/
> >


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 0/2] Simplify mtty driver and mdev core
  2019-08-14 13:45                       ` Parav Pandit
@ 2019-08-14 14:57                         ` Alex Williamson
  2019-08-14 16:21                           ` Parav Pandit
  0 siblings, 1 reply; 38+ messages in thread
From: Alex Williamson @ 2019-08-14 14:57 UTC (permalink / raw)
  To: Parav Pandit
  Cc: Cornelia Huck, Kirti Wankhede, kvm, linux-kernel, cjia,
	Jiri Pirko, netdev

On Wed, 14 Aug 2019 13:45:49 +0000
Parav Pandit <parav@mellanox.com> wrote:

> > -----Original Message-----
> > From: Cornelia Huck <cohuck@redhat.com>
> > Sent: Wednesday, August 14, 2019 6:39 PM
> > To: Parav Pandit <parav@mellanox.com>
> > Cc: Alex Williamson <alex.williamson@redhat.com>; Kirti Wankhede
> > <kwankhede@nvidia.com>; kvm@vger.kernel.org; linux-
> > kernel@vger.kernel.org; cjia@nvidia.com; Jiri Pirko <jiri@mellanox.com>;
> > netdev@vger.kernel.org
> > Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core
> > 
> > On Wed, 14 Aug 2019 12:27:01 +0000
> > Parav Pandit <parav@mellanox.com> 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 <cohuck@redhat.com>
> > > > Sent: Wednesday, August 14, 2019 1:32 PM
> > > > To: Parav Pandit <parav@mellanox.com>
> > > > Cc: Alex Williamson <alex.williamson@redhat.com>; Kirti Wankhede
> > > > <kwankhede@nvidia.com>; kvm@vger.kernel.org; linux-
> > > > kernel@vger.kernel.org; cjia@nvidia.com
> > > > Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core
> > > >
> > > > On Wed, 14 Aug 2019 05:54:36 +0000
> > > > Parav Pandit <parav@mellanox.com> 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 that
> > userspace can refer to a representor netdevice. Makes sense.
> > For generating that name, udev uses the phys_port_name (which represents
> > the devlink port, IIUC). Also makes sense.
> >   
> You understood it correctly.
> 
> > >
> > > phys_port_name has limitation to be only 15 characters long.
> > > UUID doesn't fit in phys_port_name.  
> > 
> > Understood. But why do we need to derive the phys_port_name from the mdev
> > device name? This netdevice use case seems to be just one use case for using
> > mdev devices? If this is a specialized mdev type for this setup, why not just
> > expose a shorter identifier via an extra attribute?
> >   
> Representor netdev, represents mdev's switch port (like PCI SRIOV VF's switch port).
> So user must be able to relate this two objects in similar manner as SRIOV VFs.
> Phys_port_name is derived from the PCI PF and VF numbering scheme.
> Similarly mdev's such port should be derived from mdev's id/name/attribute.
> 
> > > Longer UUID names are creating snow ball effect, not just in networking stack  
> > but many user space tools too.
> > 
> > This snowball effect mainly comes from the device name -> phys_port_name
> > setup, IIUC.
> >   
> Right.
> 
> > > (as opposed to recently introduced mdevctl, are they more mdev tools
> > > which has dependency on UUID name?)  
> > 
> > I am aware that people have written scripts etc. to manage their mdevs.
> > Given that the mdev infrastructure has been around for quite some time, I'd
> > say the chance of some of those scripts relying on uuid names is non-zero.
> >   
> Ok. but those scripts have never managed networking devices.
> So those scripts won't break because they will always create mdev devices using UUID.
> When they use these new networking devices, they need more things than their scripts.
> So user space upgrade for such mixed mode case is reasonable.

Tools like mdevctl are agnostic of the type of mdev device they're
managing, it shouldn't matter than they've never managed a networking
mdev previously, it follows the standards of mdev management.

> > >
> > > 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.

Note that these are kernel generated names, as are the other examples.
In the case of mdev, the user is providing the UUID, which becomes the
device name.  When a user writes to the create attribute, there needs
to be determinism that the user can identify the device they created vs
another that may have been created concurrently.  I don't see that we
can put users in the path of managing device instance numbers.

> > > 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.  
> > 
> > Actually, I think that second option makes much more sense, as you avoid
> > potentially breaking existing tooling.  
> Let's first understand of what exactly will break with existing tool
> if they see non_uuid based device.

Do we really want a mixed namespace of device names, some UUID, some...
something else?  That seems like a mess.

> Existing tooling continue to work with UUID devices.
> Do you have example of what can break if they see non_uuid based
> device name? I think you are clear, but to be sure, UUID based
> creation will continue to be there. Optionally mdev will be created
> with alpha-numeric string, if we don't it as additional attribute.

I'm not onboard with a UUID being just one of the possible naming
strings via which we can create mdev devices.  I think that becomes
untenable for userspace.  I don't think a sufficient argument has been
made against the alias approach, which seems to keep the UUID as a
canonical name, providing a consistent namespace, augmented with user
or kernel provided short alias.  Thanks,

Alex

> > >  
> > > > 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#L6
> > > 921
> > > [5] https://www.openvswitch.org/
> > >  
> 


^ permalink raw reply	[flat|nested] 38+ messages in thread

* RE: [PATCH v2 0/2] Simplify mtty driver and mdev core
  2019-08-14 14:57                         ` Alex Williamson
@ 2019-08-14 16:21                           ` Parav Pandit
  0 siblings, 0 replies; 38+ messages in thread
From: Parav Pandit @ 2019-08-14 16:21 UTC (permalink / raw)
  To: Alex Williamson
  Cc: Cornelia Huck, Kirti Wankhede, kvm, linux-kernel, cjia,
	Jiri Pirko, netdev



> -----Original Message-----
> From: Alex Williamson <alex.williamson@redhat.com>
> Sent: Wednesday, August 14, 2019 8:28 PM
> To: Parav Pandit <parav@mellanox.com>
> Cc: Cornelia Huck <cohuck@redhat.com>; Kirti Wankhede
> <kwankhede@nvidia.com>; kvm@vger.kernel.org; linux-
> kernel@vger.kernel.org; cjia@nvidia.com; Jiri Pirko <jiri@mellanox.com>;
> netdev@vger.kernel.org
> Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core
> 
> On Wed, 14 Aug 2019 13:45:49 +0000
> Parav Pandit <parav@mellanox.com> wrote:
> 
> > > -----Original Message-----
> > > From: Cornelia Huck <cohuck@redhat.com>
> > > Sent: Wednesday, August 14, 2019 6:39 PM
> > > To: Parav Pandit <parav@mellanox.com>
> > > Cc: Alex Williamson <alex.williamson@redhat.com>; Kirti Wankhede
> > > <kwankhede@nvidia.com>; kvm@vger.kernel.org; linux-
> > > kernel@vger.kernel.org; cjia@nvidia.com; Jiri Pirko
> > > <jiri@mellanox.com>; netdev@vger.kernel.org
> > > Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core
> > >
> > > On Wed, 14 Aug 2019 12:27:01 +0000
> > > Parav Pandit <parav@mellanox.com> 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 <cohuck@redhat.com>
> > > > > Sent: Wednesday, August 14, 2019 1:32 PM
> > > > > To: Parav Pandit <parav@mellanox.com>
> > > > > Cc: Alex Williamson <alex.williamson@redhat.com>; Kirti Wankhede
> > > > > <kwankhede@nvidia.com>; kvm@vger.kernel.org; linux-
> > > > > kernel@vger.kernel.org; cjia@nvidia.com
> > > > > Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core
> > > > >
> > > > > On Wed, 14 Aug 2019 05:54:36 +0000 Parav Pandit
> > > > > <parav@mellanox.com> 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 that userspace can refer to a representor netdevice. Makes sense.
> > > For generating that name, udev uses the phys_port_name (which
> > > represents the devlink port, IIUC). Also makes sense.
> > >
> > You understood it correctly.
> >
> > > >
> > > > phys_port_name has limitation to be only 15 characters long.
> > > > UUID doesn't fit in phys_port_name.
> > >
> > > Understood. But why do we need to derive the phys_port_name from the
> > > mdev device name? This netdevice use case seems to be just one use
> > > case for using mdev devices? If this is a specialized mdev type for
> > > this setup, why not just expose a shorter identifier via an extra attribute?
> > >
> > Representor netdev, represents mdev's switch port (like PCI SRIOV VF's switch
> port).
> > So user must be able to relate this two objects in similar manner as SRIOV
> VFs.
> > Phys_port_name is derived from the PCI PF and VF numbering scheme.
> > Similarly mdev's such port should be derived from mdev's id/name/attribute.
> >
> > > > Longer UUID names are creating snow ball effect, not just in
> > > > networking stack
> > > but many user space tools too.
> > >
> > > This snowball effect mainly comes from the device name ->
> > > phys_port_name setup, IIUC.
> > >
> > Right.
> >
> > > > (as opposed to recently introduced mdevctl, are they more mdev
> > > > tools which has dependency on UUID name?)
> > >
> > > I am aware that people have written scripts etc. to manage their mdevs.
> > > Given that the mdev infrastructure has been around for quite some
> > > time, I'd say the chance of some of those scripts relying on uuid names is
> non-zero.
> > >
> > Ok. but those scripts have never managed networking devices.
> > So those scripts won't break because they will always create mdev devices
> using UUID.
> > When they use these new networking devices, they need more things than
> their scripts.
> > So user space upgrade for such mixed mode case is reasonable.
> 
> Tools like mdevctl are agnostic of the type of mdev device they're managing, it
> shouldn't matter than they've never managed a networking mdev previously, it
> follows the standards of mdev management.
> 
> > > >
> > > > 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.
> 
> Note that these are kernel generated names, as are the other examples.
No. I probably gave the wrong examples.
Mdev user provided names can be 'foo', 'bar', 'foo1'.

> In the case of mdev, the user is providing the UUID, which becomes the device
> name.  When a user writes to the create attribute, there needs to be
> determinism that the user can identify the device they created vs another that
> may have been created concurrently.  I don't see that we can put users in the
> path of managing device instance numbers.
No. Its just user provided names.

> 
> > > > 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.
> > >
> > > Actually, I think that second option makes much more sense, as you
> > > avoid potentially breaking existing tooling.
> > Let's first understand of what exactly will break with existing tool
> > if they see non_uuid based device.
> 
> Do we really want a mixed namespace of device names, some UUID, some...
> something else?  That seems like a mess.
> 
So you prefer alias as an attribute? If so, it should be an optional additional parameter during create time, 
because it is desired to not invent new callbacks for such attributes setting and (and rewrite them).

> > Existing tooling continue to work with UUID devices.
> > Do you have example of what can break if they see non_uuid based
> > device name? I think you are clear, but to be sure, UUID based
> > creation will continue to be there. Optionally mdev will be created
> > with alpha-numeric string, if we don't it as additional attribute.
> 
> I'm not onboard with a UUID being just one of the possible naming strings via
> which we can create mdev devices.  I think that becomes untenable for
> userspace.  I don't think a sufficient argument has been made against the alias
> approach, which seems to keep the UUID as a canonical name, providing a
> consistent namespace, augmented with user or kernel provided short alias.
> Thanks,
> 
If I understand you correctly, you prefer alias name approach to keep UUID naming scheme intact in mdev?

> Alex
> 
> > > >
> > > > > 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/ether
> > > > net/
> > > > 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#L6
> > > > 921
> > > > [5] https://www.openvswitch.org/
> > > >
> >


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 2/2] vfio/mdev: Removed unused and redundant API for mdev UUID
  2019-08-08 14:12   ` [PATCH v2 2/2] vfio/mdev: Removed unused and redundant API for mdev UUID Parav Pandit
  2019-08-13 16:39     ` Christoph Hellwig
@ 2019-08-16 15:22     ` Cornelia Huck
  1 sibling, 0 replies; 38+ messages in thread
From: Cornelia Huck @ 2019-08-16 15:22 UTC (permalink / raw)
  To: Parav Pandit; +Cc: kvm, kwankhede, linux-kernel, alex.williamson, cjia

On Thu,  8 Aug 2019 09:12:55 -0500
Parav Pandit <parav@mellanox.com> wrote:

> There is no single production driver who is interested in mdev device
> uuid. Currently UUID is mainly used to derive a device name.
> Additionally mdev device name is already available using core kernel
> API dev_name().
> 
> Hence removed unused exported symbol.

FWIW, I just sent
https://lore.kernel.org/kvm/20190816151505.9853-1-cohuck@redhat.com/,
for which dev_name() is not an option.

> 
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> Signed-off-by: Parav Pandit <parav@mellanox.com>
> ---
> Changelog:
> v0->v1:
>  - Updated commit log to address comments from Cornelia
> ---
>  drivers/vfio/mdev/mdev_core.c | 6 ------
>  include/linux/mdev.h          | 1 -
>  2 files changed, 7 deletions(-)
> 
> diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
> index b558d4cfd082..c2b809cbe59f 100644
> --- a/drivers/vfio/mdev/mdev_core.c
> +++ b/drivers/vfio/mdev/mdev_core.c
> @@ -57,12 +57,6 @@ struct mdev_device *mdev_from_dev(struct device *dev)
>  }
>  EXPORT_SYMBOL(mdev_from_dev);
>  
> -const guid_t *mdev_uuid(struct mdev_device *mdev)
> -{
> -	return &mdev->uuid;
> -}
> -EXPORT_SYMBOL(mdev_uuid);
> -
>  /* Should be called holding parent_list_lock */
>  static struct mdev_parent *__find_parent_device(struct device *dev)
>  {
> diff --git a/include/linux/mdev.h b/include/linux/mdev.h
> index 0ce30ca78db0..375a5830c3d8 100644
> --- a/include/linux/mdev.h
> +++ b/include/linux/mdev.h
> @@ -131,7 +131,6 @@ struct mdev_driver {
>  
>  void *mdev_get_drvdata(struct mdev_device *mdev);
>  void mdev_set_drvdata(struct mdev_device *mdev, void *data);
> -const guid_t *mdev_uuid(struct mdev_device *mdev);
>  
>  extern struct bus_type mdev_bus_type;
>  


^ permalink raw reply	[flat|nested] 38+ messages in thread

end of thread, back to index

Thread overview: 38+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-02  6:59 [PATCH 0/2] Simplify mtty driver and mdev core Parav Pandit
2019-08-02  6:59 ` [PATCH 1/2] vfio-mdev/mtty: Simplify interrupt generation Parav Pandit
2019-08-06  8:15   ` Cornelia Huck
2019-08-02  6:59 ` [PATCH 2/2] vfio/mdev: Removed unused and redundant API for mdev name Parav Pandit
2019-08-06  8:29   ` Cornelia Huck
2019-08-06 13:12     ` Parav Pandit
2019-08-06 14:18 ` [PATCH v1 0/2] Simplify mtty driver and mdev core Parav Pandit
2019-08-06 14:18   ` [PATCH v1 1/2] vfio-mdev/mtty: Simplify interrupt generation Parav Pandit
2019-08-06 14:18   ` [PATCH v1 2/2] vfio/mdev: Removed unused and redundant API for mdev UUID Parav Pandit
2019-08-07  9:28     ` Cornelia Huck
2019-08-07 16:33       ` Parav Pandit
2019-08-08  8:29         ` Cornelia Huck
2019-08-08 14:01           ` Parav Pandit
2019-08-08 14:12 ` [PATCH v2 0/2] Simplify mtty driver and mdev core Parav Pandit
2019-08-08 14:12   ` [PATCH v2 1/2] vfio-mdev/mtty: Simplify interrupt generation Parav Pandit
2019-08-13 16:39     ` Christoph Hellwig
2019-08-08 14:12   ` [PATCH v2 2/2] vfio/mdev: Removed unused and redundant API for mdev UUID Parav Pandit
2019-08-13 16:39     ` Christoph Hellwig
2019-08-16 15:22     ` Cornelia Huck
2019-08-08 23:02   ` [PATCH v2 0/2] Simplify mtty driver and mdev core Alex Williamson
2019-08-09  8:07     ` Cornelia Huck
2019-08-12 11:35     ` Kirti Wankhede
2019-08-13 14:40       ` Parav Pandit
2019-08-13 14:52         ` Alex Williamson
2019-08-13 16:28           ` Parav Pandit
2019-08-13 16:34             ` Cornelia Huck
2019-08-13 17:11             ` Alex Williamson
2019-08-14  5:54               ` Parav Pandit
2019-08-14  8:01                 ` Cornelia Huck
2019-08-14 12:27                   ` Parav Pandit
2019-08-14 13:09                     ` Cornelia Huck
2019-08-14 13:45                       ` Parav Pandit
2019-08-14 14:57                         ` Alex Williamson
2019-08-14 16:21                           ` Parav Pandit
2019-08-13 16:37         ` Christoph Hellwig
2019-08-13 17:40           ` Greg Kroah-Hartman
2019-08-14  5:30             ` Parav Pandit
2019-08-13 14:48     ` Parav Pandit

KVM Archive on lore.kernel.org

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

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


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


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