LinuxLists.cc - [PATCH net 0/3] SLCAN/SLIP fixes and performance

[permalink] [raw]

Subject: [PATCH net 2/3] lib: introduce upper case hex ascii helpers

To be able to use the hex ascii functions in case sensitive environments
the array hex_asc_upper[] and the needed functions for hex_byte_pack_upper()
are introduced.

Signed-off-by: Andre Naujoks <[email protected]>
---
include/linux/kernel.h | 11 +++++++++++
lib/hexdump.c | 2 ++
2 files changed, 13 insertions(+)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 482ad2d..672ddc4 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -439,6 +439,17 @@ static inline char *hex_byte_pack(char *buf, u8 byte)
return buf;
}

+extern const char hex_asc_upper[];
+#define hex_asc_upper_lo(x) hex_asc_upper[((x) & 0x0f)]
+#define hex_asc_upper_hi(x) hex_asc_upper[((x) & 0xf0) >> 4]
+
+static inline char *hex_byte_pack_upper(char *buf, u8 byte)
+{
+ *buf++ = hex_asc_upper_hi(byte);
+ *buf++ = hex_asc_upper_lo(byte);
+ return buf;
+}
+
static inline char * __deprecated pack_hex_byte(char *buf, u8 byte)
{
return hex_byte_pack(buf, byte);
diff --git a/lib/hexdump.c b/lib/hexdump.c
index 3f0494c..8499c81 100644
--- a/lib/hexdump.c
+++ b/lib/hexdump.c
@@ -14,6 +14,8 @@

const char hex_asc[] = "0123456789abcdef";
EXPORT_SYMBOL(hex_asc);
+const char hex_asc_upper[] = "0123456789ABCDEF";
+EXPORT_SYMBOL(hex_asc_upper);

/**
* hex_to_bin - convert a hex digit to its real value
--
1.8.4.rc3

2013-09-13 17:38:00

[permalink] [raw]

Subject: [PATCH net 3/3] slcan: rewrite of slc_bump and slc_encaps

The old implementation was heavy on str* functions and sprintf calls.
This version is more manual, but faster.

Profiling just the printing of a 3 char CAN-id resulted in 60 instructions
for the manual method and over 2000 for the sprintf method. Bear in
mind the profiling was done against libc and not the kernel sprintf.

Together with this rewrite an issue with sending and receiving of RTR frames
has been fixed by Oliver for the cases that the DLC is not zero.

Signed-off-by: Andre Naujoks <[email protected]>
Tested-by: Oliver Hartkopp <[email protected]>
---
drivers/net/can/slcan.c | 136 +++++++++++++++++++++++++++++++-----------------
1 file changed, 87 insertions(+), 49 deletions(-)

diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
index d571e2e..25377e5 100644
--- a/drivers/net/can/slcan.c
+++ b/drivers/net/can/slcan.c
@@ -76,6 +76,10 @@ MODULE_PARM_DESC(maxdev, "Maximum number of slcan interfaces");
/* maximum rx buffer len: extended CAN frame with timestamp */
#define SLC_MTU (sizeof("T1111222281122334455667788EA5F\r")+1)

+#define SLC_CMD_LEN 1
+#define SLC_SFF_ID_LEN 3
+#define SLC_EFF_ID_LEN 8
+
struct slcan {
int magic;

@@ -142,47 +146,63 @@ static void slc_bump(struct slcan *sl)
{
struct sk_buff *skb;
struct can_frame cf;
- int i, dlc_pos, tmp;
- unsigned long ultmp;
- char cmd = sl->rbuff[0];
-
- if ((cmd != 't') && (cmd != 'T') && (cmd != 'r') && (cmd != 'R'))
+ int i, tmp;
+ u32 tmpid;
+ char *cmd = sl->rbuff;
+
+ cf.can_id = 0;
+
+ switch (*cmd) {
+ case 'r':
+ cf.can_id = CAN_RTR_FLAG;
+ /* fallthrough */
+ case 't':
+ /* store dlc ASCII value and terminate SFF CAN ID string */
+ cf.can_dlc = sl->rbuff[SLC_CMD_LEN + SLC_SFF_ID_LEN];
+ sl->rbuff[SLC_CMD_LEN + SLC_SFF_ID_LEN] = 0;
+ /* point to payload data behind the dlc */
+ cmd += SLC_CMD_LEN + SLC_SFF_ID_LEN + 1;
+ break;
+ case 'R':
+ cf.can_id = CAN_RTR_FLAG;
+ /* fallthrough */
+ case 'T':
+ cf.can_id |= CAN_EFF_FLAG;
+ /* store dlc ASCII value and terminate EFF CAN ID string */
+ cf.can_dlc = sl->rbuff[SLC_CMD_LEN + SLC_EFF_ID_LEN];
+ sl->rbuff[SLC_CMD_LEN + SLC_EFF_ID_LEN] = 0;
+ /* point to payload data behind the dlc */
+ cmd += SLC_CMD_LEN + SLC_EFF_ID_LEN + 1;
+ break;
+ default:
return;
+ }

- if (cmd & 0x20) /* tiny chars 'r' 't' => standard frame format */
- dlc_pos = 4; /* dlc position tiiid */
- else
- dlc_pos = 9; /* dlc position Tiiiiiiiid */
-
- if (!((sl->rbuff[dlc_pos] >= '0') && (sl->rbuff[dlc_pos] < '9')))
+ if (kstrtou32(sl->rbuff + SLC_CMD_LEN, 16, &tmpid))
return;

- cf.can_dlc = sl->rbuff[dlc_pos] - '0'; /* get can_dlc from ASCII val */
+ cf.can_id |= tmpid;

- sl->rbuff[dlc_pos] = 0; /* terminate can_id string */
-
- if (kstrtoul(sl->rbuff+1, 16, &ultmp))
+ /* get can_dlc from sanitized ASCII value */
+ if (cf.can_dlc >= '0' && cf.can_dlc < '9')
+ cf.can_dlc -= '0';
+ else
return;

- cf.can_id = ultmp;
-
- if (!(cmd & 0x20)) /* NO tiny chars => extended frame format */
- cf.can_id |= CAN_EFF_FLAG;
-
- if ((cmd | 0x20) == 'r') /* RTR frame */
- cf.can_id |= CAN_RTR_FLAG;
-
*(u64 *) (&cf.data) = 0; /* clear payload */

- for (i = 0, dlc_pos++; i < cf.can_dlc; i++) {
- tmp = hex_to_bin(sl->rbuff[dlc_pos++]);
- if (tmp < 0)
- return;
- cf.data[i] = (tmp << 4);
- tmp = hex_to_bin(sl->rbuff[dlc_pos++]);
- if (tmp < 0)
- return;
- cf.data[i] |= tmp;
+ /* RTR frames may have a dlc > 0 but they never have any data bytes */
+ if (!(cf.can_id & CAN_RTR_FLAG)) {
+ for (i = 0; i < cf.can_dlc; i++) {
+ tmp = hex_to_bin(*cmd++);
+ if (tmp < 0)
+ return;
+ cf.data[i] = (tmp << 4);
+ tmp = hex_to_bin(*cmd++);
+ if (tmp < 0)
+ return;
+ cf.data[i] |= tmp;
+ }
}

skb = dev_alloc_skb(sizeof(struct can_frame) +
@@ -209,7 +229,6 @@ static void slc_bump(struct slcan *sl)
/* parse tty input stream */
static void slcan_unesc(struct slcan *sl, unsigned char s)
{
-
if ((s == '\r') || (s == '\a')) { /* CR or BEL ends the pdu */
if (!test_and_clear_bit(SLF_ERROR, &sl->flags) &&
(sl->rcount > 4)) {
@@ -236,27 +255,46 @@ static void slcan_unesc(struct slcan *sl, unsigned char s)
/* Encapsulate one can_frame and stuff into a TTY queue. */
static void slc_encaps(struct slcan *sl, struct can_frame *cf)
{
- int actual, idx, i;
- char cmd;
+ int actual, i;
+ unsigned char *pos;
+ unsigned char *endpos;
+ canid_t id = cf->can_id;
+
+ pos = sl->xbuff;

if (cf->can_id & CAN_RTR_FLAG)
- cmd = 'R'; /* becomes 'r' in standard frame format */
+ *pos = 'R'; /* becomes 'r' in standard frame format (SFF) */
else
- cmd = 'T'; /* becomes 't' in standard frame format */
+ *pos = 'T'; /* becomes 't' in standard frame format (SSF) */

- if (cf->can_id & CAN_EFF_FLAG)
- sprintf(sl->xbuff, "%c%08X%d", cmd,
- cf->can_id & CAN_EFF_MASK, cf->can_dlc);
- else
- sprintf(sl->xbuff, "%c%03X%d", cmd | 0x20,
- cf->can_id & CAN_SFF_MASK, cf->can_dlc);
+ /* determine number of chars for the CAN-identifier */
+ if (cf->can_id & CAN_EFF_FLAG) {
+ id &= CAN_EFF_MASK;
+ endpos = pos + SLC_EFF_ID_LEN;
+ } else {
+ *pos |= 0x20; /* convert R/T to lower case for SFF */
+ id &= CAN_SFF_MASK;
+ endpos = pos + SLC_SFF_ID_LEN;
+ }
+
+ /* build 3 (SFF) or 8 (EFF) digit CAN identifier */
+ pos++;
+ while (endpos >= pos) {
+ *endpos-- = hex_asc_upper[id & 0xf];
+ id >>= 4;
+ }
+
+ pos += (cf->can_id & CAN_EFF_FLAG) ? SLC_EFF_ID_LEN : SLC_SFF_ID_LEN;

- idx = strlen(sl->xbuff);
+ *pos++ = cf->can_dlc + '0';

- for (i = 0; i < cf->can_dlc; i++)
- sprintf(&sl->xbuff[idx + 2*i], "%02X", cf->data[i]);
+ /* RTR frames may have a dlc > 0 but they never have any data bytes */
+ if (!(cf->can_id & CAN_RTR_FLAG)) {
+ for (i = 0; i < cf->can_dlc; i++)
+ pos = hex_byte_pack_upper(pos, cf->data[i]);
+ }

- strcat(sl->xbuff, "\r"); /* add terminating character */
+ *pos++ = '\r';

/* Order of next two lines is *very* important.
* When we are sending a little amount of data,
@@ -267,8 +305,8 @@ static void slc_encaps(struct slcan *sl, struct can_frame *cf)
* 14 Oct 1994 Dmitry Gorodchanin.
*/
set_bit(TTY_DO_WRITE_WAKEUP, &sl->tty->flags);
- actual = sl->tty->ops->write(sl->tty, sl->xbuff, strlen(sl->xbuff));
- sl->xleft = strlen(sl->xbuff) - actual;
+ actual = sl->tty->ops->write(sl->tty, sl->xbuff, pos - sl->xbuff);
+ sl->xleft = (pos - sl->xbuff) - actual;
sl->xhead = sl->xbuff + actual;
sl->dev->stats.tx_bytes += cf->can_dlc;
}
--
1.8.4.rc3

2013-09-13 17:37:25

[permalink] [raw]

Subject: [PATCH net 1/3] slip/slcan: added locking in wakeup function

The locking is needed, since the the internal buffer for the CAN frames is
changed during the wakeup call. This could cause buffer inconsistencies
under high loads, especially for the outgoing short CAN packet skbuffs.

The needed locks led to deadlocks before commit
"5ede52538ee2b2202d9dff5b06c33bfde421e6e4 tty: Remove extra wakeup from pty
write() path", which removed the direct callback to the wakeup function from the
tty layer.

As slcan.c is based on slip.c the issue in the original code is fixed, too.

Signed-off-by: Andre Naujoks <[email protected]>
---
drivers/net/can/slcan.c | 3 +++
drivers/net/slip/slip.c | 3 +++
2 files changed, 6 insertions(+)

diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
index 874188b..d571e2e 100644
--- a/drivers/net/can/slcan.c
+++ b/drivers/net/can/slcan.c
@@ -286,11 +286,13 @@ static void slcan_write_wakeup(struct tty_struct *tty)
if (!sl || sl->magic != SLCAN_MAGIC || !netif_running(sl->dev))
return;

+ spin_lock(&sl->lock);
if (sl->xleft <= 0) {
/* Now serial buffer is almost free & we can start
* transmission of another packet */
sl->dev->stats.tx_packets++;
clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
+ spin_unlock(&sl->lock);
netif_wake_queue(sl->dev);
return;
}
@@ -298,6 +300,7 @@ static void slcan_write_wakeup(struct tty_struct *tty)
actual = tty->ops->write(tty, sl->xhead, sl->xleft);
sl->xleft -= actual;
sl->xhead += actual;
+ spin_unlock(&sl->lock);
}

/* Send a can_frame to a TTY queue. */
diff --git a/drivers/net/slip/slip.c b/drivers/net/slip/slip.c
index a34d6bf..cc70ecf 100644
--- a/drivers/net/slip/slip.c
+++ b/drivers/net/slip/slip.c
@@ -429,11 +429,13 @@ static void slip_write_wakeup(struct tty_struct *tty)
if (!sl || sl->magic != SLIP_MAGIC || !netif_running(sl->dev))
return;

+ spin_lock(&sl->lock);
if (sl->xleft <= 0) {
/* Now serial buffer is almost free & we can start
* transmission of another packet */
sl->dev->stats.tx_packets++;
clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
+ spin_unlock(&sl->lock);
sl_unlock(sl);
return;
}
@@ -441,6 +443,7 @@ static void slip_write_wakeup(struct tty_struct *tty)
actual = tty->ops->write(tty, sl->xhead, sl->xleft);
sl->xleft -= actual;
sl->xhead += actual;
+ spin_unlock(&sl->lock);
}

static void sl_tx_timeout(struct net_device *dev)
--
1.8.4.rc3

2013-09-13 18:43:19

by Oliver Hartkopp

[permalink] [raw]

Subject: Re: [PATCH net 3/3] slcan: rewrite of slc_bump and slc_encaps

On 13.09.2013 19:37, Andre Naujoks wrote:
> The old implementation was heavy on str* functions and sprintf calls.
> This version is more manual, but faster.
>
> Profiling just the printing of a 3 char CAN-id resulted in 60 instructions
> for the manual method and over 2000 for the sprintf method. Bear in
> mind the profiling was done against libc and not the kernel sprintf.
>
> Together with this rewrite an issue with sending and receiving of RTR frames
> has been fixed by Oliver for the cases that the DLC is not zero.
>
> Signed-off-by: Andre Naujoks <[email protected]>
> Tested-by: Oliver Hartkopp <[email protected]>
> ---
> drivers/net/can/slcan.c | 136 +++++++++++++++++++++++++++++++-----------------
> 1 file changed, 87 insertions(+), 49 deletions(-)

As the layout of the generated content is fixed the provided flexibility of
the used string functions was indeed inadequate.

Thanks for the effort, Andre.

Acked-by: Oliver Hartkopp <[email protected]>

2013-09-13 18:45:33

by Oliver Hartkopp

[permalink] [raw]

Subject: Re: [PATCH net 1/3] slip/slcan: added locking in wakeup function

On 13.09.2013 19:37, Andre Naujoks wrote:
> The locking is needed, since the the internal buffer for the CAN frames is
> changed during the wakeup call. This could cause buffer inconsistencies
> under high loads, especially for the outgoing short CAN packet skbuffs.
>
> The needed locks led to deadlocks before commit
> "5ede52538ee2b2202d9dff5b06c33bfde421e6e4 tty: Remove extra wakeup from pty
> write() path", which removed the direct callback to the wakeup function from the
> tty layer.
>
> As slcan.c is based on slip.c the issue in the original code is fixed, too.
>
> Signed-off-by: Andre Naujoks <[email protected]>

At least for slcan.c:

Acked-by: Oliver Hartkopp <[email protected]>

Tnx for figuring that out with your heavy load testing.

Best regards,
Oliver

> ---
> drivers/net/can/slcan.c | 3 +++
> drivers/net/slip/slip.c | 3 +++
> 2 files changed, 6 insertions(+)
>
> diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
> index 874188b..d571e2e 100644
> --- a/drivers/net/can/slcan.c
> +++ b/drivers/net/can/slcan.c
> @@ -286,11 +286,13 @@ static void slcan_write_wakeup(struct tty_struct *tty)
> if (!sl || sl->magic != SLCAN_MAGIC || !netif_running(sl->dev))
> return;
>
> + spin_lock(&sl->lock);
> if (sl->xleft <= 0) {
> /* Now serial buffer is almost free & we can start
> * transmission of another packet */
> sl->dev->stats.tx_packets++;
> clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
> + spin_unlock(&sl->lock);
> netif_wake_queue(sl->dev);
> return;
> }
> @@ -298,6 +300,7 @@ static void slcan_write_wakeup(struct tty_struct *tty)
> actual = tty->ops->write(tty, sl->xhead, sl->xleft);
> sl->xleft -= actual;
> sl->xhead += actual;
> + spin_unlock(&sl->lock);
> }
>
> /* Send a can_frame to a TTY queue. */
> diff --git a/drivers/net/slip/slip.c b/drivers/net/slip/slip.c
> index a34d6bf..cc70ecf 100644
> --- a/drivers/net/slip/slip.c
> +++ b/drivers/net/slip/slip.c
> @@ -429,11 +429,13 @@ static void slip_write_wakeup(struct tty_struct *tty)
> if (!sl || sl->magic != SLIP_MAGIC || !netif_running(sl->dev))
> return;
>
> + spin_lock(&sl->lock);
> if (sl->xleft <= 0) {
> /* Now serial buffer is almost free & we can start
> * transmission of another packet */
> sl->dev->stats.tx_packets++;
> clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
> + spin_unlock(&sl->lock);
> sl_unlock(sl);
> return;
> }
> @@ -441,6 +443,7 @@ static void slip_write_wakeup(struct tty_struct *tty)
> actual = tty->ops->write(tty, sl->xhead, sl->xleft);
> sl->xleft -= actual;
> sl->xhead += actual;
> + spin_unlock(&sl->lock);
> }
>
> static void sl_tx_timeout(struct net_device *dev)
>

2013-09-14 10:46:16

[permalink] [raw]

Subject: Re: [PATCH net 0/3] SLCAN/SLIP fixes and performance

On 09/13/2013 07:37 PM, Andre Naujoks wrote:
> Hi Dave,
>
> these are some loosely related patches, that fix an ancient locking problem in
> the slip and slcan drivers, add general ASCII-HEX to bin functions for
> uppercase ASCII, fix the handling of CAN RTR frames in the slcan driver

Can you get an Acked-by for the ASCII-HEX functions from the appropriate
maintainer?

> and increase the performance for the slcan driver.
>
> As these patches mainly contain fixes for the slip/slcan drivers that require
> a tty layer fix included in 3.11, I would suggest to get the patches in via
> the net tree for the 3.12 cycle. They should apply properly on the latest net
> and mainline tree.

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 |

Attachments:

2013-09-14 11:22:20

[permalink] [raw]

Subject: Re: [PATCH net 0/3] SLCAN/SLIP fixes and performance

On 14.09.2013 12:45, Marc Kleine-Budde wrote:
> On 09/13/2013 07:37 PM, Andre Naujoks wrote:
>> Hi Dave,
>>
>> these are some loosely related patches, that fix an ancient locking problem in
>> the slip and slcan drivers, add general ASCII-HEX to bin functions for
>> uppercase ASCII, fix the handling of CAN RTR frames in the slcan driver
>
> Can you get an Acked-by for the ASCII-HEX functions from the appropriate
> maintainer?

The patch went out to the maintainers I got from the get_maintainer.pl
script. Is there anything else I can or should do to get an Ack from them?

Regards
Andre

>
>> and increase the performance for the slcan driver.
>>
>> As these patches mainly contain fixes for the slip/slcan drivers that require
>> a tty layer fix included in 3.11, I would suggest to get the patches in via
>> the net tree for the 3.12 cycle. They should apply properly on the latest net
>> and mainline tree.
>
> Marc
>

2013-09-15 04:27:10

by Thiago Farina

[permalink] [raw]

Subject: Re: [PATCH net 2/3] lib: introduce upper case hex ascii helpers

On Fri, Sep 13, 2013 at 2:37 PM, Andre Naujoks <[email protected]> wrote:
> To be able to use the hex ascii functions in case sensitive environments
> the array hex_asc_upper[] and the needed functions for hex_byte_pack_upper()
> are introduced.
>
> Signed-off-by: Andre Naujoks <[email protected]>
> ---
> include/linux/kernel.h | 11 +++++++++++
> lib/hexdump.c | 2 ++
> 2 files changed, 13 insertions(+)
>
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 482ad2d..672ddc4 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -439,6 +439,17 @@ static inline char *hex_byte_pack(char *buf, u8 byte)
> return buf;
> }
>
> +extern const char hex_asc_upper[];
> +#define hex_asc_upper_lo(x) hex_asc_upper[((x) & 0x0f)]
> +#define hex_asc_upper_hi(x) hex_asc_upper[((x) & 0xf0) >> 4]
Does using a macro instead of a real function (static inline)
generates a better code?

--
Thiago Farina

2013-09-15 04:35:37

by Andrew Morton

[permalink] [raw]

Subject: Re: [PATCH net 2/3] lib: introduce upper case hex ascii helpers

On Sun, 15 Sep 2013 01:27:03 -0300 Thiago Farina <[email protected]> wrote:

> On Fri, Sep 13, 2013 at 2:37 PM, Andre Naujoks <[email protected]> wrote:
> > To be able to use the hex ascii functions in case sensitive environments
> > the array hex_asc_upper[] and the needed functions for hex_byte_pack_upper()
> > are introduced.
> >
> > Signed-off-by: Andre Naujoks <[email protected]>
> > ---
> > include/linux/kernel.h | 11 +++++++++++
> > lib/hexdump.c | 2 ++
> > 2 files changed, 13 insertions(+)
> >
> > diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> > index 482ad2d..672ddc4 100644
> > --- a/include/linux/kernel.h
> > +++ b/include/linux/kernel.h
> > @@ -439,6 +439,17 @@ static inline char *hex_byte_pack(char *buf, u8 byte)
> > return buf;
> > }
> >
> > +extern const char hex_asc_upper[];
> > +#define hex_asc_upper_lo(x) hex_asc_upper[((x) & 0x0f)]
> > +#define hex_asc_upper_hi(x) hex_asc_upper[((x) & 0xf0) >> 4]
> Does using a macro instead of a real function (static inline)
> generates a better code?

Yes, a static inline would be nicer, but these are derived from
hex_asc_lo/hex_asc_hi. If we change one we should change the other
and that becomes a separate cleanup. So I think this patch is
OK as-is.

Also, it would make sense to get all the *hex* stuff out of kernel.h
and into a new header file (hexchar.h?). They're a clean
self-contained thing and kernel.h is rather a dumping ground.

2013-09-19 09:36:15

[permalink] [raw]

Subject: Re: [PATCH net 1/3] slip/slcan: added locking in wakeup function

On 09/13/2013 07:37 PM, Andre Naujoks wrote:
> The locking is needed, since the the internal buffer for the CAN frames is
> changed during the wakeup call. This could cause buffer inconsistencies
> under high loads, especially for the outgoing short CAN packet skbuffs.
>
> The needed locks led to deadlocks before commit
> "5ede52538ee2b2202d9dff5b06c33bfde421e6e4 tty: Remove extra wakeup from pty
> write() path", which removed the direct callback to the wakeup function from the
> tty layer.

What does that mean for older kernels?
(< 5ede52538ee2b2202d9dff5b06c33bfde421e6e4)

> As slcan.c is based on slip.c the issue in the original code is fixed, too.
>
> Signed-off-by: Andre Naujoks <[email protected]>
Acked-by: Marc Kleine-Budde <[email protected]>

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 |

Attachments:

2013-09-19 09:38:50

[permalink] [raw]

Subject: Re: [PATCH net 2/3] lib: introduce upper case hex ascii helpers

On 09/15/2013 06:35 AM, Andrew Morton wrote:
> On Sun, 15 Sep 2013 01:27:03 -0300 Thiago Farina <[email protected]> wrote:
>
>> On Fri, Sep 13, 2013 at 2:37 PM, Andre Naujoks <[email protected]> wrote:
>>> To be able to use the hex ascii functions in case sensitive environments
>>> the array hex_asc_upper[] and the needed functions for hex_byte_pack_upper()
>>> are introduced.
>>>
>>> Signed-off-by: Andre Naujoks <[email protected]>
>>> ---
>>> include/linux/kernel.h | 11 +++++++++++
>>> lib/hexdump.c | 2 ++
>>> 2 files changed, 13 insertions(+)
>>>
>>> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
>>> index 482ad2d..672ddc4 100644
>>> --- a/include/linux/kernel.h
>>> +++ b/include/linux/kernel.h
>>> @@ -439,6 +439,17 @@ static inline char *hex_byte_pack(char *buf, u8 byte)
>>> return buf;
>>> }
>>>
>>> +extern const char hex_asc_upper[];
>>> +#define hex_asc_upper_lo(x) hex_asc_upper[((x) & 0x0f)]
>>> +#define hex_asc_upper_hi(x) hex_asc_upper[((x) & 0xf0) >> 4]
>> Does using a macro instead of a real function (static inline)
>> generates a better code?
>
> Yes, a static inline would be nicer, but these are derived from
> hex_asc_lo/hex_asc_hi. If we change one we should change the other
> and that becomes a separate cleanup. So I think this patch is
> OK as-is.

Is this an Acked-by?

> Also, it would make sense to get all the *hex* stuff out of kernel.h
> and into a new header file (hexchar.h?). They're a clean
> self-contained thing and kernel.h is rather a dumping ground.

Who is taking this series?

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 |

Attachments:

2013-09-19 09:48:09

[permalink] [raw]

Subject: Re: [PATCH net 3/3] slcan: rewrite of slc_bump and slc_encaps

On 09/13/2013 07:37 PM, Andre Naujoks wrote:
> The old implementation was heavy on str* functions and sprintf calls.
> This version is more manual, but faster.
>
> Profiling just the printing of a 3 char CAN-id resulted in 60 instructions
> for the manual method and over 2000 for the sprintf method. Bear in
> mind the profiling was done against libc and not the kernel sprintf.
>
> Together with this rewrite an issue with sending and receiving of RTR frames
> has been fixed by Oliver for the cases that the DLC is not zero.
>
> Signed-off-by: Andre Naujoks <[email protected]>
> Tested-by: Oliver Hartkopp <[email protected]>
Acked-by: Marc Kleine-Budde <[email protected]>

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 |

Attachments:

2013-09-19 09:57:44

by Oliver Hartkopp

[permalink] [raw]

Subject: Re: [PATCH net 2/3] lib: introduce upper case hex ascii helpers

On 19.09.2013 11:38, Marc Kleine-Budde wrote:
> On 09/15/2013 06:35 AM, Andrew Morton wrote:
>> On Sun, 15 Sep 2013 01:27:03 -0300 Thiago Farina <[email protected]> wrote:
>>
>>> On Fri, Sep 13, 2013 at 2:37 PM, Andre Naujoks <[email protected]> wrote:
>>>> To be able to use the hex ascii functions in case sensitive environments
>>>> the array hex_asc_upper[] and the needed functions for hex_byte_pack_upper()
>>>> are introduced.
>>>>
>>>> Signed-off-by: Andre Naujoks <[email protected]>
>>>> ---
>>>> include/linux/kernel.h | 11 +++++++++++
>>>> lib/hexdump.c | 2 ++
>>>> 2 files changed, 13 insertions(+)
>>>>
>>>> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
>>>> index 482ad2d..672ddc4 100644
>>>> --- a/include/linux/kernel.h
>>>> +++ b/include/linux/kernel.h
>>>> @@ -439,6 +439,17 @@ static inline char *hex_byte_pack(char *buf, u8 byte)
>>>> return buf;
>>>> }
>>>>
>>>> +extern const char hex_asc_upper[];
>>>> +#define hex_asc_upper_lo(x) hex_asc_upper[((x) & 0x0f)]
>>>> +#define hex_asc_upper_hi(x) hex_asc_upper[((x) & 0xf0) >> 4]
>>> Does using a macro instead of a real function (static inline)
>>> generates a better code?
>>
>> Yes, a static inline would be nicer, but these are derived from
>> hex_asc_lo/hex_asc_hi. If we change one we should change the other
>> and that becomes a separate cleanup. So I think this patch is
>> OK as-is.
>
> Is this an Acked-by?
>
>> Also, it would make sense to get all the *hex* stuff out of kernel.h
>> and into a new header file (hexchar.h?). They're a clean
>> self-contained thing and kernel.h is rather a dumping ground.
>
> Who is taking this series?

