Re: [PATCH] can: enable multi-queue for SocketCAN devices

Re: [PATCH] can: enable multi-queue for SocketCAN devices

[Jonas Mark (BT-FIR/ENG1)]

Hello Marc,

> > The existing SocketCAN implementation provides alloc_candev() to
> > allocate a CAN device using a single Tx and Rx queue. This can lead to
> > priority inversion in case the single Tx queue is already full with low
> > priority messages and a high priority message needs to be sent while the
> > bus is fully loaded with medium priority messages.
> >
> > This problem can be solved by using the existing multi-queue support of
> > the network subsystem. The commit makes it possible to use multi-queue in
> > the CAN subsystem in the same way it is used in the Ethernet subsystem
> > by adding an alloc_candev_mqs() call and accompanying macros. With this
> > support a CAN device can use multi-queue qdisc (e.g. mqprio) to avoid
> > the aforementioned priority inversion.
> >
> > The existing functionality of alloc_candev() is the same as before.
> >
> > CAN devices need to have prioritized multiple hardware queues or are
> > able to abort waiting for arbitration to make sensible use of
> > multi-queues.
> >
> > Signed-off-by: Zhu Yi <yi.zhu5@cn.bosch.com>
> > Signed-off-by: Mark Jonas <mark.jonas@de.bosch.com>
> > Reviewed-by: Heiko Schocher <hs@denx.de>
> 
> Do you have a driver or a patch to make a driver mq aware?

Yes, we have CAN hardware with multiple queues and we also have a SocketCAN driver for it.

IMHO the driver will be of very little use for the Linux community because the HW is proprietary.

Our motivation to separate this patch from the proprietary SocketCAN driver is that we felt it was odd that the multi-queue infrastructure is in place and only very few lines of code are missing to offer it to the CAN networking subsystem as well.

Greetings,
Mark

Re: [PATCH] can: enable multi-queue for SocketCAN devices

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1243 bytes --]

On 03/14/2018 01:26 PM, Jonas Mark (BT-FIR/ENG1) wrote:
>> Do you have a driver or a patch to make a driver mq aware?
> 
> Yes, we have CAN hardware with multiple queues and we also have a
> SocketCAN driver for it.
> 
> IMHO the driver will be of very little use for the Linux community
> because the HW is proprietary.

That doesn't matter. It would be the first driver that makes use of the
feature, so we can learn from it. And you might get a free review of
your driver.

> Our motivation to separate this patch from the proprietary SocketCAN
> driver

Looking at your email address I assume your employer sells devices with
this hardware and the driver. So someone has to provide the sources for
it anyways to fulfil the GPL license requirements. :)

> is that we felt it was odd that the multi-queue infrastructure is in
> place and only very few lines of code are missing to offer it to the
> CAN networking subsystem as well.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] can: enable multi-queue for SocketCAN devices

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1277 bytes --]

On 03/15/2018 10:08 AM, Jonas Mark (BT-FIR/ENG1) wrote:
>> Normal SPI?
> 
> Yes, normal SPI (CLK, MOSI, MISO, SEL) plus two GPIOs for flow control.
> The CAN peripheral (SPI slave) asserts the additional signals to
> request a transfer and to signal that it is busy.

Sounds interesting.

>>> I will have to check but I am pretty confident that we would be willing
>>> to publish the driver on these mailing lists as well. We will anyhow
>>> ship the source code (or a written offer) with every distribution.
>>
>> Sure - can I find your devices used and low priced on ebay :) ?
>
> That is rather unlikely. Though, you could buy a building containing
> one.

One building one device sounds like a bad deal to me :)

>>> If you are really serious about the review I will get the process
>>> rolling for an early publication of our driver.
>>
>> Yes, please go ahead!
> 
> Ok, I'll kick off the process and start to collect the required signatures.

Tnx.

regards,
Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] can: enable multi-queue for SocketCAN devices

[Jonas Mark (BT-FIR/ENG1)]

Hello Marc,

