Re: [ANNOUNCE] Chelsio 10Gb TOE (TCP Offload Engine)

[Stephen Hemminger]

Great to see TOE support getting reviewed.

Some general stuff:
  * Use linux indentation style (Documentation/Codingstyle)
 Tabs are 8 characters, and thus indentations are also 8
characters. You are using 4 spaces.

  * Don't use // for comments

  * All patches must have Signed-off-by: line. This is a legal
requirement see Documentation/SubmittingPatches.

   * Why use name for binding, it is slow to lookup and can change.
 Either use if_index or pointer. If you used driver model/sysfs
 you wouldn't need to.

   * Please make the toe devices part of sysfs.
 Probably simplest to make toedev a kobject that is a child
 of the netdevice.

   * Can't you use sysfs instead of /proc? (see above)

   * Please change all EXPORT_SYMBOL to EXPORT_SYMBOL_GPL to explicitly
 impede binary drivers.

   * Please don't include 2.4 compatiablity in the 2.6 version. This
 is new code.

   * Consider making TOE a config option, and stubbing as appropriate
 many platforms may not want it.


diff -Naur linux-2.6.13-rc3/include/linux/toedev.h linux-2.6.13-
rc3.patched/include/linux/toedev.h --- linux-2.6.13-rc3/include/linux/
toedev.h1969-12-31 16:00:00.0 -0800 +++ linux-2.6.13-
rc3.patched/include/linux/toedev.h  2005-07-26 12:23:53.796497048
-0700 @@ -0,0 +1,96 @@

...
+
+#ifndef _TOEDEV_H_
+#define _TOEDEV_H_
+
+#include linux/list.h
+#include asm/atomic.h
+
+#define TOENAMSIZ 16
See above comments about name

+/* belongs in linux/netdevice.h */
+#define NETIF_F_TCPIP_OFFLOAD (1  16)
If it belongs there, then put it there!

+/* Get the toedev associated with a net_device */
+#define TOEDEV(netdev) ((struct toedev *)(netdev)-ec_ptr)
Use inline instead to get type checking.

+/* TOE type ids */
+enum {
+TOE_ID_CHELSIO_T1  = 1,
+TOE_ID_CHELSIO_T1C,
+TOE_ID_CHELSIO_T3,
+};
+
+struct toe_id {
+unsigned int id;
+unsigned long data;
+};
+
+#define END_OF_TOE_ID_TABLE { 0, 0UL }
Get rid of this macro. that's just obfuscation.

+
+struct net_device;
+struct neighbour;
+struct tom_info;
+struct proc_dir_entry;
+struct sock;
+struct sk_buff;

Include the necessary definitions.


+struct toedev {
+char name[TOENAMSIZ];   /* TOE device name */
+struct list_head toe_list;  /* for list linking */
+int toe_index;  /* unique TOE device index */
+unsigned int ttid;  /* TOE type id */
+unsigned long flags;/* device flags */
+unsigned int mtu;   /* max size of TX offloaded
data */ 
+unsigned int nconn; /* max # of offloaded
connections */
+struct net_device *lldev;  /* LL device associated with TOE
messages */ ...
diff -Naur linux-2.6.13-rc3/include/net/offload.h linux-2.6.13-
rc3.patched/include/net/offload.h --- linux-2.6.13-rc3/include/net/
offload.h   1969-12-31 16:00:00.0 -0800 +++ linux-2.6.13-
rc3.patched/include/net/offload.h   2005-07-26 12:23:53.796497048
-0700 @@ -0,0 +1,47 @@ +
+/* Returns true if sk is an offloaded IPv4 TCP socket. */
+#define IS_OFFLOADED(sk) (((sk)-sk_family == AF_INET  (sk)-
sk_prot != tcp_prot))

inline?

+struct toedev;
+struct sk_buff;
+struct sock;

More redundant fwd declarations.


+/* Per-skb backlog handler.  Run when a socket's backlog is processed.
*/ +struct blog_skb_cb {
+   void (*backlog_rcv)(struct sock *sk, struct sk_buff *skb);
+   struct toedev *dev;
+};
+
+#define BLOG_SKB_CB(skb) ((struct blog_skb_cb *)(skb)-cb)
+
+/* belongs in linux/tcp_diag.h */
+#define TCPDIAG_OFFLOAD 5
+
Please patch tcp_diag.h then.


diff -Naur linux-2.6.13-rc3/net/core/toedev.c linux-2.6.13-rc3.patched/
net/core/toedev.c --- linux-2.6.13-rc3/net/core/toedev.c
1969-12-31 16:00:00.0 -0800 +++ linux-2.6.13-rc3.patched/net/
core/toedev.c   2005-07-26 12:23:53.799496592 -0700 @@ -0,0 +1,432
@@ +
+/* 2.4 compatibility */
+#ifndef subsys_initcall
+#define subsys_initcall(fn) module_init(fn)
+
+static int boot_phase = 1;
+#else
+#define boot_phase 0
+#endif

No, just do 2.6
+
+/*
+ * Returns the entry in the TOE id table 'table' that has a given id,
or NULL
+ * if the id is not found.
+ */
+static const struct toe_id *id_find(unsigned int id,
+   const struct toe_id *table)
+{
+const struct toe_id *p;
+
+for (p = table; p-id; ++p)
+   if (p-id == id) return p;
please split line


Could you use RCU?
+   
+/*
+ * Register a TCP Offload Module (TOM).
+ */
+int register_tom(struct tom_info *t)
+{
+down(toedev_db_lock);
+list_add(t-list_node, tom_list);
+up(toedev_db_lock);
+return 0;
+}

Using a semaphore seems more than you need here.
Given the code path you could either assume RT
netlink semaphore or just use a spinlock.

... 
diff -Naur linux-2.6.13-rc3/net/ipv4/tcp_ipv4.c linux-2.6.13-
rc3.patched/net/ipv4/tcp_ipv4.c --- linux-2.6.13-rc3/net/ipv4/
tcp_ipv4.c  2005-07-26 10:16:10.0 

[ANNOUNCE] Chelsio 10Gb TOE (TCP Offload Engine)

[Scott Bardone]

Chelsio Communications would like to announce the availability of its
TCP offload (TOE) support for Linux under the GPL.  This is code
developed by us over the past couple of years and has been in production
for over a year.  The code, architecture description, and some papers
comparing TOE performance to other technologies are available from 
https://service.chelsio.com/open_toe/index.html.


We are aware that TOEs are viewed with much skepticism in the Linux
community but we believe that a lot of the concerns often brought up
have to do with implementation details of particular products rather
than with the technology as a whole.  Chelsio is proposing a solution
that we feel allows TOEs to coexist alongside the regular stack's TCP
without breaking networking features, and allows the combined network
stack to offer superior TCP performance.  The code we are releasing
today has been used with an older version of Linux to set the current
Internet2 land speed record and has demonstrated improved performance
with a variety of applications and benchmarks.  As another example of
performance benefits, while today's NICs cannot handle 10G receive with
regular frames, a TOE can comfortably do so with much of the CPU left
for application processing.

The proposed design is intended to accommodate products from multiple
vendors and roughly has the following components:

- a vendor neutral cut-down analog of core/dev.c that provides
registration and activation facilities for TOE devices and some basic
data path functionality (mostly to deal with sniffers).  This component
does not introduce any new soft irqs, instead TOE devices use regular
facilities, such as NAPI, to service incoming traffic;

- some changes to existing TCP code and some additions to provide
offloading.  Changes to existing code are a few dozen lines and are
usually either notification of TOEs when the SW stack processes certain
events, e.g., ARP, or they allow TOEs to perform some socket operations
differently from the SW stack (usually this is done by changing sk_prot,
but some of the differing operations aren't covered by that and so need
to be done through other changes);

- the offloading support specific to each TOE is provided by two
drivers, one that deals with HW and one that interfaces with the SW
TCP/IP and sockets layer (these are separate conceptually, they may be
one driver implementation-wise)

More details of the proposed scheme and of the working of the various
operations can be found in the architecture document at the above URL.
We are including a patch containing the TCP changes below (against
2.6.12), and the rest of the vendor-neutral pieces will follow in
subsequent emails.  We are not posting the drivers on the list due to
their size (the TOE driver though is an extension of Chelsio's NIC
driver presently in Jeff's tree).  All the code is available at the
above URL.  (We'd like to point out that the released code is our
current production codebase that accommodates both 2.4 and 2.6 kernels.
We are aware that we'll need to strip the compatibility stuff and plan
to do so.)

Thanks for your attention and we are looking forward to your comments.

Chelsio Communications.
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html