Andre suggested that Dave Miller could take these three patches for 3.12 as
they are mainly network fixes:

http://marc.info/?l=linux-can&m=137909384116115&w=2

Regards,
Oliver

2013-09-19 10:29:54

[permalink] [raw]

Subject: Re: [PATCH net 1/3] slip/slcan: added locking in wakeup function

On 19.09.2013 11:36, schrieb Marc Kleine-Budde:
> On 09/13/2013 07:37 PM, Andre Naujoks wrote:
>> The locking is needed, since the the internal buffer for the CAN
>> frames is changed during the wakeup call. This could cause buffer
>> inconsistencies under high loads, especially for the outgoing
>> short CAN packet skbuffs.
>>
>> The needed locks led to deadlocks before commit
>> "5ede52538ee2b2202d9dff5b06c33bfde421e6e4 tty: Remove extra
>> wakeup from pty write() path", which removed the direct callback
>> to the wakeup function from the tty layer.
>
> What does that mean for older kernels? (<
> 5ede52538ee2b2202d9dff5b06c33bfde421e6e4)

It seems the slcan (and slip) driver is broken for older kernels. See
this thread for a discussion about the patch in pty.c.

http://marc.info/?l=linux-kernel&m=137269017002789&w=2

The patch from Peter Hurley was actually already in the queue, when I
ran into the problem, and is now in kernel 3.12.