> > The driver consists of two layers because the HW is accessed via SPI.
> 
> Normal SPI?

Yes, normal SPI (CLK, MOSI, MISO, SEL) plus two GPIOs for flow control.
The CAN peripheral (SPI slave) asserts the additional signals to
request a transfer and to signal that it is busy.

> > I will have to check but I am pretty confident that we would be willing
> > to publish the driver on these mailing lists as well. We will anyhow
> > ship the source code (or a written offer) with every distribution.
> 
> Sure - can I find your devices used and low priced on ebay :) ?

That is rather unlikely. Though, you could buy a building containing
one.

> > If you are really serious about the review I will get the process
> > rolling for an early publication of our driver.
> 
> Yes, please go ahead!

Ok, I'll kick off the process and start to collect the required signatures.

Greetings,
Mark

Bosch Building Technologies, Panel Software Fire (ST-FIR/ENG1) 
Bosch Sicherheitssysteme GmbH | Postfach 11 11 | 85626 Grasbrunn | GERMANY | www.boschsecurity.com

Sitz: Stuttgart, Registergericht: Amtsgericht Stuttgart HRB 23118 
Aufsichtsratsvorsitzender: Stefan Hartung; Geschäftsführung: Gert van Iperen, Andreas Bartz, Thomas Quante, Bernhard Schuster

Re: [PATCH] can: enable multi-queue for SocketCAN devices

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1827 bytes --]

On 03/14/2018 05:51 PM, Jonas Mark (BT-FIR/ENG1) wrote:
>> That doesn't matter. It would be the first driver that makes use of the
>> feature, so we can learn from it. And you might get a free review of
>> your driver.
> 
> Of course we would be happy if somebody was volunteering to review
> our driver for free.

If my time permits, I'll throw some eyeballs on the driver.

> The driver consists of two layers because the HW is accessed via SPI.

Normal SPI?

> So there is a SPI driver of 1000 lines. On top of that is the CAN
> driver and it has 600 lines. Because the HW does more than just
> CAN there is another driver which implements a char device with
> additional 300 lines.
> 
> Are there still volunteers?

The SPI and the CAN driver would be the most interesting ones.

>>> Our motivation to separate this patch from the proprietary SocketCAN
>>> driver
>>
>> Looking at your email address I assume your employer sells devices with
>> this hardware and the driver. So someone has to provide the sources for
>> it anyways to fulfil the GPL license requirements. :)
> 
> I will have to check but I am pretty confident that we would be willing
> to publish the driver on these mailing lists as well. We will anyhow
> ship the source code (or a written offer) with every distribution.

Sure - can I find your devices used and low priced on ebay :) ?

> If you are really serious about the review I will get the process
> rolling for an early publication of our driver.

Yes, please go ahead!

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] can: enable multi-queue for SocketCAN devices

[Jonas Mark (BT-FIR/ENG1)]

Hello Marc,

> >> Do you have a driver or a patch to make a driver mq aware?
> >
> > Yes, we have CAN hardware with multiple queues and we also have a
> > SocketCAN driver for it.
> >
> > IMHO the driver will be of very little use for the Linux community
> > because the HW is proprietary.
> 
> That doesn't matter. It would be the first driver that makes use of the
> feature, so we can learn from it. And you might get a free review of
> your driver.

Of course we would be happy if somebody was volunteering to review
our driver for free.

The driver consists of two layers because the HW is accessed via SPI.
So there is a SPI driver of 1000 lines. On top of that is the CAN
driver and it has 600 lines. Because the HW does more than just
CAN there is another driver which implements a char device with
additional 300 lines.

Are there still volunteers?

> > Our motivation to separate this patch from the proprietary SocketCAN
> > driver
> 
> Looking at your email address I assume your employer sells devices with
> this hardware and the driver. So someone has to provide the sources for
> it anyways to fulfil the GPL license requirements. :)

I will have to check but I am pretty confident that we would be willing
to publish the driver on these mailing lists as well. We will anyhow
ship the source code (or a written offer) with every distribution.

