Date: Mon, 7 Apr 2014 09:13:13 -0500
From: David Fries <David@Fries.net>
To: linux-kernel@vger.kernel.org
Cc: Evgeniy Polyakov <zbr@ioremap.net>
Subject: Re: [RFC] w1: fixes and bundling replies
Message-ID: <20140407141313.GC5096@spacedout.fries.net>
References: <1395538067-4029-1-git-send-email-David@Fries.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1395538067-4029-1-git-send-email-David@Fries.net>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org

Evgeniy,
Could you review this set of patches in this thread?  Thanks.

On Sat, Mar 22, 2014 at 08:27:44PM -0500, David Fries wrote:
> On Fri, Feb 21, 2014 at 01:07:28AM +0400, Evgeniy Polyakov wrote:
> > 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(-)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
David Fries <david@fries.net>    PGP pub CB1EE8F0
http://fries.net/~david/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/