Date: Sun, 25 Mar 2007 22:22:40 +0200 (CEST) From: Stefan Richter Subject: ieee1394: remove usage of skb_queue as packet queue This considerably reduces the memory requirements for a packet and eliminates ieee1394's dependency on CONFIG_NET. Signed-off-by: Stefan Richter --- Update: - Keep explicit packet->data_size = 0 initializations. - Add check for header size in raw1394. The old code relied on payload memory being appended directly after header memory. This assumption could break in the future. TODO: - Double-check if there are any drivers whose packet complete routine really needs process context. If there are none, get rid of khpsbpkt and execute the complete routine in the low-level driver's bottom half's context. - Check whether the complete packet really has to be zeroed when allocated. - Use kmem_cache for packets with up to 8 quadlets payload? Nodemgr issues bursts of quadlet read requests; sbp2 frequently sends octlet write requests. (For packets with bigger payloads, hpsb_alloc_packet may allocate extra data sections.) drivers/ieee1394/Kconfig | 1 drivers/ieee1394/hosts.c | 12 - drivers/ieee1394/hosts.h | 4 drivers/ieee1394/ieee1394_core.c | 291 ++++++++++++++++--------------- drivers/ieee1394/ieee1394_core.h | 27 -- drivers/ieee1394/raw1394.c | 3 6 files changed, 166 insertions(+), 172 deletions(-) Index: foo/drivers/ieee1394/Kconfig =================================================================== --- foo.orig/drivers/ieee1394/Kconfig +++ foo/drivers/ieee1394/Kconfig @@ -5,7 +5,6 @@ menu "IEEE 1394 (FireWire) support" config IEEE1394 tristate "IEEE 1394 (FireWire) support" depends on PCI || BROKEN - select NET help IEEE 1394 describes a high performance serial bus, which is also known as FireWire(tm) or i.Link(tm) and is used for connecting all Index: foo/drivers/ieee1394/hosts.c =================================================================== --- foo.orig/drivers/ieee1394/hosts.c +++ foo/drivers/ieee1394/hosts.c @@ -94,14 +94,6 @@ static int alloc_hostnum_cb(struct hpsb_ return 0; } -/* - * The pending_packet_queue is special in that it's processed - * from hardirq context too (such as hpsb_bus_reset()). Hence - * split the lock class from the usual networking skb-head - * lock class by using a separate key for it: - */ -static struct lock_class_key pending_packet_queue_key; - static DEFINE_MUTEX(host_num_alloc); /** @@ -137,9 +129,7 @@ struct hpsb_host *hpsb_alloc_host(struct h->hostdata = h + 1; h->driver = drv; - skb_queue_head_init(&h->pending_packet_queue); - lockdep_set_class(&h->pending_packet_queue.lock, - &pending_packet_queue_key); + INIT_LIST_HEAD(&h->pending_packets); INIT_LIST_HEAD(&h->addr_space); for (i = 2; i < 16; i++) Index: foo/drivers/ieee1394/hosts.h =================================================================== --- foo.orig/drivers/ieee1394/hosts.h +++ foo/drivers/ieee1394/hosts.h @@ -3,7 +3,6 @@ #include #include -#include #include #include #include @@ -25,8 +24,7 @@ struct hpsb_host { atomic_t generation; - struct sk_buff_head pending_packet_queue; - + struct list_head pending_packets; struct timer_list timeout; unsigned long timeout_interval; Index: foo/drivers/ieee1394/ieee1394_core.c =================================================================== --- foo.orig/drivers/ieee1394/ieee1394_core.c +++ foo/drivers/ieee1394/ieee1394_core.c @@ -30,7 +30,6 @@ #include #include #include -#include #include #include #include @@ -103,6 +102,8 @@ static void queue_packet_complete(struct * * Set the task that runs when a packet completes. You cannot call this more * than once on a single packet before it is sent. + * + * Typically, the complete @routine is responsible to call hpsb_free_packet(). */ void hpsb_set_packet_complete_task(struct hpsb_packet *packet, void (*routine)(void *), void *data) @@ -115,12 +116,12 @@ void hpsb_set_packet_complete_task(struc /** * hpsb_alloc_packet - allocate new packet structure - * @data_size: size of the data block to be allocated + * @data_size: size of the data block to be allocated, in bytes * * This function allocates, initializes and returns a new &struct hpsb_packet. - * It can be used in interrupt context. A header block is always included, its - * size is big enough to contain all possible 1394 headers. The data block is - * only allocated when @data_size is not zero. + * It can be used in interrupt context. A header block is always included and + * initialized with zeros. Its size is big enough to contain all possible 1394 + * headers. The data block is only allocated if @data_size is not zero. * * For packets for which responses will be received the @data_size has to be big * enough to contain the response's data block since no further allocation @@ -135,50 +136,42 @@ void hpsb_set_packet_complete_task(struc */ struct hpsb_packet *hpsb_alloc_packet(size_t data_size) { - struct hpsb_packet *packet = NULL; - struct sk_buff *skb; + struct hpsb_packet *packet; data_size = ((data_size + 3) & ~3); - skb = alloc_skb(data_size + sizeof(*packet), GFP_ATOMIC); - if (skb == NULL) + packet = kzalloc(sizeof(*packet) + data_size, GFP_ATOMIC); + if (!packet) return NULL; - memset(skb->data, 0, data_size + sizeof(*packet)); - - packet = (struct hpsb_packet *)skb->data; - packet->skb = skb; - - packet->header = packet->embedded_header; packet->state = hpsb_unused; packet->generation = -1; INIT_LIST_HEAD(&packet->driver_list); + INIT_LIST_HEAD(&packet->queue); atomic_set(&packet->refcnt, 1); if (data_size) { - packet->data = (quadlet_t *)(skb->data + sizeof(*packet)); - packet->data_size = data_size; + packet->data = packet->embedded_data; + packet->allocated_data_size = data_size; } - return packet; } - /** * hpsb_free_packet - free packet and data associated with it * @packet: packet to free (is NULL safe) * - * This function will free packet->data and finally the packet itself. + * Frees @packet->data only if it was allocated through hpsb_alloc_packet(). */ void hpsb_free_packet(struct hpsb_packet *packet) { if (packet && atomic_dec_and_test(&packet->refcnt)) { - BUG_ON(!list_empty(&packet->driver_list)); - kfree_skb(packet->skb); + BUG_ON(!list_empty(&packet->driver_list) || + !list_empty(&packet->queue)); + kfree(packet); } } - /** * hpsb_reset_bus - initiate bus reset on the given host * @host: host controller whose bus to reset @@ -494,6 +487,8 @@ void hpsb_selfid_complete(struct hpsb_ho highlevel_host_reset(host); } +static spinlock_t pending_packets_lock = SPIN_LOCK_UNLOCKED; + /** * hpsb_packet_sent - notify core of sending a packet * @@ -509,24 +504,24 @@ void hpsb_packet_sent(struct hpsb_host * { unsigned long flags; - spin_lock_irqsave(&host->pending_packet_queue.lock, flags); + spin_lock_irqsave(&pending_packets_lock, flags); packet->ack_code = ackcode; if (packet->no_waiter || packet->state == hpsb_complete) { /* if packet->no_waiter, must not have a tlabel allocated */ - spin_unlock_irqrestore(&host->pending_packet_queue.lock, flags); + spin_unlock_irqrestore(&pending_packets_lock, flags); hpsb_free_packet(packet); return; } atomic_dec(&packet->refcnt); /* drop HC's reference */ - /* here the packet must be on the host->pending_packet_queue */ + /* here the packet must be on the host->pending_packets queue */ if (ackcode != ACK_PENDING || !packet->expect_response) { packet->state = hpsb_complete; - __skb_unlink(packet->skb, &host->pending_packet_queue); - spin_unlock_irqrestore(&host->pending_packet_queue.lock, flags); + list_del_init(&packet->queue); + spin_unlock_irqrestore(&pending_packets_lock, flags); queue_packet_complete(packet); return; } @@ -534,7 +529,7 @@ void hpsb_packet_sent(struct hpsb_host * packet->state = hpsb_pending; packet->sendtime = jiffies; - spin_unlock_irqrestore(&host->pending_packet_queue.lock, flags); + spin_unlock_irqrestore(&pending_packets_lock, flags); mod_timer(&host->timeout, jiffies + host->timeout_interval); } @@ -609,12 +604,16 @@ int hpsb_send_packet(struct hpsb_packet WARN_ON(packet->no_waiter && packet->expect_response); if (!packet->no_waiter || packet->expect_response) { + unsigned long flags; + atomic_inc(&packet->refcnt); /* Set the initial "sendtime" to 10 seconds from now, to prevent premature expiry. If a packet takes more than 10 seconds to hit the wire, we have bigger problems :) */ packet->sendtime = jiffies + 10 * HZ; - skb_queue_tail(&host->pending_packet_queue, packet->skb); + spin_lock_irqsave(&pending_packets_lock, flags); + list_add_tail(&packet->queue, &host->pending_packets); + spin_unlock_irqrestore(&pending_packets_lock, flags); } if (packet->node_id == host->node_id) { @@ -690,86 +689,97 @@ static void send_packet_nocare(struct hp } } +static size_t packet_size_to_data_size(size_t packet_size, size_t header_size, + size_t buffer_size, int tcode) +{ + size_t ret = packet_size <= header_size ? 0 : packet_size - header_size; + + if (unlikely(ret > buffer_size)) + ret = buffer_size; + + if (unlikely(ret + header_size != packet_size)) + HPSB_ERR("unexpected packet size %d (tcode %d), bug?", + packet_size, tcode); + return ret; +} static void handle_packet_response(struct hpsb_host *host, int tcode, quadlet_t *data, size_t size) { - struct hpsb_packet *packet = NULL; - struct sk_buff *skb; - int tcode_match = 0; - int tlabel; + struct hpsb_packet *packet; + int tlabel = (data[0] >> 10) & 0x3f; + size_t header_size; unsigned long flags; - tlabel = (data[0] >> 10) & 0x3f; - - spin_lock_irqsave(&host->pending_packet_queue.lock, flags); - - skb_queue_walk(&host->pending_packet_queue, skb) { - packet = (struct hpsb_packet *)skb->data; - if ((packet->tlabel == tlabel) - && (packet->node_id == (data[1] >> 16))){ - break; - } - - packet = NULL; - } + spin_lock_irqsave(&pending_packets_lock, flags); - if (packet == NULL) { - HPSB_DEBUG("unsolicited response packet received - no tlabel match"); - dump_packet("contents", data, 16, -1); - spin_unlock_irqrestore(&host->pending_packet_queue.lock, flags); - return; - } + list_for_each_entry(packet, &host->pending_packets, queue) + if (packet->tlabel == tlabel && + packet->node_id == (data[1] >> 16)) + goto found; + + spin_unlock_irqrestore(&pending_packets_lock, flags); + HPSB_DEBUG("unsolicited response packet received - %s", + "no tlabel match"); + dump_packet("contents", data, 16, -1); + return; +found: switch (packet->tcode) { case TCODE_WRITEQ: case TCODE_WRITEB: - if (tcode != TCODE_WRITE_RESPONSE) + if (unlikely(tcode != TCODE_WRITE_RESPONSE)) break; - tcode_match = 1; - memcpy(packet->header, data, 12); - break; + header_size = 12; + size = 0; + goto dequeue; + case TCODE_READQ: - if (tcode != TCODE_READQ_RESPONSE) + if (unlikely(tcode != TCODE_READQ_RESPONSE)) break; - tcode_match = 1; - memcpy(packet->header, data, 16); - break; + header_size = 16; + size = 0; + goto dequeue; + case TCODE_READB: - if (tcode != TCODE_READB_RESPONSE) + if (unlikely(tcode != TCODE_READB_RESPONSE)) break; - tcode_match = 1; - BUG_ON(packet->skb->len - sizeof(*packet) < size - 16); - memcpy(packet->header, data, 16); - memcpy(packet->data, data + 4, size - 16); - break; + header_size = 16; + size = packet_size_to_data_size(size, header_size, + packet->allocated_data_size, + tcode); + goto dequeue; + case TCODE_LOCK_REQUEST: - if (tcode != TCODE_LOCK_RESPONSE) + if (unlikely(tcode != TCODE_LOCK_RESPONSE)) break; - tcode_match = 1; - size = min((size - 16), (size_t)8); - BUG_ON(packet->skb->len - sizeof(*packet) < size); - memcpy(packet->header, data, 16); - memcpy(packet->data, data + 4, size); - break; + header_size = 16; + size = packet_size_to_data_size(min(size, (size_t)(16 + 8)), + header_size, + packet->allocated_data_size, + tcode); + goto dequeue; } - if (!tcode_match) { - spin_unlock_irqrestore(&host->pending_packet_queue.lock, flags); - HPSB_INFO("unsolicited response packet received - tcode mismatch"); - dump_packet("contents", data, 16, -1); - return; - } + spin_unlock_irqrestore(&pending_packets_lock, flags); + HPSB_DEBUG("unsolicited response packet received - %s", + "tcode mismatch"); + dump_packet("contents", data, 16, -1); + return; - __skb_unlink(skb, &host->pending_packet_queue); +dequeue: + list_del_init(&packet->queue); + spin_unlock_irqrestore(&pending_packets_lock, flags); if (packet->state == hpsb_queued) { packet->sendtime = jiffies; packet->ack_code = ACK_PENDING; } - packet->state = hpsb_complete; - spin_unlock_irqrestore(&host->pending_packet_queue.lock, flags); + + memcpy(packet->header, data, header_size); + if (size) + memcpy(packet->data, data + 4, size); queue_packet_complete(packet); } @@ -783,6 +793,7 @@ static struct hpsb_packet *create_reply_ p = hpsb_alloc_packet(dsize); if (unlikely(p == NULL)) { /* FIXME - send data_error response */ + HPSB_ERR("out of memory, cannot send response packet"); return NULL; } @@ -832,7 +843,6 @@ static void fill_async_readblock_resp(st static void fill_async_write_resp(struct hpsb_packet *packet, int rcode) { PREP_ASYNC_HEAD_RCODE(TCODE_WRITE_RESPONSE); - packet->header[2] = 0; packet->header_size = 12; packet->data_size = 0; } @@ -1002,8 +1012,8 @@ void hpsb_packet_received(struct hpsb_ho { int tcode; - if (host->in_bus_reset) { - HPSB_INFO("received packet during reset; ignoring"); + if (unlikely(host->in_bus_reset)) { + HPSB_DEBUG("received packet during reset; ignoring"); return; } @@ -1037,23 +1047,27 @@ void hpsb_packet_received(struct hpsb_ho break; default: - HPSB_NOTICE("received packet with bogus transaction code %d", - tcode); + HPSB_DEBUG("received packet with bogus transaction code %d", + tcode); break; } } - static void abort_requests(struct hpsb_host *host) { - struct hpsb_packet *packet; - struct sk_buff *skb; + struct hpsb_packet *packet, *p; + struct list_head tmp; + unsigned long flags; host->driver->devctl(host, CANCEL_REQUESTS, 0); - while ((skb = skb_dequeue(&host->pending_packet_queue)) != NULL) { - packet = (struct hpsb_packet *)skb->data; + INIT_LIST_HEAD(&tmp); + spin_lock_irqsave(&pending_packets_lock, flags); + list_splice_init(&host->pending_packets, &tmp); + spin_unlock_irqrestore(&pending_packets_lock, flags); + list_for_each_entry_safe(packet, p, &tmp, queue) { + list_del_init(&packet->queue); packet->state = hpsb_complete; packet->ack_code = ACKX_ABORTED; queue_packet_complete(packet); @@ -1063,87 +1077,90 @@ static void abort_requests(struct hpsb_h void abort_timedouts(unsigned long __opaque) { struct hpsb_host *host = (struct hpsb_host *)__opaque; - unsigned long flags; - struct hpsb_packet *packet; - struct sk_buff *skb; - unsigned long expire; + struct hpsb_packet *packet, *p; + struct list_head tmp; + unsigned long flags, expire, j; spin_lock_irqsave(&host->csr.lock, flags); expire = host->csr.expire; spin_unlock_irqrestore(&host->csr.lock, flags); - /* Hold the lock around this, since we aren't dequeuing all - * packets, just ones we need. */ - spin_lock_irqsave(&host->pending_packet_queue.lock, flags); - - while (!skb_queue_empty(&host->pending_packet_queue)) { - skb = skb_peek(&host->pending_packet_queue); - - packet = (struct hpsb_packet *)skb->data; - - if (time_before(packet->sendtime + expire, jiffies)) { - __skb_unlink(skb, &host->pending_packet_queue); - packet->state = hpsb_complete; - packet->ack_code = ACKX_TIMEOUT; - queue_packet_complete(packet); - } else { + j = jiffies; + INIT_LIST_HEAD(&tmp); + spin_lock_irqsave(&pending_packets_lock, flags); + + list_for_each_entry_safe(packet, p, &host->pending_packets, queue) { + if (time_before(packet->sendtime + expire, j)) + list_move_tail(&packet->queue, &tmp); + else /* Since packets are added to the tail, the oldest * ones are first, always. When we get to one that * isn't timed out, the rest aren't either. */ break; - } } + if (!list_empty(&host->pending_packets)) + mod_timer(&host->timeout, j + host->timeout_interval); - if (!skb_queue_empty(&host->pending_packet_queue)) - mod_timer(&host->timeout, jiffies + host->timeout_interval); + spin_unlock_irqrestore(&pending_packets_lock, flags); - spin_unlock_irqrestore(&host->pending_packet_queue.lock, flags); + list_for_each_entry_safe(packet, p, &tmp, queue) { + list_del_init(&packet->queue); + packet->state = hpsb_complete; + packet->ack_code = ACKX_TIMEOUT; + queue_packet_complete(packet); + } } - -/* Kernel thread and vars, which handles packets that are completed. Only - * packets that have a "complete" function are sent here. This way, the - * completion is run out of kernel context, and doesn't block the rest of - * the stack. */ static struct task_struct *khpsbpkt_thread; -static struct sk_buff_head hpsbpkt_queue; +static LIST_HEAD(hpsbpkt_queue); static void queue_packet_complete(struct hpsb_packet *packet) { + unsigned long flags; + if (packet->no_waiter) { hpsb_free_packet(packet); return; } if (packet->complete_routine != NULL) { - skb_queue_tail(&hpsbpkt_queue, packet->skb); + spin_lock_irqsave(&pending_packets_lock, flags); + list_add_tail(&packet->queue, &hpsbpkt_queue); + spin_unlock_irqrestore(&pending_packets_lock, flags); wake_up_process(khpsbpkt_thread); } return; } +/* + * Kernel thread which handles packets that are completed. This way the + * packet's "complete" function is asynchronously run in process context. + * Only packets which have a "complete" function may be sent here. + */ static int hpsbpkt_thread(void *__hi) { - struct sk_buff *skb; - struct hpsb_packet *packet; - void (*complete_routine)(void*); - void *complete_data; + struct hpsb_packet *packet, *p; + struct list_head tmp; + int may_schedule; current->flags |= PF_NOFREEZE; while (!kthread_should_stop()) { - while ((skb = skb_dequeue(&hpsbpkt_queue)) != NULL) { - packet = (struct hpsb_packet *)skb->data; - complete_routine = packet->complete_routine; - complete_data = packet->complete_data; - - packet->complete_routine = packet->complete_data = NULL; - - complete_routine(complete_data); + INIT_LIST_HEAD(&tmp); + spin_lock_irq(&pending_packets_lock); + list_splice_init(&hpsbpkt_queue, &tmp); + spin_unlock_irq(&pending_packets_lock); + + list_for_each_entry_safe(packet, p, &tmp, queue) { + list_del_init(&packet->queue); + packet->complete_routine(packet->complete_data); } set_current_state(TASK_INTERRUPTIBLE); - if (!skb_peek(&hpsbpkt_queue)) + spin_lock_irq(&pending_packets_lock); + may_schedule = list_empty(&hpsbpkt_queue); + spin_unlock_irq(&pending_packets_lock); + if (may_schedule) schedule(); __set_current_state(TASK_RUNNING); } @@ -1154,8 +1171,6 @@ static int __init ieee1394_init(void) { int i, ret; - skb_queue_head_init(&hpsbpkt_queue); - /* non-fatal error */ if (hpsb_init_config_roms()) { HPSB_ERR("Failed to initialize some config rom entries.\n"); Index: foo/drivers/ieee1394/ieee1394_core.h =================================================================== --- foo.orig/drivers/ieee1394/ieee1394_core.h +++ foo/drivers/ieee1394/ieee1394_core.h @@ -4,7 +4,6 @@ #include #include #include -#include #include #include @@ -13,7 +12,7 @@ struct hpsb_packet { /* This struct is basically read-only for hosts with the exception of - * the data buffer contents and xnext - see below. */ + * the data buffer contents and driver_list. */ /* This can be used for host driver internal linking. * @@ -49,35 +48,27 @@ struct hpsb_packet { /* Speed to transmit with: 0 = 100Mbps, 1 = 200Mbps, 2 = 400Mbps */ unsigned speed_code:2; - /* - * *header and *data are guaranteed to be 32-bit DMAable and may be - * overwritten to allow in-place byte swapping. Neither of these is - * CRCed (the sizes also don't include CRC), but contain space for at - * least one additional quadlet to allow in-place CRCing. The memory is - * also guaranteed to be DMA mappable. - */ - quadlet_t *header; - quadlet_t *data; - size_t header_size; - size_t data_size; - struct hpsb_host *host; unsigned int generation; atomic_t refcnt; + struct list_head queue; /* Function (and possible data to pass to it) to call when this * packet is completed. */ void (*complete_routine)(void *); void *complete_data; - /* XXX This is just a hack at the moment */ - struct sk_buff *skb; - /* Store jiffies for implementing bus timeouts. */ unsigned long sendtime; - quadlet_t embedded_header[5]; + /* Sizes are in bytes. *data can be DMA-mapped. */ + size_t allocated_data_size; /* as allocated */ + size_t data_size; /* as filled in */ + size_t header_size; /* as filled in, not counting the CRC */ + quadlet_t *data; + quadlet_t header[5]; + quadlet_t embedded_data[0]; /* keep as last member */ }; void hpsb_set_packet_complete_task(struct hpsb_packet *packet, Index: foo/drivers/ieee1394/raw1394.c =================================================================== --- foo.orig/drivers/ieee1394/raw1394.c +++ foo/drivers/ieee1394/raw1394.c @@ -938,7 +938,8 @@ static int handle_async_send(struct file int header_length = req->req.misc & 0xffff; int expect_response = req->req.misc >> 16; - if ((header_length > req->req.length) || (header_length < 12)) { + if (header_length > req->req.length || header_length < 12 || + header_length > FIELD_SIZEOF(struct hpsb_packet, header)) { req->req.error = RAW1394_ERROR_INVALID_ARG; req->req.length = 0; queue_complete_req(req);