If you are really serious about the review I will get the process
rolling for an early publication of our driver.

Greetings,
Mark

Bosch Building Technologies, Panel Software Fire (ST-FIR/ENG1) 
Bosch Sicherheitssysteme GmbH | Postfach 11 11 | 85626 Grasbrunn | GERMANY | www.boschsecurity.com 

Sitz: Stuttgart, Registergericht: Amtsgericht Stuttgart HRB 23118
Aufsichtsratsvorsitzender: Stefan Hartung; Geschäftsführung: Gert van Iperen, Andreas Bartz, Thomas Quante, Bernhard Schuster

Re: [PATCH] can: enable multi-queue for SocketCAN devices

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 3841 bytes --]

On 03/14/2018 11:33 AM, Mark Jonas wrote:
> From: Zhu Yi <yi.zhu5@cn.bosch.com>
> 
> The existing SocketCAN implementation provides alloc_candev() to
> allocate a CAN device using a single Tx and Rx queue. This can lead to
> priority inversion in case the single Tx queue is already full with low
> priority messages and a high priority message needs to be sent while the
> bus is fully loaded with medium priority messages.
> 
> This problem can be solved by using the existing multi-queue support of
> the network subsystem. The commit makes it possible to use multi-queue in
> the CAN subsystem in the same way it is used in the Ethernet subsystem
> by adding an alloc_candev_mqs() call and accompanying macros. With this
> support a CAN device can use multi-queue qdisc (e.g. mqprio) to avoid
> the aforementioned priority inversion.
> 
> The existing functionality of alloc_candev() is the same as before.
> 
> CAN devices need to have prioritized multiple hardware queues or are
> able to abort waiting for arbitration to make sensible use of
> multi-queues.
> 
> Signed-off-by: Zhu Yi <yi.zhu5@cn.bosch.com>
> Signed-off-by: Mark Jonas <mark.jonas@de.bosch.com>
> Reviewed-by: Heiko Schocher <hs@denx.de>

Do you have a driver or a patch to make a driver mq aware?

> ---
>  drivers/net/can/dev.c   | 8 +++++---
>  include/linux/can/dev.h | 7 ++++++-
>  2 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
> index b177956..636f853 100644
> --- a/drivers/net/can/dev.c
> +++ b/drivers/net/can/dev.c
> @@ -703,7 +703,8 @@ EXPORT_SYMBOL_GPL(alloc_can_err_skb);
>  /*
>   * Allocate and setup space for the CAN network device
>   */
> -struct net_device *alloc_candev(int sizeof_priv, unsigned int echo_skb_max)
> +struct net_device *alloc_candev_mqs(int sizeof_priv, unsigned int echo_skb_max,
> +				    unsigned int txqs, unsigned int rxqs)
>  {
>  	struct net_device *dev;
>  	struct can_priv *priv;
> @@ -715,7 +716,8 @@ struct net_device *alloc_candev(int sizeof_priv, unsigned int echo_skb_max)
>  	else
>  		size = sizeof_priv;
>  
> -	dev = alloc_netdev(size, "can%d", NET_NAME_UNKNOWN, can_setup);
> +	dev = alloc_netdev_mqs(size, "can%d", NET_NAME_UNKNOWN, can_setup,
> +			       txqs, rxqs);
>  	if (!dev)
>  		return NULL;
>  
> @@ -734,7 +736,7 @@ struct net_device *alloc_candev(int sizeof_priv, unsigned int echo_skb_max)
>  
>  	return dev;
>  }
> -EXPORT_SYMBOL_GPL(alloc_candev);
> +EXPORT_SYMBOL_GPL(alloc_candev_mqs);
>  
>  /*
>   * Free space of the CAN network device
> diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
> index 055aaf5..a83e1f6 100644
> --- a/include/linux/can/dev.h
> +++ b/include/linux/can/dev.h
> @@ -143,7 +143,12 @@ u8 can_dlc2len(u8 can_dlc);
>  /* map the sanitized data length to an appropriate data length code */
>  u8 can_len2dlc(u8 len);
>  
> -struct net_device *alloc_candev(int sizeof_priv, unsigned int echo_skb_max);
> +struct net_device *alloc_candev_mqs(int sizeof_priv, unsigned int echo_skb_max,
> +				    unsigned int txqs, unsigned int rxqs);
> +#define alloc_candev(sizeof_priv, echo_skb_max) \
> +	alloc_candev_mqs(sizeof_priv, echo_skb_max, 1, 1)
> +#define alloc_candev_mq(sizeof_priv, echo_skb_max, count) \
> +	alloc_candev_mqs(sizeof_priv, echo_skb_max, count, count)

Can you make this a static inline function.

>  void free_candev(struct net_device *dev);
>  
>  /* a candev safe wrapper around netdev_priv */
> 

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH] can: enable multi-queue for SocketCAN devices

[Mark Jonas]

From: Zhu Yi <yi.zhu5@cn.bosch.com>

The existing SocketCAN implementation provides alloc_candev() to
allocate a CAN device using a single Tx and Rx queue. This can lead to
priority inversion in case the single Tx queue is already full with low
priority messages and a high priority message needs to be sent while the
bus is fully loaded with medium priority messages.

This problem can be solved by using the existing multi-queue support of
the network subsystem. The commit makes it possible to use multi-queue in
the CAN subsystem in the same way it is used in the Ethernet subsystem
by adding an alloc_candev_mqs() call and accompanying macros. With this
support a CAN device can use multi-queue qdisc (e.g. mqprio) to avoid
the aforementioned priority inversion.

The existing functionality of alloc_candev() is the same as before.

CAN devices need to have prioritized multiple hardware queues or are
able to abort waiting for arbitration to make sensible use of
multi-queues.

Signed-off-by: Zhu Yi <yi.zhu5@cn.bosch.com>
Signed-off-by: Mark Jonas <mark.jonas@de.bosch.com>
Reviewed-by: Heiko Schocher <hs@denx.de>
---
 drivers/net/can/dev.c   | 8 +++++---
 include/linux/can/dev.h | 7 ++++++-
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index b177956..636f853 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -703,7 +703,8 @@ EXPORT_SYMBOL_GPL(alloc_can_err_skb);
 /*
  * Allocate and setup space for the CAN network device
  */
-struct net_device *alloc_candev(int sizeof_priv, unsigned int echo_skb_max)
+struct net_device *alloc_candev_mqs(int sizeof_priv, unsigned int echo_skb_max,
+				    unsigned int txqs, unsigned int rxqs)
 {
 	struct net_device *dev;
 	struct can_priv *priv;
@@ -715,7 +716,8 @@ struct net_device *alloc_candev(int sizeof_priv, unsigned int echo_skb_max)
 	else
 		size = sizeof_priv;
 
-	dev = alloc_netdev(size, "can%d", NET_NAME_UNKNOWN, can_setup);
+	dev = alloc_netdev_mqs(size, "can%d", NET_NAME_UNKNOWN, can_setup,
+			       txqs, rxqs);
 	if (!dev)
 		return NULL;
 
@@ -734,7 +736,7 @@ struct net_device *alloc_candev(int sizeof_priv, unsigned int echo_skb_max)
 
 	return dev;
 }
-EXPORT_SYMBOL_GPL(alloc_candev);
+EXPORT_SYMBOL_GPL(alloc_candev_mqs);
 
 /*
  * Free space of the CAN network device
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 055aaf5..a83e1f6 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -143,7 +143,12 @@ u8 can_dlc2len(u8 can_dlc);
 /* map the sanitized data length to an appropriate data length code */
 u8 can_len2dlc(u8 len);
 