Without the pty patch and slow CAN traffic, the driver works, because
the wakeup is called directly from the pty driver. That is also the
reason why there was no locking. It would just deadlock.

When the pty driver defers the wakeup, we ran into synchronisation
problems (which should be fixed by the locking) and eventually into a
kernel panic because of a recursive loop (which should be fixed by the
pty.c patch).

Maybe it is possible to get both patches back into the stable branches?

Regards
Andre

>
>> As slcan.c is based on slip.c the issue in the original code is
>> fixed, too.
>>
>> Signed-off-by: Andre Naujoks <[email protected]>
> Acked-by: Marc Kleine-Budde <[email protected]>
>
> Marc
>

2013-09-19 10:35:13

[permalink] [raw]

Subject: Re: [PATCH net 1/3] slip/slcan: added locking in wakeup function

On 09/19/2013 12:29 PM, Andre Naujoks wrote:
> On 19.09.2013 11:36, schrieb Marc Kleine-Budde:
>> On 09/13/2013 07:37 PM, Andre Naujoks wrote:
>>> The locking is needed, since the the internal buffer for the CAN
>>> frames is changed during the wakeup call. This could cause buffer
>>> inconsistencies under high loads, especially for the outgoing
>>> short CAN packet skbuffs.
>>>
>>> The needed locks led to deadlocks before commit
>>> "5ede52538ee2b2202d9dff5b06c33bfde421e6e4 tty: Remove extra
>>> wakeup from pty write() path", which removed the direct callback
>>> to the wakeup function from the tty layer.
>>
>> What does that mean for older kernels? (<
>> 5ede52538ee2b2202d9dff5b06c33bfde421e6e4)
>
> It seems the slcan (and slip) driver is broken for older kernels. See
> this thread for a discussion about the patch in pty.c.
>
> http://marc.info/?l=linux-kernel&m=137269017002789&w=2

Thanks for the info.

> The patch from Peter Hurley was actually already in the queue, when I
> ran into the problem, and is now in kernel 3.12.
>
> Without the pty patch and slow CAN traffic, the driver works, because
> the wakeup is called directly from the pty driver. That is also the
> reason why there was no locking. It would just deadlock.
>
> When the pty driver defers the wakeup, we ran into synchronisation
> problems (which should be fixed by the locking) and eventually into a
> kernel panic because of a recursive loop (which should be fixed by the
> pty.c patch).
>
> Maybe it is possible to get both patches back into the stable branches?

Sounds reasonable. You might get in touch with Peter Hurley, if his
patch is scheduled for stable. Documentation/stable_kernel_rules.txt
suggests a procedure if your patch depends on others to be cherry picked.

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 |

Attachments: