netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 4/6] IB: address translation to map IP to IB addresses (GIDs)
From: Sean Hefty @ 2006-03-06 19:10 UTC (permalink / raw)
  To: 'Roland Dreier', linux-kernel, netdev; +Cc: openib-general
In-Reply-To: <adaslpz2l9p.fsf@cisco.com>

Add an address translation service that maps IP addresses to Infiniband
GID addresses using IPoIB.

Signed-off-by: Sean Hefty <sean.hefty@intel.com>

---

diff -uprN -X linux-2.6.git/Documentation/dontdiff 
linux-2.6.git/drivers/infiniband/core/addr.c 
linux-2.6.ib/drivers/infiniband/core/addr.c
--- linux-2.6.git/drivers/infiniband/core/addr.c	1969-12-31 16:00:00.000000000 -0800
+++ linux-2.6.ib/drivers/infiniband/core/addr.c	2006-01-16 16:14:24.000000000 -0800
@@ -0,0 +1,356 @@
+/*
+ * Copyright (c) 2005 Voltaire Inc.  All rights reserved.
+ * Copyright (c) 2002-2005, Network Appliance, Inc. All rights reserved.
+ * Copyright (c) 1999-2005, Mellanox Technologies, Inc. All rights reserved.
+ * Copyright (c) 2005 Intel Corporation.  All rights reserved.
+ *
+ * This Software is licensed under one of the following licenses:
+ *
+ * 1) under the terms of the "Common Public License 1.0" a copy of which is
+ *    available from the Open Source Initiative, see
+ *    http://www.opensource.org/licenses/cpl.php.
+ *
+ * 2) under the terms of the "The BSD License" a copy of which is
+ *    available from the Open Source Initiative, see
+ *    http://www.opensource.org/licenses/bsd-license.php.
+ *
+ * 3) under the terms of the "GNU General Public License (GPL) Version 2" a
+ *    copy of which is available from the Open Source Initiative, see
+ *    http://www.opensource.org/licenses/gpl-license.php.
+ *
+ * Licensee has the right to choose one of the above licenses.
+ *
+ * Redistributions of source code must retain the above copyright
+ * notice and one of the license notices.
+ *
+ * Redistributions in binary form must reproduce both the above copyright
+ * notice, one of the license notices in the documentation
+ * and/or other materials provided with the distribution.
+ */
+#include <linux/inetdevice.h>
+#include <linux/workqueue.h>
+#include <net/arp.h>
+#include <net/neighbour.h>
+#include <net/route.h>
+#include <rdma/ib_addr.h>
+
+MODULE_AUTHOR("Sean Hefty");
+MODULE_DESCRIPTION("IB Address Translation");
+MODULE_LICENSE("Dual BSD/GPL");
+
+struct addr_req {
+	struct list_head list;
+	struct sockaddr src_addr;
+	struct sockaddr dst_addr;
+	struct rdma_dev_addr *addr;
+	void *context;
+	void (*callback)(int status, struct sockaddr *src_addr,
+			 struct rdma_dev_addr *addr, void *context);
+	unsigned long timeout;
+	int status;
+};
+
+static void process_req(void *data);
+
+static DECLARE_MUTEX(mutex);
+static LIST_HEAD(req_list);
+static DECLARE_WORK(work, process_req, NULL);
+struct workqueue_struct *rdma_wq;
+EXPORT_SYMBOL(rdma_wq);
+
+static int copy_addr(struct rdma_dev_addr *dev_addr, struct net_device *dev,
+		     unsigned char *dst_dev_addr)
+{
+	switch (dev->type) {
+	case ARPHRD_INFINIBAND:
+		dev_addr->dev_type = IB_NODE_CA;
+		break;
+	default:
+		return -EADDRNOTAVAIL;
+	}
+
+	memcpy(dev_addr->src_dev_addr, dev->dev_addr, MAX_ADDR_LEN);
+	memcpy(dev_addr->broadcast, dev->broadcast, MAX_ADDR_LEN);
+	if (dst_dev_addr)
+		memcpy(dev_addr->dst_dev_addr, dst_dev_addr, MAX_ADDR_LEN);
+	return 0;
+}
+
+int rdma_translate_ip(struct sockaddr *addr, struct rdma_dev_addr *dev_addr)
+{
+	struct net_device *dev;
+	u32 ip = ((struct sockaddr_in *) addr)->sin_addr.s_addr;
+	int ret;
+
+	dev = ip_dev_find(ip);
+	if (!dev)
+		return -EADDRNOTAVAIL;
+
+	ret = copy_addr(dev_addr, dev, NULL);
+	dev_put(dev);
+	return ret;
+}
+EXPORT_SYMBOL(rdma_translate_ip);
+
+static void set_timeout(unsigned long time)
+{
+	unsigned long delay;
+
+	cancel_delayed_work(&work);
+
+	delay = time - jiffies;
+	if ((long)delay <= 0)
+		delay = 1;
+
+	queue_delayed_work(rdma_wq, &work, delay);
+}
+
+static void queue_req(struct addr_req *req)
+{
+	struct addr_req *temp_req;
+
+	down(&mutex);
+	list_for_each_entry_reverse(temp_req, &req_list, list) {
+		if (time_after(req->timeout, temp_req->timeout))
+			break;
+	}
+
+	list_add(&req->list, &temp_req->list);
+
+	if (req_list.next == &req->list)
+		set_timeout(req->timeout);
+	up(&mutex);
+}
+
+static void addr_send_arp(struct sockaddr_in *dst_in)
+{
+	struct rtable *rt;
+	struct flowi fl;
+	u32 dst_ip = dst_in->sin_addr.s_addr;
+
+	memset(&fl, 0, sizeof fl);
+	fl.nl_u.ip4_u.daddr = dst_ip;
+	if (ip_route_output_key(&rt, &fl))
+		return;
+
+	arp_send(ARPOP_REQUEST, ETH_P_ARP, rt->rt_gateway, rt->idev->dev,
+		 rt->rt_src, NULL, rt->idev->dev->dev_addr, NULL);
+	ip_rt_put(rt);
+}
+
+static int addr_resolve_remote(struct sockaddr_in *src_in,
+			       struct sockaddr_in *dst_in,
+			       struct rdma_dev_addr *addr)
+{
+	u32 src_ip = src_in->sin_addr.s_addr;
+	u32 dst_ip = dst_in->sin_addr.s_addr;
+	struct flowi fl;
+	struct rtable *rt;
+	struct neighbour *neigh;
+	int ret;
+
+	memset(&fl, 0, sizeof fl);
+	fl.nl_u.ip4_u.daddr = dst_ip;
+	fl.nl_u.ip4_u.saddr = src_ip;
+	ret = ip_route_output_key(&rt, &fl);
+	if (ret)
+		goto out;
+
+	neigh = neigh_lookup(&arp_tbl, &rt->rt_gateway, rt->idev->dev);
+	if (!neigh) {
+		ret = -ENODATA;
+		goto err1;
+	}
+
+	if (!(neigh->nud_state & NUD_VALID)) {
+		ret = -ENODATA;
+		goto err2;
+	}
+
+	if (!src_ip) {
+		src_in->sin_family = dst_in->sin_family;
+		src_in->sin_addr.s_addr = rt->rt_src;
+	}
+
+	ret = copy_addr(addr, neigh->dev, neigh->ha);
+err2:
+	neigh_release(neigh);
+err1:
+	ip_rt_put(rt);
+out:
+	return ret;
+}
+
+static void process_req(void *data)
+{
+	struct addr_req *req, *temp_req;
+	struct sockaddr_in *src_in, *dst_in;
+	struct list_head done_list;
+
+	INIT_LIST_HEAD(&done_list);
+
+	down(&mutex);
+	list_for_each_entry_safe(req, temp_req, &req_list, list) {
+		if (req->status) {
+			src_in = (struct sockaddr_in *) &req->src_addr;
+			dst_in = (struct sockaddr_in *) &req->dst_addr;
+			req->status = addr_resolve_remote(src_in, dst_in,
+							  req->addr);
+		}
+		if (req->status && time_after(jiffies, req->timeout))
+			req->status = -ETIMEDOUT;
+		else if (req->status == -ENODATA)
+			continue;
+
+		list_del(&req->list);
+		list_add_tail(&req->list, &done_list);
+	}
+
+	if (!list_empty(&req_list)) {
+		req = list_entry(req_list.next, struct addr_req, list);
+		set_timeout(req->timeout);
+	}
+	up(&mutex);
+
+	list_for_each_entry_safe(req, temp_req, &done_list, list) {
+		list_del(&req->list);
+		req->callback(req->status, &req->src_addr, req->addr,
+			      req->context);
+		kfree(req);
+	}
+}
+
+static int addr_resolve_local(struct sockaddr_in *src_in,
+			      struct sockaddr_in *dst_in,
+			      struct rdma_dev_addr *addr)
+{
+	struct net_device *dev;
+	u32 src_ip = src_in->sin_addr.s_addr;
+	u32 dst_ip = dst_in->sin_addr.s_addr;
+	int ret;
+
+	dev = ip_dev_find(dst_ip);
+	if (!dev)
+		return -EADDRNOTAVAIL;
+
+	if (!src_ip) {
+		src_in->sin_family = dst_in->sin_family;
+		src_in->sin_addr.s_addr = dst_ip;
+		ret = copy_addr(addr, dev, dev->dev_addr);
+	} else {
+		ret = rdma_translate_ip((struct sockaddr *)src_in, addr);
+		if (!ret)
+			memcpy(addr->dst_dev_addr, dev->dev_addr, MAX_ADDR_LEN);
+	}
+
+	dev_put(dev);
+	return ret;
+}
+
+int rdma_resolve_ip(struct sockaddr *src_addr, struct sockaddr *dst_addr,
+		    struct rdma_dev_addr *addr, int timeout_ms,
+		    void (*callback)(int status, struct sockaddr *src_addr,
+				     struct rdma_dev_addr *addr, void *context),
+		    void *context)
+{
+	struct sockaddr_in *src_in, *dst_in;
+	struct addr_req *req;
+	int ret = 0;
+
+	req = kmalloc(sizeof *req, GFP_KERNEL);
+	if (!req)
+		return -ENOMEM;
+	memset(req, 0, sizeof *req);
+
+	if (src_addr)
+		memcpy(&req->src_addr, src_addr, ip_addr_size(src_addr));
+	memcpy(&req->dst_addr, dst_addr, ip_addr_size(dst_addr));
+	req->addr = addr;
+	req->callback = callback;
+	req->context = context;
+
+	src_in = (struct sockaddr_in *) &req->src_addr;
+	dst_in = (struct sockaddr_in *) &req->dst_addr;
+
+	req->status = addr_resolve_local(src_in, dst_in, addr);
+	if (req->status == -EADDRNOTAVAIL)
+		req->status = addr_resolve_remote(src_in, dst_in, addr);
+
+	switch (req->status) {
+	case 0:
+		req->timeout = jiffies;
+		queue_req(req);
+		break;
+	case -ENODATA:
+		req->timeout = msecs_to_jiffies(timeout_ms) + jiffies;
+		queue_req(req);
+		addr_send_arp(dst_in);
+		break;
+	default:
+		ret = req->status;
+		kfree(req);
+		break;
+	}
+	return ret;
+}
+EXPORT_SYMBOL(rdma_resolve_ip);
+
+void rdma_addr_cancel(struct rdma_dev_addr *addr)
+{
+	struct addr_req *req, *temp_req;
+
+	up(&mutex);
+	list_for_each_entry_safe(req, temp_req, &req_list, list) {
+		if (req->addr == addr) {
+			req->status = -ECANCELED;
+			req->timeout = jiffies;
+			list_del(&req->list);
+			list_add(&req->list, &req_list);
+			set_timeout(req->timeout);
+			break;
+		}
+	}
+	up(&mutex);
+}
+EXPORT_SYMBOL(rdma_addr_cancel);
+
+static int addr_arp_recv(struct sk_buff *skb, struct net_device *dev,
+			 struct packet_type *pkt, struct net_device *orig_dev)
+{
+	struct arphdr *arp_hdr;
+
+	arp_hdr = (struct arphdr *) skb->nh.raw;
+
+	if (dev->type == ARPHRD_INFINIBAND &&
+	    (arp_hdr->ar_op == __constant_htons(ARPOP_REQUEST) ||
+	     arp_hdr->ar_op == __constant_htons(ARPOP_REPLY)))
+		set_timeout(jiffies);
+
+	kfree_skb(skb);
+	return 0;
+}
+
+static struct packet_type addr_arp = {
+	.type           = __constant_htons(ETH_P_ARP),
+	.func           = addr_arp_recv,
+	.af_packet_priv = (void*) 1,
+};
+
+static int addr_init(void)
+{
+	rdma_wq = create_singlethread_workqueue("rdma_wq");
+	if (!rdma_wq)
+		return -ENOMEM;
+
+	dev_add_pack(&addr_arp);
+	return 0;
+}
+
+static void addr_cleanup(void)
+{
+	dev_remove_pack(&addr_arp);
+	destroy_workqueue(rdma_wq);
+}
+
+module_init(addr_init);
+module_exit(addr_cleanup);
diff -uprN -X linux-2.6.git/Documentation/dontdiff 
linux-2.6.git/drivers/infiniband/core/Makefile 
linux-2.6.ib/drivers/infiniband/core/Makefile
--- linux-2.6.git/drivers/infiniband/core/Makefile	2006-01-16 16:03:08.000000000 -0800
+++ linux-2.6.ib/drivers/infiniband/core/Makefile	2006-01-16 16:14:24.000000000 -0800
@@ -1,5 +1,5 @@
 obj-$(CONFIG_INFINIBAND) +=		ib_core.o ib_mad.o ib_sa.o \
-					ib_cm.o
+					ib_cm.o ib_addr.o
 obj-$(CONFIG_INFINIBAND_USER_MAD) +=	ib_umad.o
 obj-$(CONFIG_INFINIBAND_USER_ACCESS) +=	ib_uverbs.o ib_ucm.o
 
@@ -12,6 +12,8 @@ ib_sa-y :=			sa_query.o
 
 ib_cm-y :=			cm.o
 
+ib_addr-y :=			addr.o
+
 ib_umad-y :=			user_mad.o
 
 ib_ucm-y :=			ucm.o
diff -uprN -X linux-2.6.git/Documentation/dontdiff 
linux-2.6.git/include/rdma/ib_addr.h 
linux-2.6.ib/include/rdma/ib_addr.h
--- linux-2.6.git/include/rdma/ib_addr.h	1969-12-31 16:00:00.000000000 -0800
+++ linux-2.6.ib/include/rdma/ib_addr.h	2006-01-16 16:14:24.000000000 -0800
@@ -0,0 +1,117 @@
+/*
+ * Copyright (c) 2005 Voltaire Inc.  All rights reserved.
+ * Copyright (c) 2005 Intel Corporation.  All rights reserved.
+ *
+ * This Software is licensed under one of the following licenses:
+ *
+ * 1) under the terms of the "Common Public License 1.0" a copy of which is
+ *    available from the Open Source Initiative, see
+ *    http://www.opensource.org/licenses/cpl.php.
+ *
+ * 2) under the terms of the "The BSD License" a copy of which is
+ *    available from the Open Source Initiative, see
+ *    http://www.opensource.org/licenses/bsd-license.php.
+ *
+ * 3) under the terms of the "GNU General Public License (GPL) Version 2" a
+ *    copy of which is available from the Open Source Initiative, see
+ *    http://www.opensource.org/licenses/gpl-license.php.
+ *
+ * Licensee has the right to choose one of the above licenses.
+ *
+ * Redistributions of source code must retain the above copyright
+ * notice and one of the license notices.
+ *
+ * Redistributions in binary form must reproduce both the above copyright
+ * notice, one of the license notices in the documentation
+ * and/or other materials provided with the distribution.
+ *
+ */
+
+#if !defined(IB_ADDR_H)
+#define IB_ADDR_H
+
+#include <linux/in.h>
+#include <linux/in6.h>
+#include <linux/netdevice.h>
+#include <linux/socket.h>
+#include <rdma/ib_verbs.h>
+
+extern struct workqueue_struct *rdma_wq;
+
+struct rdma_dev_addr {
+	unsigned char src_dev_addr[MAX_ADDR_LEN];
+	unsigned char dst_dev_addr[MAX_ADDR_LEN];
+	unsigned char broadcast[MAX_ADDR_LEN];
+	enum ib_node_type dev_type;
+};
+
+/**
+ * rdma_translate_ip - Translate a local IP address to an RDMA hardware
+ *   address.
+ */
+int rdma_translate_ip(struct sockaddr *addr, struct rdma_dev_addr *dev_addr);
+
+/**
+ * rdma_resolve_ip - Resolve source and destination IP addresses to
+ *   RDMA hardware addresses.
+ * @src_addr: An optional source address to use in the resolution.  If a
+ *   source address is not provided, a usable address will be returned via
+ *   the callback.
+ * @dst_addr: The destination address to resolve.
+ * @addr: A reference to a data location that will receive the resolved
+ *   addresses.  The data location must remain valid until the callback has
+ *   been invoked.
+ * @timeout_ms: Amount of time to wait for the address resolution to complete.
+ * @callback: Call invoked once address resolution has completed, timed out,
+ *   or been canceled.  A status of 0 indicates success.
+ * @context: User-specified context associated with the call.
+ */
+int rdma_resolve_ip(struct sockaddr *src_addr, struct sockaddr *dst_addr,
+		    struct rdma_dev_addr *addr, int timeout_ms,
+		    void (*callback)(int status, struct sockaddr *src_addr,
+				     struct rdma_dev_addr *addr, void *context),
+		    void *context);
+
+void rdma_addr_cancel(struct rdma_dev_addr *addr);
+
+static inline int ip_addr_size(struct sockaddr *addr)
+{
+	return addr->sa_family == AF_INET6 ?
+	       sizeof(struct sockaddr_in6) : sizeof(struct sockaddr_in);
+}
+
+static inline u16 ib_addr_get_pkey(struct rdma_dev_addr *dev_addr)
+{
+	return ((u16)dev_addr->broadcast[8] << 8) | (u16)dev_addr->broadcast[9];
+}
+
+static inline void ib_addr_set_pkey(struct rdma_dev_addr *dev_addr, u16 pkey)
+{
+	dev_addr->broadcast[8] = pkey >> 8;
+	dev_addr->broadcast[9] = (unsigned char) pkey;
+}
+
+static inline union ib_gid* ib_addr_get_sgid(struct rdma_dev_addr *dev_addr)
+{
+	return 	(union ib_gid *) (dev_addr->src_dev_addr + 4);
+}
+
+static inline void ib_addr_set_sgid(struct rdma_dev_addr *dev_addr,
+				    union ib_gid *gid)
+{
+	memcpy(dev_addr->src_dev_addr + 4, gid, sizeof *gid);
+}
+
+static inline union ib_gid* ib_addr_get_dgid(struct rdma_dev_addr *dev_addr)
+{
+	return 	(union ib_gid *) (dev_addr->dst_dev_addr + 4);
+}
+
+static inline void ib_addr_set_dgid(struct rdma_dev_addr *dev_addr,
+				    union ib_gid *gid)
+{
+	memcpy(dev_addr->dst_dev_addr + 4, gid, sizeof *gid);
+}
+
+#endif /* IB_ADDR_H */
+

^ permalink raw reply

* [PATCH 3/6] net/IB: export ip_dev_find
From: Sean Hefty @ 2006-03-06 19:07 UTC (permalink / raw)
  To: 'Roland Dreier', linux-kernel, netdev; +Cc: openib-general
In-Reply-To: <adaslpz2l9p.fsf@cisco.com>

Export ip_dev_find to allow locating a net_device given an IP address.

Signed-off-by: Sean Hefty <sean.hefty@intel.com>

---

diff -uprN -X linux-2.6.git/Documentation/dontdiff 
linux-2.6.git/net/ipv4/fib_frontend.c 
linux-2.6.ib/net/ipv4/fib_frontend.c
--- linux-2.6.git/net/ipv4/fib_frontend.c	2006-01-16 10:28:29.000000000 -0800
+++ linux-2.6.ib/net/ipv4/fib_frontend.c	2006-01-16 16:14:24.000000000 -0800
@@ -666,4 +666,5 @@ void __init ip_fib_init(void)
 }
 
 EXPORT_SYMBOL(inet_addr_type);
+EXPORT_SYMBOL(ip_dev_find);
 EXPORT_SYMBOL(ip_rt_ioctl);

^ permalink raw reply

* RFC: move SDP from AF_INET_SDP to IPPROTO_SDP
From: Michael S. Tsirkin @ 2006-03-06 19:06 UTC (permalink / raw)
  To: Linux Kernel Mailing List, netdev

Hi!
I am working on implementing the Sockets Direct Protocol (SDP) for InfiniBand on
Linux. SDP uses the regular IPv4/IPv6 addresses and utilizes the IPv4/IPv6 layer
on top of InfiniBand for address resolution. However, all data is transferred by
means of an infiniband reliable connection.

Some existing SDP implementations posted on the openib.org subversion tree
create a new address family in a free slot, for this purpose.

Would it make sense to move SDP from using a separate address family to
a separate protocol under AF_INET and AF_INET6?
Something like IPPROTO_SDP?

The main advantages of this approach are
- IPv6 support will come more naturally and without further extending
  to a yet another address family
- We could use a protocol number > 255 to avoid conflicting
  with any IP based protocol.
  There are much more free protocol numbers that free family numbers
  (which only go up to 32 in linux for now).
- I could reuse more code for creating connections from af_inet.c

I also have a hunch this might make getaddrinfo and friends work better on sdp
selecting IPv4/IPv6 as appropriate but I'm not sure.

Comments? Are there disadvantages to this approach that someone can see?

Thanks,

-- 
Michael S. Tsirkin
Staff Engineer, Mellanox Technologies
_______________________________________________
openib-general mailing list
openib-general@openib.org
http://openib.org/mailman/listinfo/openib-general

To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general

----- End forwarded message -----

-- 
Michael S. Tsirkin
Staff Engineer, Mellanox Technologies

^ permalink raw reply

* [PATCH 2/6] IB: match connection requests based on private data
From: Sean Hefty @ 2006-03-06 19:04 UTC (permalink / raw)
  To: 'Roland Dreier'; +Cc: netdev, linux-kernel, openib-general
In-Reply-To: <adaslpz2l9p.fsf@cisco.com>

Extend matching connection requests to listens in the Infiniband CM to include
private data checks.

This allows applications to listen on the same service identifier, with private
data directing the request to the appropriate application.

Signed-off-by: Sean Hefty <sean.hefty@intel.com>

---

diff -uprN -X linux-2.6.git/Documentation/dontdiff 
linux-2.6.git/drivers/infiniband/core/cm.c 
linux-2.6.ib/drivers/infiniband/core/cm.c
--- linux-2.6.git/drivers/infiniband/core/cm.c	2006-01-16 10:25:26.000000000 -0800
+++ linux-2.6.ib/drivers/infiniband/core/cm.c	2006-01-16 16:03:35.000000000 -0800
@@ -32,7 +32,7 @@
  * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
  * SOFTWARE.
  *
- * $Id: cm.c 2821 2005-07-08 17:07:28Z sean.hefty $
+ * $Id: cm.c 4311 2005-12-05 18:42:01Z sean.hefty $
  */
 #include <linux/dma-mapping.h>
 #include <linux/err.h>
@@ -130,6 +130,7 @@ struct cm_id_private {
 	/* todo: use alternate port on send failure */
 	struct cm_av av;
 	struct cm_av alt_av;
+	struct ib_cm_private_data_compare *compare_data;
 
 	void *private_data;
 	__be64 tid;
@@ -355,6 +356,40 @@ static struct cm_id_private * cm_acquire
 	return cm_id_priv;
 }
 
+static void cm_mask_compare_data(u8 *dst, u8 *src, u8 *mask)
+{
+	int i;
+
+	for (i = 0; i < IB_CM_PRIVATE_DATA_COMPARE_SIZE; i++)
+		dst[i] = src[i] & mask[i];
+}
+
+static int cm_compare_data(struct ib_cm_private_data_compare *src_data,
+			   struct ib_cm_private_data_compare *dst_data)
+{
+	u8 src[IB_CM_PRIVATE_DATA_COMPARE_SIZE];
+	u8 dst[IB_CM_PRIVATE_DATA_COMPARE_SIZE];
+
+	if (!src_data || !dst_data)
+		return 0;
+	
+	cm_mask_compare_data(src, src_data->data, dst_data->mask);
+	cm_mask_compare_data(dst, dst_data->data, src_data->mask);
+	return memcmp(src, dst, IB_CM_PRIVATE_DATA_COMPARE_SIZE);
+}
+
+static int cm_compare_private_data(u8 *private_data,
+				   struct ib_cm_private_data_compare *dst_data)
+{
+	u8 src[IB_CM_PRIVATE_DATA_COMPARE_SIZE];
+
+	if (!dst_data)
+		return 0;
+	
+	cm_mask_compare_data(src, private_data, dst_data->mask);
+	return memcmp(src, dst_data->data, IB_CM_PRIVATE_DATA_COMPARE_SIZE);
+}
+
 static struct cm_id_private * cm_insert_listen(struct cm_id_private *cm_id_priv)
 {
 	struct rb_node **link = &cm.listen_service_table.rb_node;
@@ -362,14 +397,18 @@ static struct cm_id_private * cm_insert_
 	struct cm_id_private *cur_cm_id_priv;
 	__be64 service_id = cm_id_priv->id.service_id;
 	__be64 service_mask = cm_id_priv->id.service_mask;
+	int data_cmp;
 
 	while (*link) {
 		parent = *link;
 		cur_cm_id_priv = rb_entry(parent, struct cm_id_private,
 					  service_node);
+		data_cmp = cm_compare_data(cm_id_priv->compare_data,
+					   cur_cm_id_priv->compare_data);
 		if ((cur_cm_id_priv->id.service_mask & service_id) ==
 		    (service_mask & cur_cm_id_priv->id.service_id) &&
-		    (cm_id_priv->id.device == cur_cm_id_priv->id.device))
+		    (cm_id_priv->id.device == cur_cm_id_priv->id.device) &&
+		    !data_cmp)
 			return cur_cm_id_priv;
 
 		if (cm_id_priv->id.device < cur_cm_id_priv->id.device)
@@ -378,6 +417,10 @@ static struct cm_id_private * cm_insert_
 			link = &(*link)->rb_right;
 		else if (service_id < cur_cm_id_priv->id.service_id)
 			link = &(*link)->rb_left;
+		else if (service_id > cur_cm_id_priv->id.service_id)
+			link = &(*link)->rb_right;
+		else if (data_cmp < 0)
+			link = &(*link)->rb_left;
 		else
 			link = &(*link)->rb_right;
 	}
@@ -387,16 +430,20 @@ static struct cm_id_private * cm_insert_
 }
 
 static struct cm_id_private * cm_find_listen(struct ib_device *device,
-					     __be64 service_id)
+					     __be64 service_id,
+					     u8 *private_data)
 {
 	struct rb_node *node = cm.listen_service_table.rb_node;
 	struct cm_id_private *cm_id_priv;
+	int data_cmp;
 
 	while (node) {
 		cm_id_priv = rb_entry(node, struct cm_id_private, service_node);
+		data_cmp = cm_compare_private_data(private_data,
+						   cm_id_priv->compare_data);
 		if ((cm_id_priv->id.service_mask & service_id) ==
 		     cm_id_priv->id.service_id &&
-		    (cm_id_priv->id.device == device))
+		    (cm_id_priv->id.device == device) && !data_cmp)
 			return cm_id_priv;
 
 		if (device < cm_id_priv->id.device)
@@ -405,6 +452,10 @@ static struct cm_id_private * cm_find_li
 			node = node->rb_right;
 		else if (service_id < cm_id_priv->id.service_id)
 			node = node->rb_left;
+		else if (service_id > cm_id_priv->id.service_id)
+			node = node->rb_right;
+		else if (data_cmp < 0)
+			node = node->rb_left;
 		else
 			node = node->rb_right;
 	}
@@ -728,15 +779,14 @@ retest:
 	wait_event(cm_id_priv->wait, !atomic_read(&cm_id_priv->refcount));
 	while ((work = cm_dequeue_work(cm_id_priv)) != NULL)
 		cm_free_work(work);
-	if (cm_id_priv->private_data && cm_id_priv->private_data_len)
-		kfree(cm_id_priv->private_data);
+	kfree(cm_id_priv->compare_data);
+	kfree(cm_id_priv->private_data);
 	kfree(cm_id_priv);
 }
 EXPORT_SYMBOL(ib_destroy_cm_id);
 
-int ib_cm_listen(struct ib_cm_id *cm_id,
-		 __be64 service_id,
-		 __be64 service_mask)
+int ib_cm_listen(struct ib_cm_id *cm_id, __be64 service_id, __be64 service_mask,
+		 struct ib_cm_private_data_compare *compare_data)
 {
 	struct cm_id_private *cm_id_priv, *cur_cm_id_priv;
 	unsigned long flags;
@@ -750,7 +800,19 @@ int ib_cm_listen(struct ib_cm_id *cm_id,
 		return -EINVAL;
 
 	cm_id_priv = container_of(cm_id, struct cm_id_private, id);
-	BUG_ON(cm_id->state != IB_CM_IDLE);
+	if (cm_id->state != IB_CM_IDLE)
+		return -EINVAL;
+
+	if (compare_data) {
+		cm_id_priv->compare_data = kzalloc(sizeof *compare_data,
+						   GFP_KERNEL);
+		if (!cm_id_priv->compare_data)
+			return -ENOMEM;
+		cm_mask_compare_data(cm_id_priv->compare_data->data,
+				     compare_data->data, compare_data->mask);
+		memcpy(cm_id_priv->compare_data->mask, compare_data->mask,
+		       IB_CM_PRIVATE_DATA_COMPARE_SIZE);
+	}
 
 	cm_id->state = IB_CM_LISTEN;
 
@@ -767,6 +829,8 @@ int ib_cm_listen(struct ib_cm_id *cm_id,
 
 	if (cur_cm_id_priv) {
 		cm_id->state = IB_CM_IDLE;
+		kfree(cm_id_priv->compare_data);
+		cm_id_priv->compare_data = NULL;
 		ret = -EBUSY;
 	}
 	return ret;
@@ -1239,7 +1303,8 @@ static struct cm_id_private * cm_match_r
 
 	/* Find matching listen request. */
 	listen_cm_id_priv = cm_find_listen(cm_id_priv->id.device,
-					   req_msg->service_id);
+					   req_msg->service_id,
+					   req_msg->private_data);
 	if (!listen_cm_id_priv) {
 		spin_unlock_irqrestore(&cm.lock, flags);
 		cm_issue_rej(work->port, work->mad_recv_wc,
@@ -2646,7 +2711,8 @@ static int cm_sidr_req_handler(struct cm
 		goto out; /* Duplicate message. */
 	}
 	cur_cm_id_priv = cm_find_listen(cm_id->device,
-					sidr_req_msg->service_id);
+					sidr_req_msg->service_id,
+					sidr_req_msg->private_data);
 	if (!cur_cm_id_priv) {
 		rb_erase(&cm_id_priv->sidr_id_node, &cm.remote_sidr_table);
 		spin_unlock_irqrestore(&cm.lock, flags);
diff -uprN -X linux-2.6.git/Documentation/dontdiff 
linux-2.6.git/drivers/infiniband/core/ucm.c 
linux-2.6.ib/drivers/infiniband/core/ucm.c
--- linux-2.6.git/drivers/infiniband/core/ucm.c	2006-01-16 16:03:08.000000000 -0800
+++ linux-2.6.ib/drivers/infiniband/core/ucm.c	2006-01-16 16:03:35.000000000 -0800
@@ -646,6 +646,17 @@ out:
 	return result;
 }
 
+static int ucm_validate_listen(__be64 service_id, __be64 service_mask)
+{
+	service_id &= service_mask;
+
+	if (((service_id & IB_CMA_SERVICE_ID_MASK) == IB_CMA_SERVICE_ID) ||
+	    ((service_id & IB_SDP_SERVICE_ID_MASK) == IB_SDP_SERVICE_ID))
+		return -EINVAL;
+
+	return 0;
+}
+
 static ssize_t ib_ucm_listen(struct ib_ucm_file *file,
 			     const char __user *inbuf,
 			     int in_len, int out_len)
@@ -661,7 +672,13 @@ static ssize_t ib_ucm_listen(struct ib_u
 	if (IS_ERR(ctx))
 		return PTR_ERR(ctx);
 
-	result = ib_cm_listen(ctx->cm_id, cmd.service_id, cmd.service_mask);
+	result = ucm_validate_listen(cmd.service_id, cmd.service_mask);
+	if (result)
+		goto out;
+
+	result = ib_cm_listen(ctx->cm_id, cmd.service_id, cmd.service_mask,
+			      NULL);
+out:
 	ib_ucm_ctx_put(ctx);
 	return result;
 }
diff -uprN -X linux-2.6.git/Documentation/dontdiff 
linux-2.6.git/include/rdma/ib_cm.h 
linux-2.6.ib/include/rdma/ib_cm.h
--- linux-2.6.git/include/rdma/ib_cm.h	2006-01-16 10:26:47.000000000 -0800
+++ linux-2.6.ib/include/rdma/ib_cm.h	2006-01-16 16:03:35.000000000 -0800
@@ -32,7 +32,7 @@
  * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
  * SOFTWARE.
  *
- * $Id: ib_cm.h 2730 2005-06-28 16:43:03Z sean.hefty $
+ * $Id: ib_cm.h 4311 2005-12-05 18:42:01Z sean.hefty $
  */
 #if !defined(IB_CM_H)
 #define IB_CM_H
@@ -102,7 +102,8 @@ enum ib_cm_data_size {
 	IB_CM_APR_INFO_LENGTH		 = 72,
 	IB_CM_SIDR_REQ_PRIVATE_DATA_SIZE = 216,
 	IB_CM_SIDR_REP_PRIVATE_DATA_SIZE = 136,
-	IB_CM_SIDR_REP_INFO_LENGTH	 = 72
+	IB_CM_SIDR_REP_INFO_LENGTH	 = 72,
+	IB_CM_PRIVATE_DATA_COMPARE_SIZE	 = 64
 };
 
 struct ib_cm_id;
@@ -238,7 +239,6 @@ struct ib_cm_sidr_rep_event_param {
 	u32			qpn;
 	void			*info;
 	u8			info_len;
-
 };
 
 struct ib_cm_event {
@@ -317,6 +317,15 @@ void ib_destroy_cm_id(struct ib_cm_id *c
 
 #define IB_SERVICE_ID_AGN_MASK	__constant_cpu_to_be64(0xFF00000000000000ULL)
 #define IB_CM_ASSIGN_SERVICE_ID __constant_cpu_to_be64(0x0200000000000000ULL)
+#define IB_CMA_SERVICE_ID	__constant_cpu_to_be64(0x0000000001000000ULL)
+#define IB_CMA_SERVICE_ID_MASK	__constant_cpu_to_be64(0xFFFFFFFFFF000000ULL)
+#define IB_SDP_SERVICE_ID	__constant_cpu_to_be64(0x0000000000010000ULL)
+#define IB_SDP_SERVICE_ID_MASK	__constant_cpu_to_be64(0xFFFFFFFFFFFF0000ULL)
+
+struct ib_cm_private_data_compare {
+	u8  data[IB_CM_PRIVATE_DATA_COMPARE_SIZE];
+	u8  mask[IB_CM_PRIVATE_DATA_COMPARE_SIZE];
+};
 
 /**
  * ib_cm_listen - Initiates listening on the specified service ID for
@@ -330,10 +339,12 @@ void ib_destroy_cm_id(struct ib_cm_id *c
  *   range of service IDs.  If set to 0, the service ID is matched
  *   exactly.  This parameter is ignored if %service_id is set to
  *   IB_CM_ASSIGN_SERVICE_ID.
+ * @compare_data: This parameter is optional.  It specifies data that must
+ *   appear in the private data of a connection request for the specified
+ *   listen request.
  */
-int ib_cm_listen(struct ib_cm_id *cm_id,
-		 __be64 service_id,
-		 __be64 service_mask);
+int ib_cm_listen(struct ib_cm_id *cm_id, __be64 service_id, __be64 service_mask,
+		 struct ib_cm_private_data_compare *compare_data);
 
 struct ib_cm_req_param {
 	struct ib_sa_path_rec	*primary_path;

^ permalink raw reply

* [PATCH 1/6] IB: common handling for marshalling parameters to/from userspace
From: Sean Hefty @ 2006-03-06 18:59 UTC (permalink / raw)
  To: 'Roland Dreier'; +Cc: netdev, linux-kernel, openib-general
In-Reply-To: <adaslpz2l9p.fsf@cisco.com>


Provide common handling for marshalling data between userspace clients
and kernel mode Infiniband drivers.

Signed-off-by: Sean Hefty <sean.hefty@intel.com>

---

diff -uprN -X linux-2.6.git/Documentation/dontdiff 
linux-2.6.git/drivers/infiniband/core/Makefile 
linux-2.6.ib/drivers/infiniband/core/Makefile
--- linux-2.6.git/drivers/infiniband/core/Makefile	2006-01-16 10:25:27.000000000 -0800
+++ linux-2.6.ib/drivers/infiniband/core/Makefile	2006-01-16 15:34:15.000000000 -0800
@@ -16,4 +16,5 @@ ib_umad-y :=			user_mad.o
 
 ib_ucm-y :=			ucm.o
 
-ib_uverbs-y :=			uverbs_main.o uverbs_cmd.o uverbs_mem.o
+ib_uverbs-y :=			uverbs_main.o uverbs_cmd.o uverbs_mem.o \
+				uverbs_marshall.o
diff -uprN -X linux-2.6.git/Documentation/dontdiff 
linux-2.6.git/drivers/infiniband/core/ucm.c 
linux-2.6.ib/drivers/infiniband/core/ucm.c
--- linux-2.6.git/drivers/infiniband/core/ucm.c	2006-01-16 10:25:26.000000000 -0800
+++ linux-2.6.ib/drivers/infiniband/core/ucm.c	2006-01-16 15:34:15.000000000 -0800
@@ -30,7 +30,7 @@
  * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
  * SOFTWARE.
  *
- * $Id: ucm.c 2594 2005-06-13 19:46:02Z libor $
+ * $Id: ucm.c 4311 2005-12-05 18:42:01Z sean.hefty $
  */
 #include <linux/init.h>
 #include <linux/fs.h>
@@ -48,6 +48,7 @@
 
 #include <rdma/ib_cm.h>
 #include <rdma/ib_user_cm.h>
+#include <rdma/ib_marshall.h>
 
 MODULE_AUTHOR("Libor Michalek");
 MODULE_DESCRIPTION("InfiniBand userspace Connection Manager access");
@@ -203,36 +204,6 @@ error:
 	return NULL;
 }
 
-static void ib_ucm_event_path_get(struct ib_ucm_path_rec *upath,
-				  struct ib_sa_path_rec	 *kpath)
-{
-	if (!kpath || !upath)
-		return;
-
-	memcpy(upath->dgid, kpath->dgid.raw, sizeof *upath->dgid);
-	memcpy(upath->sgid, kpath->sgid.raw, sizeof *upath->sgid);
-
-	upath->dlid             = kpath->dlid;
-	upath->slid             = kpath->slid;
-	upath->raw_traffic      = kpath->raw_traffic;
-	upath->flow_label       = kpath->flow_label;
-	upath->hop_limit        = kpath->hop_limit;
-	upath->traffic_class    = kpath->traffic_class;
-	upath->reversible       = kpath->reversible;
-	upath->numb_path        = kpath->numb_path;
-	upath->pkey             = kpath->pkey;
-	upath->sl	        = kpath->sl;
-	upath->mtu_selector     = kpath->mtu_selector;
-	upath->mtu              = kpath->mtu;
-	upath->rate_selector    = kpath->rate_selector;
-	upath->rate             = kpath->rate;
-	upath->packet_life_time = kpath->packet_life_time;
-	upath->preference       = kpath->preference;
-
-	upath->packet_life_time_selector =
-		kpath->packet_life_time_selector;
-}
-
 static void ib_ucm_event_req_get(struct ib_ucm_req_event_resp *ureq,
 				 struct ib_cm_req_event_param *kreq)
 {
@@ -251,8 +222,10 @@ static void ib_ucm_event_req_get(struct 
 	ureq->srq                        = kreq->srq;
 	ureq->port			 = kreq->port;
 
-	ib_ucm_event_path_get(&ureq->primary_path, kreq->primary_path);
-	ib_ucm_event_path_get(&ureq->alternate_path, kreq->alternate_path);
+	ib_copy_path_rec_to_user(&ureq->primary_path, kreq->primary_path);
+	if (kreq->alternate_path)
+		ib_copy_path_rec_to_user(&ureq->alternate_path,
+					 kreq->alternate_path);
 }
 
 static void ib_ucm_event_rep_get(struct ib_ucm_rep_event_resp *urep,
@@ -322,8 +295,8 @@ static int ib_ucm_event_process(struct i
 		info	      = evt->param.rej_rcvd.ari;
 		break;
 	case IB_CM_LAP_RECEIVED:
-		ib_ucm_event_path_get(&uvt->resp.u.lap_resp.path,
-				      evt->param.lap_rcvd.alternate_path);
+		ib_copy_path_rec_to_user(&uvt->resp.u.lap_resp.path,
+					 evt->param.lap_rcvd.alternate_path);
 		uvt->data_len = IB_CM_LAP_PRIVATE_DATA_SIZE;
 		uvt->resp.present = IB_UCM_PRES_ALTERNATE;
 		break;
@@ -635,65 +608,11 @@ static ssize_t ib_ucm_attr_id(struct ib_
 	return result;
 }
 
-static void ib_ucm_copy_ah_attr(struct ib_ucm_ah_attr *dest_attr,
-				struct ib_ah_attr *src_attr)
-{
-	memcpy(dest_attr->grh_dgid, src_attr->grh.dgid.raw,
-	       sizeof src_attr->grh.dgid);
-	dest_attr->grh_flow_label = src_attr->grh.flow_label;
-	dest_attr->grh_sgid_index = src_attr->grh.sgid_index;
-	dest_attr->grh_hop_limit = src_attr->grh.hop_limit;
-	dest_attr->grh_traffic_class = src_attr->grh.traffic_class;
-
-	dest_attr->dlid = src_attr->dlid;
-	dest_attr->sl = src_attr->sl;
-	dest_attr->src_path_bits = src_attr->src_path_bits;
-	dest_attr->static_rate = src_attr->static_rate;
-	dest_attr->is_global = (src_attr->ah_flags & IB_AH_GRH);
-	dest_attr->port_num = src_attr->port_num;
-}
-
-static void ib_ucm_copy_qp_attr(struct ib_ucm_init_qp_attr_resp *dest_attr,
-				struct ib_qp_attr *src_attr)
-{
-	dest_attr->cur_qp_state = src_attr->cur_qp_state;
-	dest_attr->path_mtu = src_attr->path_mtu;
-	dest_attr->path_mig_state = src_attr->path_mig_state;
-	dest_attr->qkey = src_attr->qkey;
-	dest_attr->rq_psn = src_attr->rq_psn;
-	dest_attr->sq_psn = src_attr->sq_psn;
-	dest_attr->dest_qp_num = src_attr->dest_qp_num;
-	dest_attr->qp_access_flags = src_attr->qp_access_flags;
-
-	dest_attr->max_send_wr = src_attr->cap.max_send_wr;
-	dest_attr->max_recv_wr = src_attr->cap.max_recv_wr;
-	dest_attr->max_send_sge = src_attr->cap.max_send_sge;
-	dest_attr->max_recv_sge = src_attr->cap.max_recv_sge;
-	dest_attr->max_inline_data = src_attr->cap.max_inline_data;
-
-	ib_ucm_copy_ah_attr(&dest_attr->ah_attr, &src_attr->ah_attr);
-	ib_ucm_copy_ah_attr(&dest_attr->alt_ah_attr, &src_attr->alt_ah_attr);
-
-	dest_attr->pkey_index = src_attr->pkey_index;
-	dest_attr->alt_pkey_index = src_attr->alt_pkey_index;
-	dest_attr->en_sqd_async_notify = src_attr->en_sqd_async_notify;
-	dest_attr->sq_draining = src_attr->sq_draining;
-	dest_attr->max_rd_atomic = src_attr->max_rd_atomic;
-	dest_attr->max_dest_rd_atomic = src_attr->max_dest_rd_atomic;
-	dest_attr->min_rnr_timer = src_attr->min_rnr_timer;
-	dest_attr->port_num = src_attr->port_num;
-	dest_attr->timeout = src_attr->timeout;
-	dest_attr->retry_cnt = src_attr->retry_cnt;
-	dest_attr->rnr_retry = src_attr->rnr_retry;
-	dest_attr->alt_port_num = src_attr->alt_port_num;
-	dest_attr->alt_timeout = src_attr->alt_timeout;
-}
-
 static ssize_t ib_ucm_init_qp_attr(struct ib_ucm_file *file,
 				   const char __user *inbuf,
 				   int in_len, int out_len)
 {
-	struct ib_ucm_init_qp_attr_resp resp;
+	struct ib_uverbs_qp_attr resp;
 	struct ib_ucm_init_qp_attr cmd;
 	struct ib_ucm_context *ctx;
 	struct ib_qp_attr qp_attr;
@@ -716,7 +635,7 @@ static ssize_t ib_ucm_init_qp_attr(struc
 	if (result)
 		goto out;
 
-	ib_ucm_copy_qp_attr(&resp, &qp_attr);
+	ib_copy_qp_attr_to_user(&resp, &qp_attr);
 
 	if (copy_to_user((void __user *)(unsigned long)cmd.response,
 			 &resp, sizeof(resp)))
@@ -791,7 +710,7 @@ static int ib_ucm_alloc_data(const void 
 
 static int ib_ucm_path_get(struct ib_sa_path_rec **path, u64 src)
 {
-	struct ib_ucm_path_rec ucm_path;
+	struct ib_user_path_rec upath;
 	struct ib_sa_path_rec  *sa_path;
 
 	*path = NULL;
@@ -803,36 +722,14 @@ static int ib_ucm_path_get(struct ib_sa_
 	if (!sa_path)
 		return -ENOMEM;
 
-	if (copy_from_user(&ucm_path, (void __user *)(unsigned long)src,
-			   sizeof(ucm_path))) {
+	if (copy_from_user(&upath, (void __user *)(unsigned long)src,
+			   sizeof(upath))) {
 
 		kfree(sa_path);
 		return -EFAULT;
 	}
 
-	memcpy(sa_path->dgid.raw, ucm_path.dgid, sizeof sa_path->dgid);
-	memcpy(sa_path->sgid.raw, ucm_path.sgid, sizeof sa_path->sgid);
-
-	sa_path->dlid	          = ucm_path.dlid;
-	sa_path->slid	          = ucm_path.slid;
-	sa_path->raw_traffic      = ucm_path.raw_traffic;
-	sa_path->flow_label       = ucm_path.flow_label;
-	sa_path->hop_limit        = ucm_path.hop_limit;
-	sa_path->traffic_class    = ucm_path.traffic_class;
-	sa_path->reversible       = ucm_path.reversible;
-	sa_path->numb_path        = ucm_path.numb_path;
-	sa_path->pkey             = ucm_path.pkey;
-	sa_path->sl               = ucm_path.sl;
-	sa_path->mtu_selector     = ucm_path.mtu_selector;
-	sa_path->mtu              = ucm_path.mtu;
-	sa_path->rate_selector    = ucm_path.rate_selector;
-	sa_path->rate             = ucm_path.rate;
-	sa_path->packet_life_time = ucm_path.packet_life_time;
-	sa_path->preference       = ucm_path.preference;
-
-	sa_path->packet_life_time_selector =
-		ucm_path.packet_life_time_selector;
-
+	ib_copy_path_rec_from_user(sa_path, &upath);
 	*path = sa_path;
 	return 0;
 }
@@ -1243,8 +1140,10 @@ static unsigned int ib_ucm_poll(struct f
 
 	poll_wait(filp, &file->poll_wait, wait);
 
+	down(&file->mutex);
 	if (!list_empty(&file->events))
 		mask = POLLIN | POLLRDNORM;
+	up(&file->mutex);
 
 	return mask;
 }
diff -uprN -X linux-2.6.git/Documentation/dontdiff 
linux-2.6.git/drivers/infiniband/core/uverbs_marshall.c 
linux-2.6.ib/drivers/infiniband/core/uverbs_marshall.c
--- linux-2.6.git/drivers/infiniband/core/uverbs_marshall.c	1969-12-31 16:00:00.000000000 -0800
+++ linux-2.6.ib/drivers/infiniband/core/uverbs_marshall.c	2006-01-16 15:34:15.000000000 -0800
@@ -0,0 +1,138 @@
+/*
+ * Copyright (c) 2005 Intel Corporation.  All rights reserved.
+ *
+ * This software is available to you under a choice of one of two
+ * licenses.  You may choose to be licensed under the terms of the GNU
+ * General Public License (GPL) Version 2, available from the file
+ * COPYING in the main directory of this source tree, or the
+ * OpenIB.org BSD license below:
+ *
+ *     Redistribution and use in source and binary forms, with or
+ *     without modification, are permitted provided that the following
+ *     conditions are met:
+ *
+ *      - Redistributions of source code must retain the above
+ *        copyright notice, this list of conditions and the following
+ *        disclaimer.
+ *
+ *      - Redistributions in binary form must reproduce the above
+ *        copyright notice, this list of conditions and the following
+ *        disclaimer in the documentation and/or other materials
+ *        provided with the distribution.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+#include <rdma/ib_marshall.h>
+
+static void ib_copy_ah_attr_to_user(struct ib_uverbs_ah_attr *dst,
+				    struct ib_ah_attr *src)
+{
+	memcpy(dst->grh.dgid, src->grh.dgid.raw, sizeof src->grh.dgid);
+	dst->grh.flow_label        = src->grh.flow_label;
+	dst->grh.sgid_index        = src->grh.sgid_index;
+	dst->grh.hop_limit         = src->grh.hop_limit;
+	dst->grh.traffic_class     = src->grh.traffic_class;
+	dst->dlid 	    	   = src->dlid;
+	dst->sl   	    	   = src->sl;
+	dst->src_path_bits 	   = src->src_path_bits;
+	dst->static_rate   	   = src->static_rate;
+	dst->is_global             = src->ah_flags & IB_AH_GRH ? 1 : 0;
+	dst->port_num 	    	   = src->port_num;
+}
+
+void ib_copy_qp_attr_to_user(struct ib_uverbs_qp_attr *dst,
+			     struct ib_qp_attr *src)
+{
+	dst->cur_qp_state	= src->cur_qp_state;
+	dst->path_mtu		= src->path_mtu;
+	dst->path_mig_state	= src->path_mig_state;
+	dst->qkey		= src->qkey;
+	dst->rq_psn		= src->rq_psn;
+	dst->sq_psn		= src->sq_psn;
+	dst->dest_qp_num	= src->dest_qp_num;
+	dst->qp_access_flags	= src->qp_access_flags;
+
+	dst->max_send_wr	= src->cap.max_send_wr;
+	dst->max_recv_wr	= src->cap.max_recv_wr;
+	dst->max_send_sge	= src->cap.max_send_sge;
+	dst->max_recv_sge	= src->cap.max_recv_sge;
+	dst->max_inline_data	= src->cap.max_inline_data;
+
+	ib_copy_ah_attr_to_user(&dst->ah_attr, &src->ah_attr);
+	ib_copy_ah_attr_to_user(&dst->alt_ah_attr, &src->alt_ah_attr);
+
+	dst->pkey_index		= src->pkey_index;
+	dst->alt_pkey_index	= src->alt_pkey_index;
+	dst->en_sqd_async_notify = src->en_sqd_async_notify;
+	dst->sq_draining	= src->sq_draining;
+	dst->max_rd_atomic	= src->max_rd_atomic;
+	dst->max_dest_rd_atomic	= src->max_dest_rd_atomic;
+	dst->min_rnr_timer	= src->min_rnr_timer;
+	dst->port_num		= src->port_num;
+	dst->timeout		= src->timeout;
+	dst->retry_cnt		= src->retry_cnt;
+	dst->rnr_retry		= src->rnr_retry;
+	dst->alt_port_num	= src->alt_port_num;
+	dst->alt_timeout	= src->alt_timeout;
+}
+EXPORT_SYMBOL(ib_copy_qp_attr_to_user);
+
+void ib_copy_path_rec_to_user(struct ib_user_path_rec *dst,
+			      struct ib_sa_path_rec *src)
+{
+	memcpy(dst->dgid, src->dgid.raw, sizeof src->dgid);
+	memcpy(dst->sgid, src->sgid.raw, sizeof src->sgid);
+
+	dst->dlid		= src->dlid;
+	dst->slid		= src->slid;
+	dst->raw_traffic	= src->raw_traffic;
+	dst->flow_label		= src->flow_label;
+	dst->hop_limit		= src->hop_limit;
+	dst->traffic_class	= src->traffic_class;
+	dst->reversible		= src->reversible;
+	dst->numb_path		= src->numb_path;
+	dst->pkey		= src->pkey;
+	dst->sl			= src->sl;
+	dst->mtu_selector	= src->mtu_selector;
+	dst->mtu		= src->mtu;
+	dst->rate_selector	= src->rate_selector;
+	dst->rate		= src->rate;
+	dst->packet_life_time	= src->packet_life_time;
+	dst->preference		= src->preference;
+	dst->packet_life_time_selector = src->packet_life_time_selector;
+}
+EXPORT_SYMBOL(ib_copy_path_rec_to_user);
+
+void ib_copy_path_rec_from_user(struct ib_sa_path_rec *dst,
+				struct ib_user_path_rec *src)
+{
+	memcpy(dst->dgid.raw, src->dgid, sizeof dst->dgid);
+	memcpy(dst->sgid.raw, src->sgid, sizeof dst->sgid);
+
+	dst->dlid		= src->dlid;
+	dst->slid		= src->slid;
+	dst->raw_traffic	= src->raw_traffic;
+	dst->flow_label		= src->flow_label;
+	dst->hop_limit		= src->hop_limit;
+	dst->traffic_class	= src->traffic_class;
+	dst->reversible		= src->reversible;
+	dst->numb_path		= src->numb_path;
+	dst->pkey		= src->pkey;
+	dst->sl			= src->sl;
+	dst->mtu_selector	= src->mtu_selector;
+	dst->mtu		= src->mtu;
+	dst->rate_selector	= src->rate_selector;
+	dst->rate		= src->rate;
+	dst->packet_life_time	= src->packet_life_time;
+	dst->preference		= src->preference;
+	dst->packet_life_time_selector = src->packet_life_time_selector;
+}
+EXPORT_SYMBOL(ib_copy_path_rec_from_user);
diff -uprN -X linux-2.6.git/Documentation/dontdiff 
linux-2.6.git/include/rdma/ib_marshall.h 
linux-2.6.ib/include/rdma/ib_marshall.h
--- linux-2.6.git/include/rdma/ib_marshall.h	1969-12-31 16:00:00.000000000 -0800
+++ linux-2.6.ib/include/rdma/ib_marshall.h	2006-01-16 15:34:15.000000000 -0800
@@ -0,0 +1,50 @@
+/*
+ * Copyright (c) 2005 Intel Corporation.  All rights reserved.
+ *
+ * This software is available to you under a choice of one of two
+ * licenses.  You may choose to be licensed under the terms of the GNU
+ * General Public License (GPL) Version 2, available from the file
+ * COPYING in the main directory of this source tree, or the
+ * OpenIB.org BSD license below:
+ *
+ *     Redistribution and use in source and binary forms, with or
+ *     without modification, are permitted provided that the following
+ *     conditions are met:
+ *
+ *      - Redistributions of source code must retain the above
+ *        copyright notice, this list of conditions and the following
+ *        disclaimer.
+ *
+ *      - Redistributions in binary form must reproduce the above
+ *        copyright notice, this list of conditions and the following
+ *        disclaimer in the documentation and/or other materials
+ *        provided with the distribution.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+#if !defined(IB_USER_MARSHALL_H)
+#define IB_USER_MARSHALL_H
+
+#include <rdma/ib_verbs.h>
+#include <rdma/ib_sa.h>
+#include <rdma/ib_user_verbs.h>
+#include <rdma/ib_user_sa.h>
+
+void ib_copy_qp_attr_to_user(struct ib_uverbs_qp_attr *dst,
+			     struct ib_qp_attr *src);
+
+void ib_copy_path_rec_to_user(struct ib_user_path_rec *dst,
+			      struct ib_sa_path_rec *src);
+
+void ib_copy_path_rec_from_user(struct ib_sa_path_rec *dst,
+				struct ib_user_path_rec *src);
+
+#endif /* IB_USER_MARSHALL_H */
diff -uprN -X linux-2.6.git/Documentation/dontdiff 
linux-2.6.git/include/rdma/ib_user_cm.h 
linux-2.6.ib/include/rdma/ib_user_cm.h
--- linux-2.6.git/include/rdma/ib_user_cm.h	2006-01-16 10:26:47.000000000 -0800
+++ linux-2.6.ib/include/rdma/ib_user_cm.h	2006-01-16 15:34:15.000000000 -0800
@@ -30,13 +30,13 @@
  * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
  * SOFTWARE.
  *
- * $Id: ib_user_cm.h 2576 2005-06-09 17:00:30Z libor $
+ * $Id: ib_user_cm.h 4019 2005-11-11 00:33:09Z sean.hefty $
  */
 
 #ifndef IB_USER_CM_H
 #define IB_USER_CM_H
 
-#include <linux/types.h>
+#include <rdma/ib_user_sa.h>
 
 #define IB_USER_CM_ABI_VERSION 4
 
@@ -110,58 +110,6 @@ struct ib_ucm_init_qp_attr {
 	__u32 qp_state;
 };
 
-struct ib_ucm_ah_attr {
-	__u8	grh_dgid[16];
-	__u32	grh_flow_label;
-	__u16	dlid;
-	__u16	reserved;
-	__u8	grh_sgid_index;
-	__u8	grh_hop_limit;
-	__u8	grh_traffic_class;
-	__u8	sl;
-	__u8	src_path_bits;
-	__u8	static_rate;
-	__u8	is_global;
-	__u8	port_num;
-};
-
-struct ib_ucm_init_qp_attr_resp {
-	__u32	qp_attr_mask;
-	__u32	qp_state;
-	__u32	cur_qp_state;
-	__u32	path_mtu;
-	__u32	path_mig_state;
-	__u32	qkey;
-	__u32	rq_psn;
-	__u32	sq_psn;
-	__u32	dest_qp_num;
-	__u32	qp_access_flags;
-
-	struct ib_ucm_ah_attr	ah_attr;
-	struct ib_ucm_ah_attr	alt_ah_attr;
-
-	/* ib_qp_cap */
-	__u32	max_send_wr;
-	__u32	max_recv_wr;
-	__u32	max_send_sge;
-	__u32	max_recv_sge;
-	__u32	max_inline_data;
-
-	__u16	pkey_index;
-	__u16	alt_pkey_index;
-	__u8	en_sqd_async_notify;
-	__u8	sq_draining;
-	__u8	max_rd_atomic;
-	__u8	max_dest_rd_atomic;
-	__u8	min_rnr_timer;
-	__u8	port_num;
-	__u8	timeout;
-	__u8	retry_cnt;
-	__u8	rnr_retry;
-	__u8	alt_port_num;
-	__u8	alt_timeout;
-};
-
 struct ib_ucm_listen {
 	__be64 service_id;
 	__be64 service_mask;
@@ -180,28 +128,6 @@ struct ib_ucm_private_data {
 	__u8  reserved[3];
 };
 
-struct ib_ucm_path_rec {
-	__u8  dgid[16];
-	__u8  sgid[16];
-	__be16 dlid;
-	__be16 slid;
-	__u32 raw_traffic;
-	__be32 flow_label;
-	__u32 reversible;
-	__u32 mtu;
-	__be16 pkey;
-	__u8  hop_limit;
-	__u8  traffic_class;
-	__u8  numb_path;
-	__u8  sl;
-	__u8  mtu_selector;
-	__u8  rate_selector;
-	__u8  rate;
-	__u8  packet_life_time_selector;
-	__u8  packet_life_time;
-	__u8  preference;
-};
-
 struct ib_ucm_req {
 	__u32 id;
 	__u32 qpn;
@@ -304,8 +230,8 @@ struct ib_ucm_event_get {
 };
 
 struct ib_ucm_req_event_resp {
-	struct ib_ucm_path_rec primary_path;
-	struct ib_ucm_path_rec alternate_path;
+	struct ib_user_path_rec primary_path;
+	struct ib_user_path_rec alternate_path;
 	__be64                 remote_ca_guid;
 	__u32                  remote_qkey;
 	__u32                  remote_qpn;
@@ -349,7 +275,7 @@ struct ib_ucm_mra_event_resp {
 };
 
 struct ib_ucm_lap_event_resp {
-	struct ib_ucm_path_rec path;
+	struct ib_user_path_rec path;
 };
 
 struct ib_ucm_apr_event_resp {
diff -uprN -X linux-2.6.git/Documentation/dontdiff 
linux-2.6.git/include/rdma/ib_user_sa.h 
linux-2.6.ib/include/rdma/ib_user_sa.h
--- linux-2.6.git/include/rdma/ib_user_sa.h	1969-12-31 16:00:00.000000000 -0800
+++ linux-2.6.ib/include/rdma/ib_user_sa.h	2006-01-16 15:34:15.000000000 -0800
@@ -0,0 +1,60 @@
+/*
+ * Copyright (c) 2005 Intel Corporation.  All rights reserved.
+ *
+ * This software is available to you under a choice of one of two
+ * licenses.  You may choose to be licensed under the terms of the GNU
+ * General Public License (GPL) Version 2, available from the file
+ * COPYING in the main directory of this source tree, or the
+ * OpenIB.org BSD license below:
+ *
+ *     Redistribution and use in source and binary forms, with or
+ *     without modification, are permitted provided that the following
+ *     conditions are met:
+ *
+ *      - Redistributions of source code must retain the above
+ *        copyright notice, this list of conditions and the following
+ *        disclaimer.
+ *
+ *      - Redistributions in binary form must reproduce the above
+ *        copyright notice, this list of conditions and the following
+ *        disclaimer in the documentation and/or other materials
+ *        provided with the distribution.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+#ifndef IB_USER_SA_H
+#define IB_USER_SA_H
+
+#include <linux/types.h>
+
+struct ib_user_path_rec {
+	__u8	dgid[16];
+	__u8	sgid[16];
+	__be16	dlid;
+	__be16	slid;
+	__u32	raw_traffic;
+	__be32	flow_label;
+	__u32	reversible;
+	__u32	mtu;
+	__be16	pkey;
+	__u8	hop_limit;
+	__u8	traffic_class;
+	__u8	numb_path;
+	__u8	sl;
+	__u8	mtu_selector;
+	__u8	rate_selector;
+	__u8	rate;
+	__u8	packet_life_time_selector;
+	__u8	packet_life_time;
+	__u8	preference;
+};
+
+#endif /* IB_USER_SA_H */
diff -uprN -X linux-2.6.git/Documentation/dontdiff 
linux-2.6.git/include/rdma/ib_user_verbs.h 
linux-2.6.ib/include/rdma/ib_user_verbs.h
--- linux-2.6.git/include/rdma/ib_user_verbs.h	2006-01-16 10:26:47.000000000 -0800
+++ linux-2.6.ib/include/rdma/ib_user_verbs.h	2006-01-16 15:34:15.000000000 -0800
@@ -31,7 +31,7 @@
  * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
  * SOFTWARE.
  *
- * $Id: ib_user_verbs.h 2708 2005-06-24 17:27:21Z roland $
+ * $Id: ib_user_verbs.h 4019 2005-11-11 00:33:09Z sean.hefty $
  */
 
 #ifndef IB_USER_VERBS_H
@@ -311,6 +311,64 @@ struct ib_uverbs_destroy_cq_resp {
 	__u32 async_events_reported;
 };
 
+struct ib_uverbs_global_route {
+	__u8  dgid[16];
+	__u32 flow_label;    
+	__u8  sgid_index;
+	__u8  hop_limit;
+	__u8  traffic_class;
+	__u8  reserved;
+};
+
+struct ib_uverbs_ah_attr {
+	struct ib_uverbs_global_route grh;
+	__u16 dlid;
+	__u8  sl;
+	__u8  src_path_bits;
+	__u8  static_rate;
+	__u8  is_global;
+	__u8  port_num;
+	__u8  reserved;
+};
+
+struct ib_uverbs_qp_attr {
+	__u32	qp_attr_mask;
+	__u32	qp_state;
+	__u32	cur_qp_state;
+	__u32	path_mtu;
+	__u32	path_mig_state;
+	__u32	qkey;
+	__u32	rq_psn;
+	__u32	sq_psn;
+	__u32	dest_qp_num;
+	__u32	qp_access_flags;
+
+	struct ib_uverbs_ah_attr ah_attr;
+	struct ib_uverbs_ah_attr alt_ah_attr;
+
+	/* ib_qp_cap */
+	__u32	max_send_wr;
+	__u32	max_recv_wr;
+	__u32	max_send_sge;
+	__u32	max_recv_sge;
+	__u32	max_inline_data;
+
+	__u16	pkey_index;
+	__u16	alt_pkey_index;
+	__u8	en_sqd_async_notify;
+	__u8	sq_draining;
+	__u8	max_rd_atomic;
+	__u8	max_dest_rd_atomic;
+	__u8	min_rnr_timer;
+	__u8	port_num;
+	__u8	timeout;
+	__u8	retry_cnt;
+	__u8	rnr_retry;
+	__u8	alt_port_num;
+	__u8	alt_timeout;
+	__u8	reserved[5];
+};
+
 struct ib_uverbs_create_qp {
 	__u64 response;
 	__u64 user_handle;
@@ -487,26 +545,6 @@ struct ib_uverbs_post_srq_recv_resp {
 	__u32 bad_wr;
 };
 
-struct ib_uverbs_global_route {
-	__u8  dgid[16];
-	__u32 flow_label;    
-	__u8  sgid_index;
-	__u8  hop_limit;
-	__u8  traffic_class;
-	__u8  reserved;
-};
-
-struct ib_uverbs_ah_attr {
-	struct ib_uverbs_global_route grh;
-	__u16 dlid;
-	__u8  sl;
-	__u8  src_path_bits;
-	__u8  static_rate;
-	__u8  is_global;
-	__u8  port_num;
-	__u8  reserved;
-};
-
 struct ib_uverbs_create_ah {
 	__u64 response;
 	__u64 user_handle;

^ permalink raw reply

* Re: [PATCH 0/8] Intel I/O Acceleration Technology (I/OAT)
From: Ingo Oeser @ 2006-03-06 17:44 UTC (permalink / raw)
  To: Evgeniy Polyakov
  Cc: David S. Miller, jengelh, christopher.leech, linux-kernel, netdev
In-Reply-To: <20060305014324.GA20026@2ka.mipt.ru>

Evgeniy Polyakov wrote:
> On Sat, Mar 04, 2006 at 01:41:44PM -0800, David S. Miller (davem@davemloft.net) wrote:
> > From: Jan Engelhardt <jengelh@linux01.gwdg.de>
> > Date: Sat, 4 Mar 2006 19:46:22 +0100 (MET)
> > 
> > > Does this buy the normal standard desktop user anything?
> > 
> > Absolutely, it optimizes end-node performance.
> 
> It really depends on how it is used.
> According to investigation made for kevent based FS AIO reading,
> get_user_pages() performange graph looks like sqrt() function

Hmm, so I should resurrect my user page table walker abstraction?

There I would hand each page to a "recording" function, which
can drop the page from the collection or coalesce it in the collector
if your scatter gather implementation allows it.

Regards

Ingo Oeser

^ permalink raw reply

* Re: [Patch 7/7] Generic netlink interface (delay accounting)
From: Shailabh Nagar @ 2006-03-06 17:00 UTC (permalink / raw)
  To: hadi; +Cc: netdev, linux-kernel, lse-tech
In-Reply-To: <1141652556.5185.64.camel@localhost.localdomain>

Jamal,

Pls keep lkml and lse-tech on cc since some of this affects the usage
of delay accounting.


jamal wrote:

>Hi Shailabh,
>Apologies for taking a week to respond ..
>
>On Mon, 2006-27-02 at 15:26 -0500, Shailabh Nagar wrote: 
>  
>
>>jamal wrote:
>>    
>>
>
>  
>
>>Yes, the current intent is to allow multiple listeners to receive the 
>>responses sent by the kernel.
>>    
>>
>
>Responses or events? There is a difference:
>Response implies the program in user space requested (ex a GET) for that
>information and is receiving such info.
>Event implies the program in user space asked to be informed of changes
>in the kernel. Example an exit would be considered an event. 
>Events are received by virtue of registering to a multicast group.
>[..] 
>  
>
My design was to have the listener get both responses (what I call 
replies in the code)
as well as events (data sent on exit of pid)

>>Since this interface (taskstats) is currently designed for that 
>>possibility, having multiple listeners, one for
>>each "component" such as delay accounting, is the model we're using.
>>We expect each component to have a pair of userspace programs, one for 
>>sending commands and the other
>>to "listen" to all replies + data generated on task exits. 
>>    
>>
>
>You need to have a sender of GETs essentially and a listener of events.
>Those are two connections. The replies of a get from user1 will not be
>sent to user2 as well - unless ... thats what you are trying to achieve;
>the question is why?
>  
>
Yes, I was trying to have an asymmetric model where the userspace sender 
of GETs
doesn't receive the reply as a unicast. Rather the reply is sent by 
multicast (alongwith all the
event data).

Reason for this unintuitive design was to make it easier to process the 
returned data.

The expected usage of delay accounting is to periodically "sample" the 
delays for all
tasks (or tgids) in the system. Also, get the delays from exiting pids 
(lets forget how tgid exit
is handled for now...irrelevant to this discussion).

Using the above two pieces of data, userspace can aggregate the "delays" 
seen by any
grouping of tasks that it chooses to implement.

In this usage scenario, its more efficient to have one receiver get both 
response and event
data and process in a loop.

However, we could switch to the model you suggest and use a 
multithreaded send/receive
userspace utility.

>>The listener 
>>is expected to register/deregister interest through
>>TASKSTATS_CMD_LISTEN and IGNORE.
>>
>>    
>>
>
>It is not necessary if you follow the model i described.
>
>  
>
>>>How does this correlate to TASKSTATS_CMD_LISTEN/IGNORE?
>>> 
>>>
>>>      
>>>
>>See above. Its mainly an optimization so that if no listener is present, 
>>there's no need to generate the data.
>>
>>    
>>
>
>Also not necessary - There is a recent netlink addition to make sure
>that events dont get sent if no listeners exist.
>genetlink needs to be extended. For now assume such a thing exists.
>  
>
Ok. Will this addition work for both unicast and multicast modes ?



>  
>
>>>>+
>>>>        
>>>>
>
>  
>
>>Good point. Should check for users sending it as a cmd and treat it as a 
>>noop. 
>>    
>>
>
>More like return an -EINVAL
>  
>
Will this be necessary ? Isn't genl_rcv_msg() going to return a -EOPNOTSUPP
automatically for us since we've not registered the command ?
 

>  
>
>>I'm just using
>>this as a placeholder for data thats returned without being requested.
>>
>>    
>>
>
>So it is unconditional?
>  
>
Yes.

>  
>
>>Come to think of it, there's no real reason to have a genlmsghdr for 
>>returned data, is there ?
>>    
>>
>
>All messages should be consistent whether they are sent from user
>or kernel.
>  
>
Ok. will retain genetlink header.

>>Other than to copy the genlmsghdr that was sent so user can identify 
>>which command was sent
>>(and I'm doing that through the reply type, perhaps redundantly).
>>
>>    
>>
>
>yes, that is a useful trick. Just make sure they are reflected
>correctly.
>
>  
>
>>Actually, the next iteration of the code will move to dynamically 
>>generated ID. But yes, will need to check for that.
>>
>>    
>>
>
>Also if you can provide feedback whether the doc i sent was any use
>and what wasnt clear etc.
>  
>
Will do.

>>Thanks for the review.
>>Couple of questions about general netlink:
>>is it intended to remain a size that will always be aligned to the 
>>NLMSG_ALIGNTO so that (NLMSG_DATA(nlhdr) + GENL_HDRLEN) can always 
>>be used as a pointer to the genlmsghdr ?
>>
>>    
>>
>
>I am not sure i followed.
>The whole message (nlhdr, genlhdr, optionalhdr, TLVs) has to be in 
>the end 32 bit aligned.
>  
>
Ok , so separate padding isn't needed to make the genlhdr, optionalhdr 
and TLV parts aligned
too.

>>Adding some macros like genlmsg_data(nlh) would be handy (currently I 
>>just define and use it locally).
>>
>>    
>>
>
>Send a patch.
>  
>
will do.


Thanks,
Shailabh

>cheers,
>jamal
>
>  
>

^ permalink raw reply

* Re: de2104x: interrupts before interrupt handler is registered
From: Martin Michlmayr @ 2006-03-06 14:35 UTC (permalink / raw)
  To: Francois Romieu; +Cc: netdev, linux-kernel
In-Reply-To: <20060305185948.GA24765@electric-eye.fr.zoreil.com>

* Francois Romieu <romieu@fr.zoreil.com> [2006-03-05 19:59]:
> > I have a system on which I can reproduce this bug 100%.  While I have
> > no idea how to fix the issue, I can provide debugging information and
> > test a fix.

> (not compile-tested)

Thanks a lot for your quick response, Francois.  I can confirm that
this patch fixes the problem for me.

> -err_out_hw:
> -	spin_lock_irqsave(&de->lock, flags);
> -	de_stop_hw(de);
> -	spin_unlock_irqrestore(&de->lock, flags);

flags is no longer used now, so we get a compilation warning.  Updated
patch below.  Francois, can you please submit it with a proper
changelog entry and your Signed-off-by.


From: Francois Romieu <romieu@fr.zoreil.com>
Signed-off-by: Martin Michlmayr <tbm@cyrius.com>

--- a/drivers/net/tulip/de2104x.c
+++ b/drivers/net/tulip/de2104x.c
@@ -1362,7 +1362,6 @@ static int de_open (struct net_device *d
 {
 	struct de_private *de = dev->priv;
 	int rc;
-	unsigned long flags;
 
 	if (netif_msg_ifup(de))
 		printk(KERN_DEBUG "%s: enabling interface\n", dev->name);
@@ -1376,18 +1375,20 @@ static int de_open (struct net_device *d
 		return rc;
 	}
 
-	rc = de_init_hw(de);
-	if (rc) {
-		printk(KERN_ERR "%s: h/w init failure, err=%d\n",
-		       dev->name, rc);
-		goto err_out_free;
-	}
+	dw32(IntrMask, 0);
 
 	rc = request_irq(dev->irq, de_interrupt, SA_SHIRQ, dev->name, dev);
 	if (rc) {
 		printk(KERN_ERR "%s: IRQ %d request failure, err=%d\n",
 		       dev->name, dev->irq, rc);
-		goto err_out_hw;
+		goto err_out_free;
+	}
+
+	rc = de_init_hw(de);
+	if (rc) {
+		printk(KERN_ERR "%s: h/w init failure, err=%d\n",
+		       dev->name, rc);
+		goto err_out_free_irq;
 	}
 
 	netif_start_queue(dev);
@@ -1395,11 +1396,8 @@ static int de_open (struct net_device *d
 
 	return 0;
 
-err_out_hw:
-	spin_lock_irqsave(&de->lock, flags);
-	de_stop_hw(de);
-	spin_unlock_irqrestore(&de->lock, flags);
-
+err_out_free_irq:
+	free_irq(dev->irq, dev);
 err_out_free:
 	de_free_rings(de);
 	return rc;

-- 
Martin Michlmayr
http://www.cyrius.com/

^ permalink raw reply

* Re: GigE on PowerMac G5
From: Benjamin Herrenschmidt @ 2006-03-06 13:15 UTC (permalink / raw)
  To: Andreas Schwab; +Cc: netdev, linuxppc64-dev
In-Reply-To: <jefylvzwhw.fsf@sykes.suse.de>

On Mon, 2006-03-06 at 11:40 +0100, Andreas Schwab wrote:
> Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
> 
> > On Sat, 2006-03-04 at 15:53 +0100, Andreas Schwab wrote:
> >> I suppose the NIC in the PowerMac G5 can do GigE, yet when plugged into a
> >> GB switch it is only willing to talk 100MB with it.  Any idea why?  Kernel
> >> is 2.6.16-rc5-git2.
> >
> > Works for me... Must be a problem with auto-neg and your switch, or the
> > cable.... Can you check how the switch is configured maybe ? You can
> > also try forcing the link speed with ethtool.
> 
> It's not the cable, I have swapped it with another system where Gb is
> working fine.  Neither it's the switch port, I have swapped it too.  I
> can't force the speed with ethtool either.  Any other idea what to look
> for?

At this point, all I can say is... does it work in OS X ?

Ben.

^ permalink raw reply

* Re: de2104x: interrupts before interrupt handler is registered
From: linux-os (Dick Johnson) @ 2006-03-06 13:02 UTC (permalink / raw)
  To: Martin Michlmayr; +Cc: Jeff Garzik, netdev, linux-kernel
In-Reply-To: <20060305180757.GA22121@deprecation.cyrius.com>


On Sun, 5 Mar 2006, Martin Michlmayr wrote:

> We have three independent reports about problems with de2104x involving
> interrupts.  Alan Stern suggested that it "sure looks as though the
> ethernet interface is generating an interrupt request before the
> de2104x driver has registered its interrupt handler".
>
> The three reports are:
> - de2104x does not work (non-fatal oops) when uhci_hcd is loaded
>   first.  http://lkml.org/lkml/2006/2/3/402  The problem does not
>   occur under 2.4 with the tulip module, so this is a regression.
> - fatal de2104x interrupt oops (without uhci_hcd).
>   http://lkml.org/lkml/2006/2/5/64
> - "kernel panic after the first transmission attempt times out"
>   Regression from 2.4.  http://bugs.debian.org/288821
>
> I have a system on which I can reproduce this bug 100%.  While I have
> no idea how to fix the issue, I can provide debugging information and
> test a fix.  However, I'm (temporarily) leaving the country in three
> weeks and won't have access to this PC for several months, so it would
> be great if someone could look into this soon.  Jeff?
>
>
> 1)
> eth0: enabling interface
> eth0: set link 10baseT auto
> eth0:    mode 0x7ffc0040, sia 0x10c4,0xffffef01,0xffffffff,0xffff0008
> eth0:    set mode 0x7ffc0040, set sia 0xef01,0xffff,0x8
> irq 10: nobody cared (try booting with the "irqpoll" option)
> [<c012f89e>] __report_bad_irq+0x31/0x73
> [<c012f96d>] note_interrupt+0x75/0x98
> [<c012f46a>] __do_IRQ+0x67/0x91
> [<c0104fc1>] do_IRQ+0x19/0x24
> [<c0103afa>] common_interrupt+0x1a/0x20
> [<c0119a1c>] __do_softirq+0x2c/0x7d
> [<c0119a8f>] do_softirq+0x22/0x26
> [<c0104fc6>] do_IRQ+0x1e/0x24
> [<c0103afa>] common_interrupt+0x1a/0x20
> [<c481da07>] de_set_rx_mode+0xf/0x12 [de2104x]
> [<c481e2c1>] de_init_hw+0x6d/0x76 [de2104x]
> [<c481e59e>] de_open+0x64/0xe4 [de2104x]
> [<c0225a5f>] dev_open+0x30/0x66
> [<c0226a9a>] dev_change_flags+0x4d/0xf0
> [<c025d301>] devinet_ioctl+0x224/0x4bd
> [<c0155541>] do_ioctl+0x21/0x50
> [<c0155774>] vfs_ioctl+0x152/0x161
> [<c01557cb>] sys_ioctl+0x48/0x65
> [<c0102a99>] syscall_call+0x7/0xb
> handlers:
> [<c4890d97>] (usb_hcd_irq+0x0/0x56 [usbcore])
> Disabling IRQ #10
>
> 3)
> eth0:    mode 0x7ffc0040, sia 0x10c4,0xffffef01,0xffffffff,0xffff0008
> eth0:    set mode 0x7ffc0040, set sia 0xef01,0xffff,0x8
> [__report_bad_irq+42/144] __report_bad_irq+0x2a/0x90
> [note_interrupt+108/160] note_interrupt+0x6c/0xa0
> [do_IRQ+289/304] do_IRQ+0x121/0x130
> [common_interrupt+24/32] common_interrupt+0x18/0x20
> [__do_softirq+48/128] __do_softirq+0x30/0x80
> [acpi_irq+0/22] acpi_irq+0x0/0x16
> [do_softirq+38/48] do_softirq+0x26/0x30
> [do_IRQ+253/304] do_IRQ+0xfd/0x130
> [common_interrupt+24/32] common_interrupt+0x18/0x20
> [__crc_do_softirq+25311/208152] de_set_rx_mode+0x26/0x50 [de2104x]
> [__crc_do_softirq+28277/208152] de_init_hw+0x8c/0x90 [de2104x]
> [__crc_do_softirq+29105/208152] de_open+0x68/0x140 [de2104x]
> [profile_hook+45/75] profile_hook+0x2d/0x4b
> [dev_open+203/256] dev_open+0xcb/0x100
> [dev_mc_upload+36/80] dev_mc_upload+0x24/0x50
> [dev_change_flags+81/288] dev_change_flags+0x51/0x120
> [devinet_ioctl+582/1424] devinet_ioctl+0x246/0x590
> [inet_ioctl+94/160] inet_ioctl+0x5e/0xa0
> [sock_ioctl+249/688] sock_ioctl+0xf9/0x2b0
> [sys_ioctl+269/656] sys_ioctl+0x10d/0x290
> [syscall_call+7/11] syscall_call+0x7/0xb
> eth0: link up, media 10baseT auto
>
> --

> Martin Michlmayr
> http://www.cyrius.com/
> -

This started to happen in a lot of PCI drivers once it became
necessary to call pci_enable_device() in order to make the
returned IRQ values valid. This has been reported numerious
times and has not been fixed. Basically, in order to get
the correct value, one needs to disable the board in some
unspecified way so it is not possible for it to generate
an interrupt before enabling the board. With some devices
this may not be possible!

Cheers,
Dick Johnson
Penguin : Linux version 2.6.15.4 on an i686 machine (5589.47 BogoMips).
Warning : 98.36% of all statistics are fiction, book release in April.
_
\x1a\x04

****************************************************************
The information transmitted in this message is confidential and may be privileged.  Any review, retransmission, dissemination, or other use of this information by persons or entities other than the intended recipient is prohibited.  If you are not the intended recipient, please notify Analogic Corporation immediately - by replying to this message or by sending an email to DeliveryErrors@analogic.com - and destroy all copies of this information, including any attachments, without reading or disclosing them.

Thank you.

^ permalink raw reply

* Re: GigE on PowerMac G5
From: Andreas Schwab @ 2006-03-06 10:40 UTC (permalink / raw)
  To: Benjamin Herrenschmidt; +Cc: netdev, linuxppc64-dev
In-Reply-To: <1141507000.17127.4.camel@localhost.localdomain>

Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:

> On Sat, 2006-03-04 at 15:53 +0100, Andreas Schwab wrote:
>> I suppose the NIC in the PowerMac G5 can do GigE, yet when plugged into a
>> GB switch it is only willing to talk 100MB with it.  Any idea why?  Kernel
>> is 2.6.16-rc5-git2.
>
> Works for me... Must be a problem with auto-neg and your switch, or the
> cable.... Can you check how the switch is configured maybe ? You can
> also try forcing the link speed with ethtool.

It's not the cable, I have swapped it with another system where Gb is
working fine.  Neither it's the switch port, I have swapped it too.  I
can't force the speed with ethtool either.  Any other idea what to look
for?

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

^ permalink raw reply

* [PATCH] net: drop duplicate assignment in request_sock
From: Norbert Kiesel @ 2006-03-05 23:45 UTC (permalink / raw)
  To: acme; +Cc: linux-kernel, netdev

From: Norbert Kiesel <nkiesel@tbdnetworks.com>

Hi,

just noticed that request_sock.[ch] contain a useless assignment of
rskq_accept_head to itself.  I assume this is a typo and the 2nd one
was supposed to be _tail.  However, setting _tail to NULL is not
needed, so the patch below just drops the 2nd assignment.

Best,
  Norbert

Signed-Off-By: Norbert Kiesel <nkiesel@tbdnetworks.com>

---

diff -ru a/include/net/request_sock.h b/include/net/request_sock.h
--- a/include/net/request_sock.h	2005-10-28 15:44:45.000000000 -0700
+++ b/include/net/request_sock.h	2006-03-05 15:22:33.000000000 -0800
@@ -145,7 +145,7 @@
 {
 	struct request_sock *req = queue->rskq_accept_head;
 
-	queue->rskq_accept_head = queue->rskq_accept_head = NULL;
+	queue->rskq_accept_head = NULL;
 	return req;
 }
 
diff -ru a/net/core/request_sock.c b/net/core/request_sock.c
--- a/net/core/request_sock.c	2006-03-05 14:40:50.000000000 -0800
+++ b/net/core/request_sock.c	2006-03-05 15:23:11.000000000 -0800
@@ -51,7 +51,7 @@
 
 	get_random_bytes(&lopt->hash_rnd, sizeof(lopt->hash_rnd));
 	rwlock_init(&queue->syn_wait_lock);
-	queue->rskq_accept_head = queue->rskq_accept_head = NULL;
+	queue->rskq_accept_head = NULL;
 	lopt->nr_table_entries = nr_table_entries;
 
 	write_lock_bh(&queue->syn_wait_lock);

^ permalink raw reply

* Please pull bcm43xx softmac-upstream and dscape-upstream branches
From: Michael Buesch @ 2006-03-05 20:47 UTC (permalink / raw)
  To: John W. Linville
  Cc: netdev-u79uwXL29TY76Z2rM5mHXA, bcm43xx-dev-0fE9KPoRgkgATYTw5x5z8w

[-- Attachment #1: Type: text/plain, Size: 2826 bytes --]

Hi John,

It took a little longer than promised, but a broken harddisk in
my server delayed my plans. ;)

Please pull branches "softmac-upstream" and "dscape-upstream"
from my repository at:
git://bu3sch.de/wireless-2.6.git

This introduces the following changesets:
Michael Buesch:
      bcm43xx-d80211: Partially fix PIO code. Add Kconfig option for PIO or DMA mode (or both).
      bcm43xx-d80211: fix typo. Missing D80211.
      bcm43xx-d80211: add note that not all devices support PIO.
      bcm43xx-d80211: Documentation fix by "Bin Zhang" <yangtze31-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
      bcm43xx-d80211: completely disable wireless_handlers, as we must not override d80211 handlers.
      bcm43xx-d80211: Move sprom lowlevel reading/writing to its own functions.
      bcm43xx-d80211: make bcm43xx_sprom_crc() static.
      bcm43xx-d80211: remove all remaining standard WX.
      bcm43xx-d80211: split channel helper functions, so that they can be called without a valid running core.
      bcm43xx-d80211: remove old unused struct.
      bcm43xx-d80211: Completely remove all WX and add a sysfs interface as substitute for the private WX.
      bcm43xx-d80211: rewrite and simplify the periodic task handling.
      bcm43xx-d80211: Fix Kconfig typo (transfer mode default)
      bcm43xx-d80211: use -Dwext for wpa_supplicant.
      bcm43xx-d80211: Don't build add_sta, if it already exists.
      bcm43xx-d80211: Workaround init_board vs IRQ race.
      bcm43xx-d80211: move initialized = 1 to the end of init_board.
      bcm43xx-d80211: Remove obsolete FIXME on wireless_handlers.
      bcm43xx-d80211: Move TX/RX related functions to its own file. Add basic RTS/CTS code.
      bcm43xx-d80211: fix typo in comment.
      bcm43xx-d80211: properly register device attributes at the right place.
      bcm43xx-d80211: remove magic add_sta.c and add the STA interface through sysfs.

Michael Buesch:
      bcm43xx: Move sprom lowlevel reading/writing to its own functions.
      bcm43xx: make bcm43xx_sprom_crc() static.
      bcm43xx: split the channel helper functions, so that they can be used without a valid running core.
      bcm43xx: remove old unused struct.
      bcm43xx: Fix Kconfig typo (transfer mode default)
      bcm43xx: Workaround init_board vs IRQ race.
      bcm43xx: move initialized = 1 to the end of init_board.
      bcm43xx: add assert(bcm->initialized) to periodic_tasks_setup().
      bcm43xx: Move TX/RX related functions to its own file. Add basic RTS/CTS code.
      bcm43xx: Add sysfs attributes for device specific tunables.

If you want to look at the individual patches, please use
my git webinterface, as the patch would be to big to inline here.
http://bu3sch.de/git?p=wireless-2.6.git;a=summary

-- 
Greetings Michael.

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

^ permalink raw reply

* Re: de2104x: interrupts before interrupt handler is registered
From: Francois Romieu @ 2006-03-05 18:59 UTC (permalink / raw)
  To: Martin Michlmayr; +Cc: Jeff Garzik, netdev, linux-kernel
In-Reply-To: <20060305180757.GA22121@deprecation.cyrius.com>

Martin Michlmayr <tbm@cyrius.com> :
[...]
> I have a system on which I can reproduce this bug 100%.  While I have
> no idea how to fix the issue, I can provide debugging information and
> test a fix.  However, I'm (temporarily) leaving the country in three
> weeks and won't have access to this PC for several months, so it would
> be great if someone could look into this soon.  Jeff?

(not compile-tested)

diff --git a/drivers/net/tulip/de2104x.c b/drivers/net/tulip/de2104x.c
index d7fb3ff..d16a5a0 100644
--- a/drivers/net/tulip/de2104x.c
+++ b/drivers/net/tulip/de2104x.c
@@ -1376,18 +1376,20 @@ static int de_open (struct net_device *d
 		return rc;
 	}
 
-	rc = de_init_hw(de);
-	if (rc) {
-		printk(KERN_ERR "%s: h/w init failure, err=%d\n",
-		       dev->name, rc);
-		goto err_out_free;
-	}
+	dw32(IntrMask, 0);
 
 	rc = request_irq(dev->irq, de_interrupt, SA_SHIRQ, dev->name, dev);
 	if (rc) {
 		printk(KERN_ERR "%s: IRQ %d request failure, err=%d\n",
 		       dev->name, dev->irq, rc);
-		goto err_out_hw;
+		goto err_out_free;
+	}
+
+	rc = de_init_hw(de);
+	if (rc) {
+		printk(KERN_ERR "%s: h/w init failure, err=%d\n",
+		       dev->name, rc);
+		goto err_out_free_irq;
 	}
 
 	netif_start_queue(dev);
@@ -1395,11 +1397,8 @@ static int de_open (struct net_device *d
 
 	return 0;
 
-err_out_hw:
-	spin_lock_irqsave(&de->lock, flags);
-	de_stop_hw(de);
-	spin_unlock_irqrestore(&de->lock, flags);
-
+err_out_free_irq:
+	free_irq(dev->irq, dev);
 err_out_free:
 	de_free_rings(de);
 	return rc;

^ permalink raw reply related

* de2104x: interrupts before interrupt handler is registered
From: Martin Michlmayr @ 2006-03-05 18:07 UTC (permalink / raw)
  To: Jeff Garzik, netdev; +Cc: linux-kernel

We have three independent reports about problems with de2104x involving
interrupts.  Alan Stern suggested that it "sure looks as though the
ethernet interface is generating an interrupt request before the
de2104x driver has registered its interrupt handler".

The three reports are:
 - de2104x does not work (non-fatal oops) when uhci_hcd is loaded
   first.  http://lkml.org/lkml/2006/2/3/402  The problem does not
   occur under 2.4 with the tulip module, so this is a regression.
 - fatal de2104x interrupt oops (without uhci_hcd).
   http://lkml.org/lkml/2006/2/5/64
 - "kernel panic after the first transmission attempt times out"
   Regression from 2.4.  http://bugs.debian.org/288821

I have a system on which I can reproduce this bug 100%.  While I have
no idea how to fix the issue, I can provide debugging information and
test a fix.  However, I'm (temporarily) leaving the country in three
weeks and won't have access to this PC for several months, so it would
be great if someone could look into this soon.  Jeff?


1)
eth0: enabling interface
eth0: set link 10baseT auto
eth0:    mode 0x7ffc0040, sia 0x10c4,0xffffef01,0xffffffff,0xffff0008
eth0:    set mode 0x7ffc0040, set sia 0xef01,0xffff,0x8
irq 10: nobody cared (try booting with the "irqpoll" option)
 [<c012f89e>] __report_bad_irq+0x31/0x73
 [<c012f96d>] note_interrupt+0x75/0x98
 [<c012f46a>] __do_IRQ+0x67/0x91
 [<c0104fc1>] do_IRQ+0x19/0x24
 [<c0103afa>] common_interrupt+0x1a/0x20
 [<c0119a1c>] __do_softirq+0x2c/0x7d
 [<c0119a8f>] do_softirq+0x22/0x26
 [<c0104fc6>] do_IRQ+0x1e/0x24
 [<c0103afa>] common_interrupt+0x1a/0x20
 [<c481da07>] de_set_rx_mode+0xf/0x12 [de2104x]
 [<c481e2c1>] de_init_hw+0x6d/0x76 [de2104x]
 [<c481e59e>] de_open+0x64/0xe4 [de2104x]
 [<c0225a5f>] dev_open+0x30/0x66
 [<c0226a9a>] dev_change_flags+0x4d/0xf0
 [<c025d301>] devinet_ioctl+0x224/0x4bd
 [<c0155541>] do_ioctl+0x21/0x50
 [<c0155774>] vfs_ioctl+0x152/0x161
 [<c01557cb>] sys_ioctl+0x48/0x65
 [<c0102a99>] syscall_call+0x7/0xb
handlers:
[<c4890d97>] (usb_hcd_irq+0x0/0x56 [usbcore])
Disabling IRQ #10

3)
eth0:    mode 0x7ffc0040, sia 0x10c4,0xffffef01,0xffffffff,0xffff0008
eth0:    set mode 0x7ffc0040, set sia 0xef01,0xffff,0x8
 [__report_bad_irq+42/144] __report_bad_irq+0x2a/0x90
 [note_interrupt+108/160] note_interrupt+0x6c/0xa0
 [do_IRQ+289/304] do_IRQ+0x121/0x130
 [common_interrupt+24/32] common_interrupt+0x18/0x20
 [__do_softirq+48/128] __do_softirq+0x30/0x80
 [acpi_irq+0/22] acpi_irq+0x0/0x16
 [do_softirq+38/48] do_softirq+0x26/0x30
 [do_IRQ+253/304] do_IRQ+0xfd/0x130
 [common_interrupt+24/32] common_interrupt+0x18/0x20
 [__crc_do_softirq+25311/208152] de_set_rx_mode+0x26/0x50 [de2104x]
 [__crc_do_softirq+28277/208152] de_init_hw+0x8c/0x90 [de2104x]
 [__crc_do_softirq+29105/208152] de_open+0x68/0x140 [de2104x]
 [profile_hook+45/75] profile_hook+0x2d/0x4b
 [dev_open+203/256] dev_open+0xcb/0x100
 [dev_mc_upload+36/80] dev_mc_upload+0x24/0x50
 [dev_change_flags+81/288] dev_change_flags+0x51/0x120
 [devinet_ioctl+582/1424] devinet_ioctl+0x246/0x590
 [inet_ioctl+94/160] inet_ioctl+0x5e/0xa0
 [sock_ioctl+249/688] sock_ioctl+0xf9/0x2b0
 [sys_ioctl+269/656] sys_ioctl+0x10d/0x290
 [syscall_call+7/11] syscall_call+0x7/0xb
eth0: link up, media 10baseT auto

-- 
Martin Michlmayr
http://www.cyrius.com/

^ permalink raw reply

* Re: [PATCH 8/8] [I/OAT] TCP recv offload to I/OAT
From: David S. Miller @ 2006-03-05 10:27 UTC (permalink / raw)
  To: akpm; +Cc: christopher.leech, linux-kernel, netdev
In-Reply-To: <20060305004534.1d94b3cf.akpm@osdl.org>

From: Andrew Morton <akpm@osdl.org>
Date: Sun, 5 Mar 2006 00:45:34 -0800

> The __get_cpu_var() here will run smp_processor_id() from preemptible
> context.  You'll get a big warning if the correct debug options are set.
> 
> The reason for this is that preemption could cause this code to hop between
> CPUs.
> 
> Please always test code with all debug options enabled and with full kernel
> preemption.

To be fair that warning doesn't trigger on some platforms, such as
sparc64 where the __get_cpu_var() implementation simply takes the
value from a fixed cpu register and doesn't do the debugging check.

Sparc64 should add the check when debugging options are enabled, for
sure, but the point is that it may not entirely be the tester's fault.
:-)

^ permalink raw reply

* Re: Discourage duplicate symbols in the kernel? [Was: Intel I/O Acc...]
From: Andrew Morton @ 2006-03-05  9:18 UTC (permalink / raw)
  To: Sam Ravnborg; +Cc: chris.leech, christopher.leech, jeff, linux-kernel, netdev
In-Reply-To: <20060305090251.GA9116@mars.ravnborg.org>

Sam Ravnborg <sam@ravnborg.org> wrote:
>
> On Sun, Mar 05, 2006 at 12:09:33AM -0800, Andrew Morton wrote:
>  > > +
>  > > +static inline u8 read_reg8(struct cb_device *device, unsigned int offset)
>  > > +{
>  > > +	return readb(device->reg_base + offset);
>  > > +}
>  > 
>  > These are fairly generic-sounding names.  In fact the as-yet-unmerged tiacx
>  > wireless driver is already using these, privately to
>  > drivers/net/wireless/tiacx/pci.c.
> 
>  Do we in general discourage duplicate symbols even if they are static?

Well, it's a bit irritating that it confuses ctags.  But in this case, one
set is in a header file so the risk of collisions is much-increased.

^ permalink raw reply

* Discourage duplicate symbols in the kernel? [Was: Intel I/O Acc...]
From: Sam Ravnborg @ 2006-03-05  9:02 UTC (permalink / raw)
  To: Andrew Morton; +Cc: chris.leech, christopher.leech, jeff, linux-kernel, netdev
In-Reply-To: <20060305000933.2d799138.akpm@osdl.org>

On Sun, Mar 05, 2006 at 12:09:33AM -0800, Andrew Morton wrote:
> > +
> > +static inline u8 read_reg8(struct cb_device *device, unsigned int offset)
> > +{
> > +	return readb(device->reg_base + offset);
> > +}
> 
> These are fairly generic-sounding names.  In fact the as-yet-unmerged tiacx
> wireless driver is already using these, privately to
> drivers/net/wireless/tiacx/pci.c.

Do we in general discourage duplicate symbols even if they are static?

[ppc64, allmodconfig]

$> nm vmlinux | fgrep ' t ' | awk '{print $3}' | sort | uniq -dc
      2 .add_bridge
      2 .base_probe
      2 .c_next
      2 .c_start
      2 .c_stop
      3 .cpu_callback
      2 .default_open
      2 .default_read_file
      2 .default_write_file
      2 .dev_ifsioc
      2 .do_open
      4 .dst_output
      2 .dump_seek
      2 .dump_write
      2 .elf_core_dump
      2 .elf_map
      2 .exact_lock
      2 .exact_match
      2 .exit_elf_binfmt
      2 .fill_note
      2 .fill_prstatus
      2 .fillonedir
      2 .fini
      2 .fixup_one_level_bus_range
      5 .init
      8 .init_once
      3 .iommu_bus_setup_null
      3 .iommu_dev_setup_null
      2 .klist_devices_get
      2 .klist_devices_put
      2 .load_elf_binary
      2 .load_elf_interp
      2 .load_elf_library
      3 .m_next
      3 .m_start
      3 .m_stop
      2 .maydump
      3 .modalias_show
      2 .next_device
      3 .notesize
      2 .padzero
      2 .raw_ioctl
      2 .s_next
      2 .s_show
      2 .s_start
      2 .s_stop
      2 .seq_next
      2 .seq_show
      2 .seq_start
      2 .seq_stop
      2 .set_brk
      2 .setkey
      2 .state_show
      2 .state_store
      2 .store_uevent
      2 .u3_ht_cfg_access
      2 .u3_ht_read_config
      2 .u3_ht_write_config
      2 .writenote
      3 __initcall_init
      2 __setup_netdev_boot_setup
      2 __setup_str_netdev_boot_setup

If I did a make allyesconfig the result looks much more scary.

	Sam

^ permalink raw reply

* Re: [PATCH 8/8] [I/OAT] TCP recv offload to I/OAT
From: Andrew Morton @ 2006-03-05  8:45 UTC (permalink / raw)
  To: Chris Leech; +Cc: linux-kernel, netdev
In-Reply-To: <20060303214236.11908.98881.stgit@gitlost.site>

Chris Leech <christopher.leech@intel.com> wrote:
>
> +#ifdef CONFIG_NET_DMA
>  +	tp->ucopy.dma_chan = NULL;
>  +	if ((len > sysctl_tcp_dma_copybreak) && !(flags & MSG_PEEK) && !sysctl_tcp_low_latency && __get_cpu_var(softnet_data.net_dma))
>  +		dma_lock_iovec_pages(msg->msg_iov, len, &tp->ucopy.locked_list);
>  +#endif

The __get_cpu_var() here will run smp_processor_id() from preemptible
context.  You'll get a big warning if the correct debug options are set.

The reason for this is that preemption could cause this code to hop between
CPUs.

Please always test code with all debug options enabled and with full kernel
preemption.

^ permalink raw reply

* Re: [PATCH 0/8] Intel I/O Acceleration Technology (I/OAT)
From: Andrew Morton @ 2006-03-05  8:09 UTC (permalink / raw)
  To: chris.leech; +Cc: christopher.leech, jeff, linux-kernel, netdev
In-Reply-To: <41b516cb0603031439n13e4df4cg8e5b21b606d2b4b8@mail.gmail.com>

"Chris Leech" <christopher.leech@intel.com> wrote:
>
> > Patch #2 didn't make it.  Too big for the list?
> 
>  Could be, it's the largest of the series.  I've attached the gziped
>  patch.  I can try and split this up for the future.
>
> ..
>
> [I/OAT] Driver for the Intel(R) I/OAT DMA engine
> Adds a new ioatdma driver
> 
> ...
> +struct cb_pci_pmcap_register {
> +	uint32_t	capid:8;	/* RO: 01h */
> +	uint32_t	nxtcapptr:8;
> +	uint32_t	version:3;	/* RO: 010b */
> +	uint32_t	pmeclk:1;	/* RO: 0b */
> +	uint32_t	reserved:1;	/* RV: 0b */
> +	uint32_t	dsi:1;		/* RO: 0b */
> +	uint32_t	aux_current:3;	/* RO: 000b */
> +	uint32_t	d1_support:1;	/* RO: 0b */
> +	uint32_t	d2_support:1;	/* RO: 0b */
> +	uint32_t	pme_support:5;	/* RO: 11001b */
> +};

This maps onto hardware registers?  No big-endian plans in Intel's future? ;)

I have a vague feeling that gcc changed its layout of bitfields many years
ago.  I guess we're fairly safe against that.  Presumably gcc and icc use the
same layout?

Still.  It's a bit of a concern, but I guess we can worry about that if it
happens.

> +
> +static inline u8 read_reg8(struct cb_device *device, unsigned int offset)
> +{
> +	return readb(device->reg_base + offset);
> +}

These are fairly generic-sounding names.  In fact the as-yet-unmerged tiacx
wireless driver is already using these, privately to
drivers/net/wireless/tiacx/pci.c.

> +static int enumerate_dma_channels(struct cb_device *device)
> +{
> +	u8 xfercap_scale;
> +	u32 xfercap;
> +	int i;
> +	struct cb_dma_chan *cb_chan;
> +
> +	device->common.chancnt = read_reg8(device, CB_CHANCNT_OFFSET);
> +	xfercap_scale = read_reg8(device, CB_XFERCAP_OFFSET);
> +	xfercap = (xfercap_scale == 0 ? ~0UL : (1 << xfercap_scale));

I recommend using just "-1" to represent the all-ones pattern.  It simply
works, in all situations.

Where you _did_ want the UL was after that "1".

> +	for (i = 0; i < device->common.chancnt; i++) {
> +		cb_chan = kzalloc(sizeof(*cb_chan), GFP_KERNEL);
> +		if (!cb_chan)
> +			return -ENOMEM;

memory leak?

> +		cb_chan->device = device;
> +		cb_chan->reg_base = device->reg_base + (0x80 * (i + 1));
> +		cb_chan->xfercap = xfercap;
> +		spin_lock_init(&cb_chan->cleanup_lock);
> +		spin_lock_init(&cb_chan->desc_lock);
> +		INIT_LIST_HEAD(&cb_chan->free_desc);
> +		INIT_LIST_HEAD(&cb_chan->used_desc);
> +		/* This should be made common somewhere in dmaengine.c */
> +		cb_chan->common.device = &device->common;
> +		cb_chan->common.client = NULL;
> +		list_add_tail(&cb_chan->common.device_node, &device->common.channels);

No locking needed for that list?

> +static struct cb_desc_sw * cb_dma_alloc_descriptor(struct cb_dma_chan *cb_chan)

There's a mix of styles here.  I don't think the space after the asterisk does
anything useful, and it could be argued that it's incorrect (or misleading)
wrt C declaration semantics.

> +{
> +	struct cb_dma_descriptor *desc;

What do all these "cb"'s stand for, anyway?

> +	struct cb_desc_sw *desc_sw;
> +	struct cb_device *cb_device = to_cb_device(cb_chan->common.device);
> +	dma_addr_t phys;
> +
> +	desc = pci_pool_alloc(cb_device->dma_pool, GFP_ATOMIC, &phys);
> +	if (!desc)
> +		return NULL;
> +
> +	desc_sw = kzalloc(sizeof(*desc_sw), GFP_ATOMIC);

GFP_ATOMIC is to be avoided if at all possible.  It stresses the memory system
and can easily fail under load.

>From my reading, two of the callers could trivially call this function outside
spin_lock_bh() and the third could perhaps do so with a little work.  You
could at least fix up two of those callers, and pass in the gfp_flags.


<wonders why the heck dma_pool_alloc() uses SLAB_ATOMIC when the caller's
passing in the gfp_flags>

> +/* returns the actual number of allocated descriptors */
> +static int cb_dma_alloc_chan_resources(struct dma_chan *chan)
> +{
> ...
> +	/* Allocate descriptors */
> +	spin_lock_bh(&cb_chan->desc_lock);
> +	for (i = 0; i < INITIAL_CB_DESC_COUNT; i++) {
> +		desc = cb_dma_alloc_descriptor(cb_chan);
> +		if (!desc) {
> +			printk(KERN_ERR "CB: Only %d initial descriptors\n", i);
> +			break;
> +		}
> +		list_add_tail(&desc->node, &cb_chan->free_desc);
> +	}
> +	spin_unlock_bh(&cb_chan->desc_lock);

Here's one such caller.

> +
> +static void cb_dma_free_chan_resources(struct dma_chan *chan)
> +{
> +	struct cb_dma_chan *cb_chan = to_cb_chan(chan);
> +	struct cb_device *cb_device = to_cb_device(chan->device);
> +	struct cb_desc_sw *desc, *_desc;
> +	u16 chanctrl;
> +	int in_use_descs = 0;
> +
> +	cb_dma_memcpy_cleanup(cb_chan);
> +
> +	chan_write_reg8(cb_chan, CB_CHANCMD_OFFSET, CB_CHANCMD_RESET);
> +
> +	spin_lock_bh(&cb_chan->desc_lock);
> +	list_for_each_entry_safe(desc, _desc, &cb_chan->used_desc, node) {
> +		in_use_descs++;
> +		list_del(&desc->node);
> +		pci_pool_free(cb_device->dma_pool, desc->hw, desc->phys);
> +		kfree(desc);
> +	}
> +	list_for_each_entry_safe(desc, _desc, &cb_chan->free_desc, node) {
> +		list_del(&desc->node);
> +		pci_pool_free(cb_device->dma_pool, desc->hw, desc->phys);
> +		kfree(desc);
> +	}
> +	spin_unlock_bh(&cb_chan->desc_lock);

Do we actually need the lock there?  If we're freeing everything which it
protects anwyay?

> +
> +static void cb_dma_memcpy_cleanup(struct cb_dma_chan *chan)
> +{
> +	unsigned long phys_complete;
> +	struct cb_desc_sw *desc, *_desc;
> +	dma_cookie_t cookie = 0;
> +
> +	prefetch(chan->completion_virt);
> +
> +	if (!spin_trylock(&chan->cleanup_lock))
> +		return;

What's going on here?  Lock ranking problems?  spin_trylock() in
non-infrastructural code is a bit of a red flag.

Whatever the reason, it needs a comment in there please.  That comment should
also explain why simply baling out is acceptable.

> +
> +static irqreturn_t cb_do_interrupt(int irq, void *data, struct pt_regs *regs)
> +{
> +	struct cb_device *instance = data;
> +	unsigned long attnstatus;
> +	u8 intrctrl;
> +
> +	intrctrl = read_reg8(instance, CB_INTRCTRL_OFFSET);
> +
> +	if (!(intrctrl & CB_INTRCTRL_MASTER_INT_EN)) {
> +		return IRQ_NONE;
> +	}

braces.

> +	attnstatus = (unsigned long) read_reg32(instance, CB_ATTNSTATUS_OFFSET);

Unneeded cast.

> +static void cb_start_null_desc(struct cb_dma_chan *cb_chan)
> +{
> +	struct cb_desc_sw *desc;
> +
> +	spin_lock_bh(&cb_chan->desc_lock);
> +
> +	if (!list_empty(&cb_chan->free_desc)) {
> +		desc = to_cb_desc(cb_chan->free_desc.next);
> +		list_del(&desc->node);
> +	} else {
> +		/* try to get another desc */
> +		desc = cb_dma_alloc_descriptor(cb_chan);
> +		/* will this ever happen? */
> +		BUG_ON(!desc);
> +	}
> +
> +	desc->hw->ctl = CB_DMA_DESCRIPTOR_NUL;
> +	desc->hw->next = 0;
> +
> +	list_add_tail(&desc->node, &cb_chan->used_desc);
> +
> +#if (BITS_PER_LONG == 64)
> +	chan_write_reg64(cb_chan, CB_CHAINADDR_OFFSET, desc->phys);
> +#else
> +	chan_write_reg32(cb_chan, CB_CHAINADDR_OFFSET_LOW, (u32) desc->phys);
> +	chan_write_reg32(cb_chan, CB_CHAINADDR_OFFSET_HIGH, 0);
> +#endif
> +	chan_write_reg8(cb_chan, CB_CHANCMD_OFFSET, CB_CHANCMD_START);
> +
> +	spin_unlock_bh(&cb_chan->desc_lock);
> +}

Can the chan_write*() calls be moved outside the locked region?

> +/*
> + * Perform a CB transaction to verify the HW works.
> + */

Damn, I wish I knew what CB meant.

> +#define CB_TEST_SIZE 2000
> +
> +static int cb_self_test(struct cb_device *device)
> +{
> +	int i;
> +	u8 *src;
> +	u8 *dest;
> +	struct dma_chan *dma_chan;
> +	dma_cookie_t cookie;
> +	int err = 0;
> +
> +	src = kzalloc(sizeof(u8) * CB_TEST_SIZE, SLAB_KERNEL);
> +	if (!src)
> +		return -ENOMEM;
> +	dest = kzalloc(sizeof(u8) * CB_TEST_SIZE, SLAB_KERNEL);
> +	if (!dest) {
> +		kfree(src);
> +		return -ENOMEM;
> +	}
> +
> +	/* Fill in src buffer */
> +	for (i = 0; i < CB_TEST_SIZE; i++)
> +		src[i] = (u8)i;

memset?

> +	/* Start copy, using first DMA channel */
> +	dma_chan = container_of(device->common.channels.next, struct dma_chan, device_node);
> +
> +	cb_dma_alloc_chan_resources(dma_chan);

cb_dma_alloc_chan_resources() can fail.

> +	cookie = cb_dma_memcpy_buf_to_buf(dma_chan, dest, src, CB_TEST_SIZE);
> +	cb_dma_memcpy_issue_pending(dma_chan);
> +
> +	udelay(1000);

msleep(1) would be preferred.

> +static int __devinit cb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> +{
> +	int err;
> +	unsigned long mmio_start, mmio_len;
> +	void *reg_base;
> +	struct cb_device *device;
> +
> +	err = pci_enable_device(pdev);
> +	if (err)
> +		goto err_enable_device;
> +
> +	err = pci_set_dma_mask(pdev, DMA_64BIT_MASK);
> +	if (err)
> +		err = pci_set_dma_mask(pdev, DMA_32BIT_MASK);
> +	if (err)
> +		goto err_set_dma_mask;
> +
> +	err = pci_request_regions(pdev, cb_pci_drv.name);
> +	if (err)
> +		goto err_request_regions;
> +
> +	mmio_start = pci_resource_start(pdev, 0);
> +	mmio_len = pci_resource_len(pdev, 0);
> +
> +	reg_base = ioremap(mmio_start, mmio_len);
> +	if (!reg_base) {
> +		err = -ENOMEM;
> +		goto err_ioremap;
> +	}
> +
> +	device = kzalloc(sizeof(*device), GFP_KERNEL);
> +	if (!device) {
> +		err = -ENOMEM;
> +		goto err_kzalloc;
> +	}
> +
> +	/* DMA coherent memory pool for DMA descriptor allocations */
> +	device->dma_pool = pci_pool_create("dma_desc_pool", pdev,
> +		sizeof(struct cb_dma_descriptor), 64, 0);
> +	if (!device->dma_pool) {
> +		err = -ENOMEM;
> +		goto err_dma_pool;
> +	}
> +
> +	device->completion_pool = pci_pool_create("completion_pool", pdev, sizeof(u64), SMP_CACHE_BYTES, SMP_CACHE_BYTES);
> +	if (!device->completion_pool) {
> +		err = -ENOMEM;
> +		goto err_completion_pool;
> +	}
> +
> +	device->pdev = pdev;
> +	pci_set_drvdata(pdev, device);
> +#ifdef CONFIG_PCI_MSI
> +	if (pci_enable_msi(pdev) == 0) {
> +		device->msi = 1;
> +	} else {
> +		device->msi = 0;
> +	}
> +#endif
> +	err = request_irq(pdev->irq, &cb_do_interrupt, SA_SHIRQ, "ioat",
> +		device);
> +	if (err)
> +		goto err_irq;
> +
> +	device->reg_base = reg_base;
> +
> +	write_reg8(device, CB_INTRCTRL_OFFSET, CB_INTRCTRL_MASTER_INT_EN);
> +	pci_set_master(pdev);
> +
> +	INIT_LIST_HEAD(&device->common.channels);
> +	enumerate_dma_channels(device);

enumerate_dma_channels() can fail.

> +	device->common.device_alloc_chan_resources = cb_dma_alloc_chan_resources;
> +	device->common.device_free_chan_resources = cb_dma_free_chan_resources;
> +	device->common.device_memcpy_buf_to_buf = cb_dma_memcpy_buf_to_buf;
> +	device->common.device_memcpy_buf_to_pg = cb_dma_memcpy_buf_to_pg;
> +	device->common.device_memcpy_pg_to_pg = cb_dma_memcpy_pg_to_pg;
> +	device->common.device_memcpy_complete = cb_dma_is_complete;
> +	device->common.device_memcpy_issue_pending = cb_dma_memcpy_issue_pending;
> +	printk(KERN_INFO "Intel(R) I/OAT DMA Engine found, %d channels\n",
> +		device->common.chancnt);
> +
> +	if ((err = cb_self_test(device)))
> +		goto err_self_test;
> +
> +	dma_async_device_register(&device->common);
> +
> +	return 0;
> +
> +err_self_test:
> +err_irq:
> +	pci_pool_destroy(device->completion_pool);
> +err_completion_pool:
> +	pci_pool_destroy(device->dma_pool);
> +err_dma_pool:
> +	kfree(device);
> +err_kzalloc:
> +	iounmap(reg_base);
> +err_ioremap:
> +	pci_release_regions(pdev);
> +err_request_regions:
> +err_set_dma_mask:

You might want a pci_disable_device() in here.

> +err_enable_device:
> +	return err;
> +}
> +
> +static void __devexit cb_remove(struct pci_dev *pdev)
> +{
> +	struct cb_device *device;
> +
> +	device = pci_get_drvdata(pdev);
> +	dma_async_device_unregister(&device->common);

pci_disable_device()?

> +	free_irq(device->pdev->irq, device);
> +#ifdef CONFIG_PCI_MSI
> +	if (device->msi)
> +		pci_disable_msi(device->pdev);
> +#endif
> +	pci_pool_destroy(device->dma_pool);
> +	pci_pool_destroy(device->completion_pool);
> +	iounmap(device->reg_base);
> +	pci_release_regions(pdev);
> +	kfree(device);
> +}
> +
> +/* MODULE API */
> +MODULE_VERSION("1.0");
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Intel Corporation");
> +
> +static int __init cb_init_module(void)
> +{
> +	/* it's currently unsafe to unload this module */
> +	/* if forced, worst case is that rmmod hangs */

How come?

> +	if (THIS_MODULE != NULL)
> +		THIS_MODULE->unsafe = 1;
> +
> +	return pci_module_init(&cb_pci_drv);
> +}
> +



> +#define CB_LOW_COMPLETION_MASK		0xffffffc0
> +
> +extern struct list_head dma_device_list;
> +extern struct list_head dma_client_list;

It's strange to see extern decls for lists, but no decl for their lock.  A
comment might help.

> +struct cb_dma_chan {
> +
> +	void *reg_base;
> +
> +	dma_cookie_t completed_cookie;
> +	unsigned long last_completion;
> +
> +	u32 xfercap;	/* XFERCAP register value expanded out */
> +
> +	spinlock_t cleanup_lock;
> +	spinlock_t desc_lock;
> +	struct list_head free_desc;
> +	struct list_head used_desc;
> +
> +	int pending;
> +
> +	struct cb_device *device;
> +	struct dma_chan common;
> +
> +	dma_addr_t completion_addr;
> +	union {
> +		u64 full; /* HW completion writeback */
> +		struct {
> +			u32 low;
> +			u32 high;
> +		};
> +	} *completion_virt;
> +};

Again, is it safe to assume that these parts will never be present in
big-endian machines?

^ permalink raw reply

* Re: [PATCH 8/8] [I/OAT] TCP recv offload to I/OAT
From: Andrew Morton @ 2006-03-05  7:30 UTC (permalink / raw)
  To: Chris Leech; +Cc: linux-kernel, netdev
In-Reply-To: <20060303214236.11908.98881.stgit@gitlost.site>

Chris Leech <christopher.leech@intel.com> wrote:
>
> Locks down user pages and sets up for DMA in tcp_recvmsg, then calls
> dma_async_try_early_copy in tcp_v4_do_rcv
> 

+#ifdef CONFIG_NET_DMA
+#ifdef CONFIG_NET_DMA
+#ifdef CONFIG_NET_DMA
+#ifdef CONFIG_NET_DMA
+#ifdef CONFIG_NET_DMA
+#ifdef CONFIG_NET_DMA
+#ifdef CONFIG_NET_DMA
+#ifdef CONFIG_NET_DMA
+#ifdef CONFIG_NET_DMA
+#ifdef CONFIG_NET_DMA
+#ifdef CONFIG_NET_DMA
+#ifdef CONFIG_NET_DMA
+#ifdef CONFIG_NET_DMA
+#ifdef CONFIG_NET_DMA
+#ifdef CONFIG_NET_DMA
+#ifdef CONFIG_NET_DMA

waaay too many ifdefs.   There are various tricks we use to minimise them.

> +#ifdef CONFIG_NET_DMA
> +	tp->ucopy.dma_chan = NULL;
> +	if ((len > sysctl_tcp_dma_copybreak) && !(flags & MSG_PEEK) && !sysctl_tcp_low_latency && __get_cpu_var(softnet_data.net_dma))
> +		dma_lock_iovec_pages(msg->msg_iov, len, &tp->ucopy.locked_list);
> +#endif

Please try to fit code into 80 columns.

That's decimal 80 ;)

> @@ -1328,13 +1342,39 @@ do_prequeue:
>  		}
>  
>  		if (!(flags & MSG_TRUNC)) {
> -			err = skb_copy_datagram_iovec(skb, offset,
> -						      msg->msg_iov, used);
> -			if (err) {
> -				/* Exception. Bailout! */
> -				if (!copied)
> -					copied = -EFAULT;
> -				break;
> +#ifdef CONFIG_NET_DMA
> +			if (!tp->ucopy.dma_chan && tp->ucopy.locked_list)
> +				tp->ucopy.dma_chan = get_softnet_dma();
> +
> +			if (tp->ucopy.dma_chan) {
> +				tp->ucopy.dma_cookie = dma_skb_copy_datagram_iovec(
> +					tp->ucopy.dma_chan, skb, offset,
> +					msg->msg_iov, used,
> +					tp->ucopy.locked_list);
> +
> +				if (tp->ucopy.dma_cookie < 0) {
> +
> +					printk(KERN_ALERT "dma_cookie < 0\n");
> +
> +					/* Exception. Bailout! */
> +					if (!copied)
> +						copied = -EFAULT;
> +					break;
> +				}
> +				if ((offset + used) == skb->len)
> +					copied_early = 1;
> +

Consider trimming some of those blank lines.  I don't think they add any
value?

> +			} else
> +#endif
> +			{

These games with ifdefs and else statements aren't at all pleasant. 
Sometimes they're hard to avoid, but you'll probably find that some code
rearrangemnt (in a preceding patch) makes it easier.  Like, split this
function into several.

> @@ -1354,15 +1394,33 @@ skip_copy:
>  
>  		if (skb->h.th->fin)
>  			goto found_fin_ok;
> -		if (!(flags & MSG_PEEK))
> -			sk_eat_skb(sk, skb);
> +		if (!(flags & MSG_PEEK)) {
> +			if (!copied_early)
> +				sk_eat_skb(sk, skb);
> +#ifdef CONFIG_NET_DMA
> +			else {
> +				__skb_unlink(skb, &sk->sk_receive_queue);
> +				__skb_queue_tail(&sk->sk_async_wait_queue, skb);
> +				copied_early = 0;
> +			}
> +#endif
> ...
> -			sk_eat_skb(sk, skb);
> +		if (!(flags & MSG_PEEK)) {
> +			if (!copied_early)
> +				sk_eat_skb(sk, skb);
> +#ifdef CONFIG_NET_DMA
> +			else {
> +				__skb_unlink(skb, &sk->sk_receive_queue);
> +				__skb_queue_tail(&sk->sk_async_wait_queue, skb);
> +				copied_early = 0;
> +			}
> +#endif
> +		}

etc.

> +#ifdef CONFIG_NET_DMA
> +			if (copied_early)
> +				__skb_queue_tail(&sk->sk_async_wait_queue, skb);
> +			else
> +#endif
>  			if (eaten)
>  				__kfree_skb(skb);
>  			else

etc.

> @@ -4049,6 +4067,52 @@ discard:
>  	return 0;
>  }
>  
> +#ifdef CONFIG_NET_DMA
> +int dma_async_try_early_copy(struct sock *sk, struct sk_buff *skb, int hlen)
> +{
> +	struct tcp_sock *tp = tcp_sk(sk);
> +	int chunk = skb->len - hlen;
> +	int dma_cookie;
> +	int copied_early = 0;
> +
> +	if (tp->ucopy.wakeup)
> +          	goto out;

In this case a simple

		return 0;

would be fine.  We haven't done anything yet.

> +#ifdef CONFIG_NET_DMA
> +		struct tcp_sock *tp = tcp_sk(sk);
> +		if (!tp->ucopy.dma_chan && tp->ucopy.locked_list)
> +			tp->ucopy.dma_chan = get_softnet_dma();
> +		if (tp->ucopy.dma_chan)
> +			ret = tcp_v4_do_rcv(sk, skb);
> +		else
> +#endif
> +		{
> +			if (!tcp_prequeue(sk, skb))
>  			ret = tcp_v4_do_rcv(sk, skb);
> +		}
>  	} else

etc.

> +#ifdef CONFIG_NET_DMA
> +                struct tcp_sock *tp = tcp_sk(sk);
> +                if (tp->ucopy.dma_chan)
> +                        ret = tcp_v6_do_rcv(sk, skb);
> +                else
> +#endif
> +		{
> +			if (!tcp_prequeue(sk, skb))
> +				ret = tcp_v6_do_rcv(sk, skb);
> +		}
>  	} else

ow, my eyes!

^ permalink raw reply

* Re: [PATCH 7/8] [I/OAT] Add a sysctl for tuning the I/OAT offloaded I/O threshold
From: Andrew Morton @ 2006-03-05  7:21 UTC (permalink / raw)
  To: Chris Leech; +Cc: linux-kernel, netdev
In-Reply-To: <20060303214234.11908.99495.stgit@gitlost.site>

Chris Leech <christopher.leech@intel.com> wrote:
>
> Any socket recv of less than this ammount will not be offloaded
> 
> ...
>
> +int sysctl_tcp_dma_copybreak = NET_DMA_DEFAULT_COPYBREAK;

Is it appropriate that this tunable be kernel-wide, rather than more
finely-grained?

^ permalink raw reply

* Re: [PATCH 5/8] [I/OAT] Structure changes for TCP recv offload to I/OAT
From: Andrew Morton @ 2006-03-05  7:19 UTC (permalink / raw)
  To: Chris Leech; +Cc: linux-kernel, netdev
In-Reply-To: <20060303214229.11908.19898.stgit@gitlost.site>

Chris Leech <christopher.leech@intel.com> wrote:
>
> +#ifdef CONFIG_NET_DMA
>  +#include <linux/dmaengine.h>
>  +#endif

Please move the ifdefs into the header and include it unconditionally
(entire patchset).

^ permalink raw reply

* Re: [PATCH 4/8] [I/OAT] Utility functions for offloading sk_buff to iovec copies
From: Andrew Morton @ 2006-03-05  7:15 UTC (permalink / raw)
  To: Chris Leech; +Cc: linux-kernel, netdev
In-Reply-To: <20060303214227.11908.75473.stgit@gitlost.site>

Chris Leech <christopher.leech@intel.com> wrote:
>
> +
> +#define NUM_PAGES_SPANNED(start, length) \
> +	((PAGE_ALIGN((unsigned long)start + length) - \
> +	((unsigned long)start & PAGE_MASK)) >> PAGE_SHIFT)

static inline all-lower-case functions are much nicer.

> +/*
> + * Lock down all the iovec pages needed for len bytes.
> + * Return a struct dma_locked_list to keep track of pages locked down.
> + *
> + * We are allocating a single chunk of memory, and then carving it up into
> + * 3 sections, the latter 2 whose size depends on the number of iovecs and the
> + * total number of pages, respectively.
> + */
> +int dma_lock_iovec_pages(struct iovec *iov, size_t len, struct dma_locked_list
> +	**locked_list)

Please rename this to dma_pin_iovec_pages().  Locking a page is a quite
different concept from pinning it, and this function doesn't lock any
pages.

> +{
> +	struct dma_locked_list *local_list;
> +	struct page **pages;
> +	int i;
> +	int ret;
> +
> +	int nr_iovecs = 0;
> +	int iovec_len_used = 0;
> +	int iovec_pages_used = 0;

Extraneous blank line there.

> +	/* don't lock down non-user-based iovecs */
> +	if (segment_eq(get_fs(), KERNEL_DS)) {
> +		*locked_list = NULL;
> +		return 0;
> +	}

hm, haven't seen that before.  Makes sense, I guess.

> +	/* determine how many iovecs/pages there are, up front */
> +	do {
> +		iovec_len_used += iov[nr_iovecs].iov_len;
> +		iovec_pages_used += NUM_PAGES_SPANNED(iov[nr_iovecs].iov_base,
> +		                                      iov[nr_iovecs].iov_len);
> +		nr_iovecs++;
> +	} while (iovec_len_used < len);
> +
> +	/* single kmalloc for locked list, page_list[], and the page arrays */
> +	local_list = kmalloc(sizeof(*local_list)
> +		+ (nr_iovecs * sizeof (struct dma_page_list))
> +		+ (iovec_pages_used * sizeof (struct page*)), GFP_KERNEL);

What is the upper bound on the size of this allocation?

> +	if (!local_list)
> +		return -ENOMEM;
> +
> +	/* list of pages starts right after the page list array */
> +	pages = (struct page **) &local_list->page_list[nr_iovecs];
> +
> +	/* it's a userspace pointer */
> +	might_sleep();

kmalloc(GFP_KERNEL) already did that.

> +	for (i = 0; i < nr_iovecs; i++) {
> +		struct dma_page_list *page_list = &local_list->page_list[i];
> +
> +		len -= iov[i].iov_len;
> +
> +		if (!access_ok(VERIFY_WRITE, iov[i].iov_base, iov[i].iov_len)) {
> +			dma_unlock_iovec_pages(local_list);
> +			return -EFAULT;
> +		}

A return statement buried down in the guts of a largeish function isn't
good from a code maintainability POV.

> +		page_list->nr_pages = NUM_PAGES_SPANNED(iov[i].iov_base,
> +		                                        iov[i].iov_len);
> +		page_list->base_address = iov[i].iov_base;
> +
> +		page_list->pages = pages;
> +		pages += page_list->nr_pages;
> +
> +		/* lock pages down */
> +		down_read(&current->mm->mmap_sem);
> +		ret = get_user_pages(
> +			current,
> +			current->mm,
> +			(unsigned long) iov[i].iov_base,
> +			page_list->nr_pages,
> +			1,
> +			0,
> +			page_list->pages,
> +			NULL);

Yes, it has a lot of args.  It's nice to add comments like this:

		ret = get_user_pages(
			current,
			current->mm,
			(unsigned long) iov[i].iov_base,
			page_list->nr_pages,
			1,			/* write */
			0,			/* force */
			page_list->pages,
			NULL);


> +		up_read(&current->mm->mmap_sem);
> +
> +		if (ret != page_list->nr_pages) {
> +			goto mem_error;
> +		}

Unneded braces.

> +		local_list->nr_iovecs = i + 1;
> +	}
> +
> +	*locked_list = local_list;
> +	return 0;

Suggest you change this function to return locked_list, or an IS_ERR value
on error.

> +void dma_unlock_iovec_pages(struct dma_locked_list *locked_list)
> +{
> +	int i, j;
> +
> +	if (!locked_list)
> +		return;
> +
> +	for (i = 0; i < locked_list->nr_iovecs; i++) {
> +		struct dma_page_list *page_list = &locked_list->page_list[i];
> +		for (j = 0; j < page_list->nr_pages; j++) {
> +			SetPageDirty(page_list->pages[j]);
> +			page_cache_release(page_list->pages[j]);
> +		}
> +	}
> +
> +	kfree(locked_list);
> +}

SetPageDirty() is very wrong.  It fails to mark pagecache pages as dirty in
the radix tree so they won't get written back.

You'll need to use set_page_dirty_lock() here or, if you happen to have
protected the inode which backs this potential mmap (really the
address_space) from reclaim then set_page_dirty() will work.  Probably
it'll be set_page_dirty_lock().

If this is called from cant-sleep context then things get ugly.  If it's
called from interrupt context then moreso.  See fs/direct-io.c,
bio_set_pages_dirty(), bio_check_pages_dirty(), etc.


I don't see a check for "did we write to user pages" here.  Because we
don't need to dirty the pages if we were reading them (transmitting from
userspace).

But given that dma_lock_iovec_pages() is only set up for writing to
userspace I guess this code is implicitly receive-only.  It's hard to tell
when the description, is, like the code comments, so scant.

> +static dma_cookie_t dma_memcpy_tokerneliovec(struct dma_chan *chan, struct
> +	iovec *iov, unsigned char *kdata, size_t len)

You owe us two underscores ;)

> +/*
> + * We have already locked down the pages we will be using in the iovecs.

"pinned"

> + * Each entry in iov array has corresponding entry in locked_list->page_list.
> + * Using array indexing to keep iov[] and page_list[] in sync.
> + * Initial elements in iov array's iov->iov_len will be 0 if already copied into
> + *   by another call.
> + * iov array length remaining guaranteed to be bigger than len.
> + */
> +dma_cookie_t dma_memcpy_toiovec(struct dma_chan *chan, struct iovec *iov,
> +	struct dma_locked_list *locked_list, unsigned char *kdata, size_t len)
> +{
> +	int iov_byte_offset;
> +	int copy;
> +	dma_cookie_t dma_cookie = 0;
> +	int iovec_idx;
> +	int page_idx;
> +
> +	if (!chan)
> +		return memcpy_toiovec(iov, kdata, len);
> +
> +	/* -> kernel copies (e.g. smbfs) */
> +	if (!locked_list)
> +		return dma_memcpy_tokerneliovec(chan, iov, kdata, len);
> +
> +	iovec_idx = 0;
> +	while (iovec_idx < locked_list->nr_iovecs) {
> +		struct dma_page_list *page_list;
> +
> +		/* skip already used-up iovecs */
> +		while (!iov[iovec_idx].iov_len)
> +			iovec_idx++;

Is it assured that this array was zero-terminated?

> +
> +dma_cookie_t dma_memcpy_pg_toiovec(struct dma_chan *chan, struct iovec *iov,
> +	struct dma_locked_list *locked_list, struct page *page,
> +	unsigned int offset, size_t len)

pleeeeeze comment your code.

> +{
> +	int iov_byte_offset;
> +	int copy;
> +	dma_cookie_t dma_cookie = 0;
> +	int iovec_idx;
> +	int page_idx;
> +	int err;
> +
> +	/* this needs as-yet-unimplemented buf-to-buff, so punt. */
> +	/* TODO: use dma for this */
> +	if (!chan || !locked_list) {

Really you should rename locked_list to pinned_list throughout, and
dma_locked_list to dma_pinned_list.

> +	iovec_idx = 0;
> +	while (iovec_idx < locked_list->nr_iovecs) {
> +		struct dma_page_list *page_list;
> +
> +		/* skip already used-up iovecs */
> +		while (!iov[iovec_idx].iov_len)
> +			iovec_idx++;

Can this also run off the end?

> +int dma_lock_iovec_pages(struct iovec *iov, size_t len, struct dma_locked_list
> +	**locked_list)
> +{
> +	*locked_list = NULL;
> +
> +	return 0;
> +}
> +
> +void dma_unlock_iovec_pages(struct dma_locked_list* locked_list)
> +{ }

You might want to make these guys static inlines in a header and not
compile this file at all if !CONFIG_DMA_ENGINE.

> +struct dma_page_list
> +{

   struct dma_page_list {

> +struct dma_locked_list
> +{

   struct dma_pinned_list {

> +	int nr_iovecs;
> +	struct dma_page_list page_list[0];

We can use [] instead of [0] now that gcc-2.95.x has gone away.

> +int dma_lock_iovec_pages(struct iovec *iov, size_t len,
> +	struct dma_locked_list	**locked_list);
> +void dma_unlock_iovec_pages(struct dma_locked_list* locked_list);

"pin", "unpin".

> +#ifdef CONFIG_NET_DMA
> +
> +/**
> + *	dma_skb_copy_datagram_iovec - Copy a datagram to an iovec.
> + *	@skb - buffer to copy
> + *	@offset - offset in the buffer to start copying from
> + *	@iovec - io vector to copy to
> + *	@len - amount of data to copy from buffer to iovec
> + *	@locked_list - locked iovec buffer data
> + *
> + *	Note: the iovec is modified during the copy.

Modifying the caller's iovec is a bit rude.    Hard to avoid, I guess.

> + */
> +int dma_skb_copy_datagram_iovec(struct dma_chan *chan,
> +			struct sk_buff *skb, int offset, struct iovec *to,
> +			size_t len, struct dma_locked_list *locked_list)
> +{
> +	int start = skb_headlen(skb);
> +	int i, copy = start - offset;
> +	dma_cookie_t cookie = 0;
> +
> +	/* Copy header. */
> +	if (copy > 0) {
> +		if (copy > len)
> +			copy = len;
> +		if ((cookie = dma_memcpy_toiovec(chan, to, locked_list,
> +		     skb->data + offset, copy)) < 0)
> +			goto fault;
> +		if ((len -= copy) == 0)
> +			goto end;

Please avoid

	if ((lhs = rhs))

constructs.  Instead do

	lhs = rhs;
	if (lhs)

(entire patchset - there are quite a lot)

> +		offset += copy;
> +	}
> +
> +	/* Copy paged appendix. Hmm... why does this look so complicated? */
> +	for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
> +		int end;
> +
> +		BUG_TRAP(start <= offset + len);

<wonders why BUG_TRAP still exists>

> +		if ((copy = end - offset) > 0) {
> ...
> +			if (!(len -= copy))
> ...
> +			if ((copy = end - offset) > 0) {
> ...
> +				if ((len -= copy) == 0)
>

See above.

> +#else
> +
> +int dma_skb_copy_datagram_iovec(struct dma_chan *chan,
> +			const struct sk_buff *skb, int offset, struct iovec *to,
> +			size_t len, struct dma_locked_list *locked_list)
> +{
> +	return skb_copy_datagram_iovec(skb, offset, to, len);
> +}
> +
> +#endif

Again, consider putting this in a header as an inline, avoid compiling this
file altogether.

^ permalink raw reply

* Re: [PATCH 0/8] Intel I/O Acceleration Technology (I/OAT)
From: David S. Miller @ 2006-03-05  2:08 UTC (permalink / raw)
  To: johnpol; +Cc: jengelh, christopher.leech, linux-kernel, netdev
In-Reply-To: <20060305014324.GA20026@2ka.mipt.ru>

From: Evgeniy Polyakov <johnpol@2ka.mipt.ru>
Date: Sun, 5 Mar 2006 04:43:25 +0300

> According to investigation made for kevent based FS AIO reading,
> get_user_pages() performange graph looks like sqrt() function
> with plato starting on about 64-80 pages on Xeon 2.4Ghz with 1Gb of ram,
> while memcopy() is linear, so it can be noticebly slower than
> copy_to_user() if get_user_pages() is used aggressively, so userspace
> application must reuse the same, already grabbed buffer for maximum
> performance, but Intel folks did not provide theirs usage case and any
> benchmarks as far as I know.

Of course, and programming the DMA controller has overhead
as well.  This is why would would not use I/O AT with small
transfer sizes.

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).