-struct net_device *alloc_candev(int sizeof_priv, unsigned int echo_skb_max);
+struct net_device *alloc_candev_mqs(int sizeof_priv, unsigned int echo_skb_max,
+				    unsigned int txqs, unsigned int rxqs);
+#define alloc_candev(sizeof_priv, echo_skb_max) \
+	alloc_candev_mqs(sizeof_priv, echo_skb_max, 1, 1)
+#define alloc_candev_mq(sizeof_priv, echo_skb_max, count) \
+	alloc_candev_mqs(sizeof_priv, echo_skb_max, count, count)
 void free_candev(struct net_device *dev);
 
 /* a candev safe wrapper around netdev_priv */
-- 
2.7.4

[PATCH] can: enable multi-queue for SocketCAN devices

[Mark Jonas]

From: Zhu Yi <yi.zhu5@cn.bosch.com>

The existing SocketCAN implementation provides alloc_candev() to
allocate a CAN device using a single Tx and Rx queue. This can lead to
priority inversion in case the single Tx queue is already full with low
priority messages and a high priority message needs to be sent while the
bus is fully loaded with medium priority messages.

This problem can be solved by using the existing multi-queue support of
the network subsystem. The commit makes it possible to use multi-queue in
the CAN subsystem in the same way it is used in the Ethernet subsystem
by adding an alloc_candev_mqs() call and accompanying macros. With this
support a CAN device can use multi-queue qdisc (e.g. mqprio) to avoid
the aforementioned priority inversion.

The existing functionality of alloc_candev() is the same as before.

CAN devices need to have prioritized multiple hardware queues or are
able to abort waiting for arbitration to make sensible use of
multi-queues.

Signed-off-by: Zhu Yi <yi.zhu5@cn.bosch.com>
Signed-off-by: Mark Jonas <mark.jonas@de.bosch.com>
Reviewed-by: Heiko Schocher <hs@denx.de>
---
 drivers/net/can/dev.c   | 8 +++++---
 include/linux/can/dev.h | 7 ++++++-
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index b177956..636f853 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -703,7 +703,8 @@ EXPORT_SYMBOL_GPL(alloc_can_err_skb);
 /*
  * Allocate and setup space for the CAN network device
  */
-struct net_device *alloc_candev(int sizeof_priv, unsigned int echo_skb_max)
+struct net_device *alloc_candev_mqs(int sizeof_priv, unsigned int echo_skb_max,
+				    unsigned int txqs, unsigned int rxqs)
 {
 	struct net_device *dev;
 	struct can_priv *priv;
@@ -715,7 +716,8 @@ struct net_device *alloc_candev(int sizeof_priv, unsigned int echo_skb_max)
 	else
 		size = sizeof_priv;
 
-	dev = alloc_netdev(size, "can%d", NET_NAME_UNKNOWN, can_setup);
+	dev = alloc_netdev_mqs(size, "can%d", NET_NAME_UNKNOWN, can_setup,
+			       txqs, rxqs);
 	if (!dev)
 		return NULL;
 
@@ -734,7 +736,7 @@ struct net_device *alloc_candev(int sizeof_priv, unsigned int echo_skb_max)
 
 	return dev;
 }
-EXPORT_SYMBOL_GPL(alloc_candev);
+EXPORT_SYMBOL_GPL(alloc_candev_mqs);
 
 /*
  * Free space of the CAN network device
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 055aaf5..a83e1f6 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -143,7 +143,12 @@ u8 can_dlc2len(u8 can_dlc);
 /* map the sanitized data length to an appropriate data length code */
 u8 can_len2dlc(u8 len);
 
-struct net_device *alloc_candev(int sizeof_priv, unsigned int echo_skb_max);
+struct net_device *alloc_candev_mqs(int sizeof_priv, unsigned int echo_skb_max,
+				    unsigned int txqs, unsigned int rxqs);
+#define alloc_candev(sizeof_priv, echo_skb_max) \
+	alloc_candev_mqs(sizeof_priv, echo_skb_max, 1, 1)
+#define alloc_candev_mq(sizeof_priv, echo_skb_max, count) \
+	alloc_candev_mqs(sizeof_priv, echo_skb_max, count, count)
 void free_candev(struct net_device *dev);
 
 /* a candev safe wrapper around netdev_priv */
-- 
2.7.4