w1: fixes and bundling replies
Subsystems (3)
Quoted 6 lines · Evgeniy Polyakov
Your approach and patch seem correct, but I worry about how old commands are processed. Do I get it right, that replies to old non-bundle commands are queued back instead of sending immediately, but since it is not bundle but single command, queued reply is being sent 'almost' immediately? Basically, nothing changed to old clients, but there is new way to send batch requests now?
Yes, with the previous patch set, if a client sent one command per message they would get the replies in individual packets. However in some cases clients had to bundle multiple commands in one message. For instance when using a slave command to read a temperature sensor it takes one W1_CMD_WRITE command to send a W1_READ_SCRATCHPAD, then another W1_CMD_READ command to read the data back and they have to be sent in the same W1_SLAVE_CMD because there can't be a reset between the two write and read, and you can't do that with a slave command. You can with master commands, but if you are going to issue individual reset, write, and read commands why would you not bundle them?
Here is a new set of patches making bundling opt in.
In this version of the patch, W1_CN_BUNDLE set in the cn_msg.flags enables the kernel to bundle the reply, otherwise replies aren't bundled.
The previous patch separated the data replies and status replies because they have a different ack value which is in the cn_msg and cn_netlink_send could only send one cn_msg in a call. I didn't much like that solution because now status and data replies were out of order. This version creates cn_netlink_send_mult which takes a length which allows multiple cn_msg structures to be sent at once (inside one nlmsghdr, so clients can see there are more cn_msg structures to read), so now status and data messages can be sent in order.
This is somewhat suboptimal as each command has a status reply and possibly a data reply, each requiring a different ack, so even if a client sent multiple commands in one w1_netlink_msg like,
cn_msg [ack ], w1_netlink_msg, w1_netlink_cmd [read], w1_netlink_cmd [read]
the response can't pack the commands into one w1_netlink_msg because of the ack, so there's duplicate of cn_msg and w1_netlink_msg structures,
cn_msg [seq+1], w1_netlink_msg, w1_netlink_cmd [data] cn_msg [ack ], w1_netlink_msg, w1_netlink_cmd [status] cn_msg [seq+1], w1_netlink_msg, w1_netlink_cmd [data] cn_msg [ack ], w1_netlink_msg, w1_netlink_cmd [status]
cn_msg is 20 bytes, w1_netlink_msg 12, so compared to sending separate packets with all the context switches, select overhead and such, bundling is going to be well worth it.
Another alternative could be one cn_msg [seq+1] with the data, and followed by cn_msg [ack ] with the status messages, but they are back to out of order.
I wasn't completely sure what to call the new sending function, I decided on mult for multiple cn_netlink_send_mult, cn_netlink_send_len as another idea, or if there are any better ideas let me know.
I tested with my client program that will bundle up 14 temperature sensor conversions, then after a delay, bundle up another set of commands to read all 14 with the bundle bit set. I also tested with a two year old version of the software that sends requests two one slave at a time (bundling only the write/read to get the data out), and doesn't have code to read the bundling the this patch adds. Both operate correctly running at the same time.
Posted before, no changes
connector support for sending multiple cn_msg
bundling,
corrects ack value to previous ack or seq + 1
corrects the variables to be name consistently
Documentation/connector/connector.txt | 15 +- Documentation/w1/w1.generic | 2 +- Documentation/w1/w1.netlink | 13 +- drivers/connector/connector.c | 17 +- drivers/w1/w1.h | 8 - drivers/w1/w1_netlink.c | 673 ++++++++++++++++++++------------- drivers/w1/w1_netlink.h | 36 ++ include/linux/connector.h | 1 + 8 files changed, 489 insertions(+), 276 deletions(-)
PATCH 1/3 w1: fix netlink refcnt leak on error path
If the message type is W1_MASTER_CMD or W1_SLAVE_CMD, then a reference is taken when searching for the slave or master device. If there isn't any following data m->len (mlen is a copy) is 0 and packing up the message for later execution is skipped leaving nothing to decrement the reference counts.
Way back when, m->len was checked before the search that increments the reference count, but W1_LIST_MASTERS has no additional data, the check was moved in 9be62e0b2fadaf5ff causing this bug.
This change reorders to put the check before the reference count is incremented avoiding the problem.
--- drivers/w1/w1_netlink.c | 44 ++++++++++++++++++++++++++------------------ 1 file changed, 26 insertions(+), 18 deletions(-)
drivers/w1/w1_netlink.c +26 -18
@@ -300,12 +300,6 @@ static int w1_process_command_root(struct cn_msg *msg,struct w1_netlink_msg *w;u32 *id; -if (mcmd->type !=W1_LIST_MASTERS ) { -printk (KERN_NOTICE "%s: msg: %x.%x, wrong type: %u, len: %u.\n" , - __func__, msg->id.idx, msg->id.val, mcmd->type, mcmd->len); -return -EPROTO ; - } - cn =kmalloc (PAGE_SIZE ,GFP_KERNEL );if (!cn)return -ENOMEM ;
@@ -441,6 +435,9 @@ static void w1_process_cb(struct w1_master *dev, struct w1_async_cmd *async_cmd)w1_netlink_send_error (&node->block->msg, node->m, cmd, node->block->portid, err); +/* ref taken in w1_search_slave or w1_search_master_id when building +* the block +*/ if (sl)w1_unref_slave (sl);else
@@ -503,30 +500,42 @@ static void w1_cn_callback(struct cn_msg *msg, struct netlink_skb_parms *nsp) msg_len = msg->len;while (msg_len && !err) { -struct w1_reg_num id; -u16 mlen = m->len; dev =NULL ; sl =NULL ; -memcpy (&id, m->id.id,sizeof (id)); -#if 0 -printk ("%s: %02x.%012llx.%02x: type=%02x, len=%u.\n" , - __func__, id.family, (unsigned long long )id.id, id.crc, m->type, m->len); -#endif if (m->len +sizeof (struct w1_netlink_msg ) > msg_len) { err = -E2BIG ;break ; } +/* execute on this thread, no need to process later */ +if (m->type ==W1_LIST_MASTERS ) { + err =w1_process_command_root (msg, m, nsp->portid); + goto out_cont; + } + +/* All following message types require additional data, +* check here before references are taken. +*/ +if (!m->len) { + err = -EPROTO ; + goto out_cont; + } + +/* both search calls take reference counts */ if (m->type ==W1_MASTER_CMD ) { dev =w1_search_master_id (m->id.mst.id); }else if (m->type ==W1_SLAVE_CMD ) { - sl =w1_search_slave (&id); + sl =w1_search_slave ((struct w1_reg_num *)m->id.id);if (sl) dev = sl->master; }else { - err =w1_process_command_root (msg, m, nsp->portid); +printk (KERN_NOTICE +"%s: msg: %x.%x, wrong type: %u, len: %u.\n" , + __func__, msg->id.idx, msg->id.val, + m->type, m->len); + err = -EPROTO ; goto out_cont; }
@@ -536,8 +545,6 @@ static void w1_cn_callback(struct cn_msg *msg, struct netlink_skb_parms *nsp) } err =0 ; -if (!mlen) - goto out_cont;atomic_inc (&block->refcnt); node->async.cb = w1_process_cb;
@@ -557,7 +564,8 @@ out_cont:if (err)w1_netlink_send_error (msg, m,NULL , nsp->portid, err); msg_len -=sizeof (struct w1_netlink_msg ) + m->len; - m = (struct w1_netlink_msg *)(((u8 *)m) +sizeof (struct w1_netlink_msg ) + m->len); + m = (struct w1_netlink_msg *)(((u8 *)m) + +sizeof (struct w1_netlink_msg ) + m->len);/* * Let's allow requests for nonexisting devices.
Signature · 1 line
1.7.10.4
PATCH 2/3 connector: allow multiple messages to be sent in one packet
In reply to David FriesThis increases the amount of bundling to reduce the number of packets sent. For the one wire use there can be multiple struct w1_netlink_cmd in a struct w1_netlink_msg and multiple of those in struct cn_msg, and with this change multiple of those in a struct nlmsghdr, and at each level the len identifies there being multiple of the next.
--- Documentation/connector/connector.txt | 13 ++++++++++--- drivers/connector/connector.c | 17 +++++++++++++++-- include/linux/connector.h | 1 + 3 files changed, 26 insertions(+), 5 deletions(-)
Documentation/connector/connector.txt +10 -3
@@ -24,7 +24,8 @@ netlink based networking for inter-process communication in a significantly easier way: int cn_add_callback(struct cb_id *id, char *name, void (*callback) (struct cn_msg *, struct netlink_skb_parms *)); -void cn_netlink_send(struct cn_msg *msg, u32 __group, int gfp_mask); +void cn_netlink_send_multi(struct cn_msg *msg, u16 len, u32 portid, u32 __group, int gfp_mask); +void cn_netlink_send(struct cn_msg *msg, u32 portid, u32 __group, int gfp_mask); struct cb_id {
@@ -71,15 +72,21 @@ void cn_del_callback(struct cb_id *id); struct cb_id *id - unique connector's user identifier. -int cn_netlink_send(struct cn_msg *msg, u32 __groups, int gfp_mask); +int cn_netlink_send_multi(struct cn_msg *msg, u16 len, u32 portid, u32 __groups, int gfp_mask); +int cn_netlink_send(struct cn_msg *msg, u32 portid, u32 __groups, int gfp_mask); Sends message to the specified groups. It can be safely called from softirq context, but may silently fail under strong memory pressure. If there are no listeners for given group -ESRCH can be returned. struct cn_msg * - message header(with attached data). + u16 len - for *_multi multiple cn_msg messages can be sent + u32 port - destination port. + If non-zero the message will be sent to the + given port, which should be set to the + original sender. u32 __group - destination group. - If __group is zero, then appropriate group will + If port and __group is zero, then appropriate group will be searched through all registered connector users, and message will be delivered to the group which was created for user with the same ID as in msg.
drivers/connector/connector.c +15 -2
@@ -43,6 +43,8 @@ static struct cn_dev cdev;static int cn_already_initialized;/* +* Sends mult (multiple) cn_msg at a time. +* * msg->seq and msg->ack are used to determine message genealogy. * When someone sends message it puts there locally unique sequence * and random acknowledge numbers. Sequence number may be copied into
@@ -62,10 +64,13 @@ static int cn_already_initialized;* the acknowledgement number in the original message + 1, then it is * a new message. * +* If msg->len != len, then additional cn_msg messages are expected following +* the first msg. +* * The message is sent to, the portid if given, the group if given, both if * both, or if both are zero then the group is looked up and sent there. */ -int cn_netlink_send (struct cn_msg *msg,u32 portid,u32 __group, +int cn_netlink_send_mult (struct cn_msg *msg,u16 len,u32 portid,u32 __group,gfp_t gfp_mask) {struct cn_callback_entry *__cbq;
@@ -98,7 +103,7 @@ int cn_netlink_send(struct cn_msg *msg, u32 portid, u32 __group,if (!portid && !netlink_has_listeners (dev->nls, group))return -ESRCH ; - size =sizeof (*msg) + msg->len; + size =sizeof (*msg) + len; skb =nlmsg_new (size, gfp_mask);if (!skb)
@@ -121,6 +126,14 @@ int cn_netlink_send(struct cn_msg *msg, u32 portid, u32 __group, gfp_mask);return netlink_unicast (dev->nls, skb, portid, !(gfp_mask&__GFP_WAIT)); } +EXPORT_SYMBOL_GPL (cn_netlink_send_mult); + +/* same as cn_netlink_send_mult except msg->len is used for len */ +int cn_netlink_send (struct cn_msg *msg,u32 portid,u32 __group, +gfp_t gfp_mask) +{ +return cn_netlink_send_mult (msg, msg->len, portid, __group, gfp_mask); +}EXPORT_SYMBOL_GPL (cn_netlink_send);/*
include/linux/connector.h +1 -0
@@ -71,6 +71,7 @@ struct cn_dev {int cn_add_callback (struct cb_id *id,const char *name,void (*callback)(struct cn_msg *,struct netlink_skb_parms *));void cn_del_callback (struct cb_id *); +int cn_netlink_send_mult (struct cn_msg *msg,u16 len,u32 portid,u32 group,gfp_t gfp_mask);int cn_netlink_send (struct cn_msg *msg,u32 portid,u32 group,gfp_t gfp_mask); int cn_queue_add_callback(struct cn_queue_dev *dev, const char *name,
Signature · 1 line
1.7.10.4
PATCH 3/3 w1: optional bundling of netlink kernel replies
In reply to David FriesApplications can submit a set of commands in one packet to the kernel, and in some cases it is required such as reading the temperature sensor results. This adds an option W1_CN_BUNDLE to the flags of cn_msg to request the kernel to reply in one packet for efficiency.
The cn_msg flags now check for unknown flag values and return an error if one is seen. See "Proper handling of unknown flags in system calls" http://lwn.net/Articles/588444/
This corrects the ack values returned as per the protocol standard, namely the original ack for status messages and seq + 1 for all others such as the data returned from a read.
Some of the common variable names have been standardized as follows. struct cn_msg *cn
struct w1_netlink_msg *msg
struct w1_netlink_cmd *cmd
struct w1_master *dev
When an argument and a function scope variable would collide, add req_ to the argument.
--- Documentation/connector/connector.txt | 2 +- Documentation/w1/w1.generic | 2 +- Documentation/w1/w1.netlink | 13 +- drivers/w1/w1.h | 8 - drivers/w1/w1_netlink.c | 649 ++++++++++++++++++++------------- drivers/w1/w1_netlink.h | 36 ++ 6 files changed, 447 insertions(+), 263 deletions(-)
Documentation/connector/connector.txt +1 -1
@@ -118,7 +118,7 @@ acknowledge number MUST be the same + 1. If we receive a message and its sequence number is not equal to one we are expecting, then it is a new message. If we receive a message and its sequence number is the same as one we are expecting, but its -acknowledge is not equal to the acknowledge number in the original +acknowledge is not equal to the sequence number in the original message + 1, then it is a new message. Obviously, the protocol header contains the above id.
Documentation/w1/w1.generic +1 -1
@@ -82,7 +82,7 @@ driver - (standard) symlink to the w1 driver w1_master_add - Manually register a slave device w1_master_attempts - the number of times a search was attempted w1_master_max_slave_count - - the maximum slaves that may be attached to a master + - maximum number of slaves to search for at a time w1_master_name - the name of the device (w1_bus_masterX) w1_master_pullup - 5V strong pullup 0 enabled, 1 disabled w1_master_remove - Manually remove a slave device
Documentation/w1/w1.netlink +8 -5
@@ -30,7 +30,7 @@ Protocol. W1_SLAVE_CMD userspace command for slave device (read/write/touch) - __u8 res - reserved + __u8 status - error indication from kernel __u16 len - size of data attached to this header data union { __u8 id[8]; - slave unique device id
@@ -44,10 +44,14 @@ Protocol. __u8 cmd - command opcode. W1_CMD_READ - read command W1_CMD_WRITE - write command - W1_CMD_TOUCH - touch command - (write and sample data back to userspace) W1_CMD_SEARCH - search command W1_CMD_ALARM_SEARCH - alarm search command + W1_CMD_TOUCH - touch command + (write and sample data back to userspace) + W1_CMD_RESET - send bus reset + W1_CMD_SLAVE_ADD - add slave to kernel list + W1_CMD_SLAVE_REMOVE - remove slave from kernel list + W1_CMD_LIST_SLAVES - get slaves list from kernel __u8 res - reserved __u16 len - length of data for this command For read command data must be allocated like for write command
@@ -87,8 +91,7 @@ format: id0 ... idN Each message is at most 4k in size, so if number of master devices - exceeds this, it will be split into several messages, - cn.seq will be increased for each one. + exceeds this, it will be split into several messages. W1 search and alarm search commands. request:
drivers/w1/w1.h +0 -8
@@ -203,7 +203,6 @@ enum w1_master_flags {* @search_id: allows continuing a search * @refcnt: reference count * @priv: private data storage -* @priv_size: size allocated * @enable_pullup: allows a strong pullup * @pullup_duration: time for the next strong pullup * @flags: one of w1_master_flags
@@ -214,7 +213,6 @@ enum w1_master_flags {* @dev: sysfs device * @bus_master: io operations available * @seq: sequence number used for netlink broadcasts -* @portid: destination for the current netlink command */ struct w1_master {
@@ -241,7 +239,6 @@ struct w1_masteratomic_t refcnt;void *priv; -int priv_size;/** 5V strong pullup enabled flag, 1 enabled, zero disabled. */ int enable_pullup;
@@ -260,11 +257,6 @@ struct w1_masterstruct w1_bus_master *bus_master;u32 seq; -/* port id to send netlink responses to. The value is temporarily -* stored here while processing a message, set after locking the -* mutex, zero before unlocking the mutex. -*/ -u32 portid; };/**
drivers/w1/w1_netlink.c +401 -248
894 lines not on this page · raw message
drivers/w1/w1_netlink.h +36 -0
@@ -28,6 +28,17 @@#include "w1.h" /** +* enum w1_cn_msg_flags - bitfield flags for struct cn_msg.flags +* +* @W1_CN_BUNDLE: Request bundling replies into fewer messagse. Be prepared +* to handle multiple struct cn_msg, struct w1_netlink_msg, and +* struct w1_netlink_cmd in one packet. +*/ +enum w1_cn_msg_flags { +W1_CN_BUNDLE =1 , +}; + +/** * enum w1_netlink_message_types - message type * * @W1_SLAVE_ADD: notification that a slave device was added
@@ -49,6 +60,19 @@ enum w1_netlink_message_types { W1_LIST_MASTERS, }; +/** +* struct w1_netlink_msg - holds w1 message type, id, and result +* +* @type: one of enum w1_netlink_message_types +* @status: kernel feedback for success 0 or errno failure value +* @len: length of data following w1_netlink_msg +* @id: union holding master bus id (msg.id) and slave device id (id[8]). +* @data: start address of any following data +* +* The base message structure for w1 messages over netlink. +* The netlink connector data sequence is, struct nlmsghdr, struct cn_msg, +* then one or more struct w1_netlink_msg (each with optional data). +*/ struct w1_netlink_msg {__u8 type;
@@ -66,6 +90,7 @@ struct w1_netlink_msg/** * enum w1_commands - commands available for master or slave operations +* * @W1_CMD_READ: read len bytes * @W1_CMD_WRITE: write len bytes * @W1_CMD_SEARCH: initiate a standard search, returns only the slave
@@ -93,6 +118,17 @@ enum w1_commands {W1_CMD_MAX }; +/** +* struct w1_netlink_cmd - holds the command and data +* +* @cmd: one of enum w1_commands +* @res: reserved +* @len: length of data following w1_netlink_cmd +* @data: start address of any following data +* +* One or more struct w1_netlink_cmd is placed starting at w1_netlink_msg.data +* each with optional data. +*/ struct w1_netlink_cmd {__u8 cmd;
Signature · 1 line
1.7.10.4
Evgeniy,
Could you review this set of patches in this thread? Thanks.
Quoted 91 lines · David Fries
Quoted 6 lines · Evgeniy Polyakov
Your approach and patch seem correct, but I worry about how old commands are processed. Do I get it right, that replies to old non-bundle commands are queued back instead of sending immediately, but since it is not bundle but single command, queued reply is being sent 'almost' immediately? Basically, nothing changed to old clients, but there is new way to send batch requests now?
Yes, with the previous patch set, if a client sent one command per message they would get the replies in individual packets. However in some cases clients had to bundle multiple commands in one message. For instance when using a slave command to read a temperature sensor it takes one
W1_CMD_WRITEcommand to send aW1_READ_SCRATCHPAD, then anotherW1_CMD_READcommand to read the data back and they have to be sent in the sameW1_SLAVE_CMDbecause there can't be a reset between the two write and read, and you can't do that with a slave command. You can with master commands, but if you are going to issue individual reset, write, and read commands why would you not bundle them?Here is a new set of patches making bundling opt in.
In this version of the patch,
W1_CN_BUNDLEset in the cn_msg.flags enables the kernel to bundle the reply, otherwise replies aren't bundled.The previous patch separated the data replies and status replies because they have a different ack value which is in the
cn_msgandcn_netlink_sendcould only send onecn_msgin a call. I didn't much like that solution because now status and data replies were out of order. This version createscn_netlink_send_multwhich takes a length which allows multiplecn_msgstructures to be sent at once (inside one nlmsghdr, so clients can see there are morecn_msgstructures to read), so now status and data messages can be sent in order.This is somewhat suboptimal as each command has a status reply and possibly a data reply, each requiring a different ack, so even if a client sent multiple commands in one
w1_netlink_msglike,
cn_msg[ack ],w1_netlink_msg,w1_netlink_cmd[read],w1_netlink_cmd[read]the response can't pack the commands into one
w1_netlink_msgbecause of the ack, so there's duplicate ofcn_msgandw1_netlink_msgstructures,
cn_msg[seq+1],w1_netlink_msg,w1_netlink_cmd[data]cn_msg[ack ],w1_netlink_msg,w1_netlink_cmd[status]cn_msg[seq+1],w1_netlink_msg,w1_netlink_cmd[data]cn_msg[ack ],w1_netlink_msg,w1_netlink_cmd[status]
cn_msgis 20 bytes,w1_netlink_msg12, so compared to sending separate packets with all the context switches, select overhead and such, bundling is going to be well worth it.Another alternative could be one
cn_msg[seq+1] with the data, and followed bycn_msg[ack ] with the status messages, but they are back to out of order.I wasn't completely sure what to call the new sending function, I decided on mult for multiple
cn_netlink_send_mult,cn_netlink_send_lenas another idea, or if there are any better ideas let me know.I tested with my client program that will bundle up 14 temperature sensor conversions, then after a delay, bundle up another set of commands to read all 14 with the bundle bit set. I also tested with a two year old version of the software that sends requests two one slave at a time (bundling only the write/read to get the data out), and doesn't have code to read the bundling the this patch adds. Both operate correctly running at the same time.
Posted before, no changes
connector support for sending multiple
cn_msgbundling,
corrects ack value to previous ack or seq + 1
corrects the variables to be name consistentlyDocumentation/connector/connector.txt | 15 +- Documentation/w1/w1.generic | 2 +- Documentation/w1/w1.netlink | 13 +- drivers/connector/connector.c | 17 +- drivers/w1/w1.h | 8 - drivers/w1/w1_netlink.c | 673 ++++++++++++++++++++------------- drivers/w1/w1_netlink.h | 36 ++ include/linux/connector.h | 1 + 8 files changed, 489 insertions(+), 276 deletions(-)--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Signature · 2 lines
David Fries <[email protected]> PGP pub CB1EE8F0 http://fries.net/~david/
Hi David
07.04.2014, 18:13, "David Fries" <[email protected]>:
Quoted 2 lines
Evgeniy,
Could you review this set of patches in this thread? Thanks.
I've checked it and it looks good, please resend it to lkml and Greg Kroah-Hartman, he will pull it into the tree. Feel free to add my Acked-by.