* [PATCH v2] b44: fix misalignment and wasted space in rx handling
@ 2009-01-09 12:39 Felix Fietkau
2009-01-09 22:25 ` Michael Buesch
2009-01-10 7:08 ` David Miller
0 siblings, 2 replies; 4+ messages in thread
From: Felix Fietkau @ 2009-01-09 12:39 UTC (permalink / raw)
To: netdev; +Cc: Michael Buesch
Broadcom 4400 puts a header of configurable size (apparently needs
to be at least 28 bytes) in front of received packets. When handling
this, the previous code accidentally added the offset 30 *twice* for
the software and once for the hardware, thereby cancelling out the
IP alignment effect of the 30 byte padding and wasting an additional
30 bytes of memory per packet.
This patch fixes this problem and improves routing throughput by
about 30% on MIPS, where unaligned access is expensive.
Signed-off-by: Felix Fietkau <nbd@openwrt.org>
---
Sorry about the line wrapping in the previous mail. Should be fixed now
--- a/drivers/net/b44.c
+++ b/drivers/net/b44.c
@@ -73,8 +73,8 @@
(BP)->tx_cons - (BP)->tx_prod - TX_RING_GAP(BP))
#define NEXT_TX(N) (((N) + 1) & (B44_TX_RING_SIZE - 1))
-#define RX_PKT_OFFSET 30
-#define RX_PKT_BUF_SZ (1536 + RX_PKT_OFFSET + 64)
+#define RX_PKT_OFFSET (RX_HEADER_LEN + 2)
+#define RX_PKT_BUF_SZ (1536 + RX_PKT_OFFSET)
/* minimum number of free TX descriptors required to wake up TX process */
#define B44_TX_WAKEUP_THRESH (B44_TX_RING_SIZE / 4)
@@ -682,7 +682,6 @@ static int b44_alloc_rx_skb(struct b44 *
}
rh = (struct rx_header *) skb->data;
- skb_reserve(skb, RX_PKT_OFFSET);
rh->len = 0;
rh->flags = 0;
@@ -693,13 +692,13 @@ static int b44_alloc_rx_skb(struct b44 *
if (src_map != NULL)
src_map->skb = NULL;
- ctrl = (DESC_CTRL_LEN & (RX_PKT_BUF_SZ - RX_PKT_OFFSET));
+ ctrl = (DESC_CTRL_LEN & RX_PKT_BUF_SZ);
if (dest_idx == (B44_RX_RING_SIZE - 1))
ctrl |= DESC_CTRL_EOT;
dp = &bp->rx_ring[dest_idx];
dp->ctrl = cpu_to_le32(ctrl);
- dp->addr = cpu_to_le32((u32) mapping + RX_PKT_OFFSET + bp->dma_offset);
+ dp->addr = cpu_to_le32((u32) mapping + bp->dma_offset);
if (bp->flags & B44_FLAG_RX_RING_HACK)
b44_sync_dma_desc_for_device(bp->sdev, bp->rx_ring_dma,
@@ -809,8 +808,8 @@ static int b44_rx(struct b44 *bp, int bu
ssb_dma_unmap_single(bp->sdev, map,
skb_size, DMA_FROM_DEVICE);
/* Leave out rx_header */
- skb_put(skb, len + RX_PKT_OFFSET);
- skb_pull(skb, RX_PKT_OFFSET);
+ skb_put(skb, len + RX_PKT_OFFSET);
+ skb_pull(skb, RX_PKT_OFFSET);
} else {
struct sk_buff *copy_skb;
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v2] b44: fix misalignment and wasted space in rx handling
2009-01-09 12:39 [PATCH v2] b44: fix misalignment and wasted space in rx handling Felix Fietkau
@ 2009-01-09 22:25 ` Michael Buesch
2009-01-09 22:55 ` Felix Fietkau
2009-01-10 7:08 ` David Miller
1 sibling, 1 reply; 4+ messages in thread
From: Michael Buesch @ 2009-01-09 22:25 UTC (permalink / raw)
To: Felix Fietkau; +Cc: netdev, jgarzik, zambrano
On Friday 09 January 2009 13:39:57 Felix Fietkau wrote:
> --- a/drivers/net/b44.c
> +++ b/drivers/net/b44.c
> @@ -73,8 +73,8 @@
> (BP)->tx_cons - (BP)->tx_prod - TX_RING_GAP(BP))
> #define NEXT_TX(N) (((N) + 1) & (B44_TX_RING_SIZE - 1))
>
> -#define RX_PKT_OFFSET 30
> -#define RX_PKT_BUF_SZ (1536 + RX_PKT_OFFSET + 64)
> +#define RX_PKT_OFFSET (RX_HEADER_LEN + 2)
> +#define RX_PKT_BUF_SZ (1536 + RX_PKT_OFFSET)
>
> /* minimum number of free TX descriptors required to wake up TX process */
> #define B44_TX_WAKEUP_THRESH (B44_TX_RING_SIZE / 4)
> @@ -682,7 +682,6 @@ static int b44_alloc_rx_skb(struct b44 *
> }
>
> rh = (struct rx_header *) skb->data;
> - skb_reserve(skb, RX_PKT_OFFSET);
Looks correct.
> rh->len = 0;
> rh->flags = 0;
> @@ -693,13 +692,13 @@ static int b44_alloc_rx_skb(struct b44 *
> if (src_map != NULL)
> src_map->skb = NULL;
>
> - ctrl = (DESC_CTRL_LEN & (RX_PKT_BUF_SZ - RX_PKT_OFFSET));
> + ctrl = (DESC_CTRL_LEN & RX_PKT_BUF_SZ);
Are you sure this is right? b43 has the same DMA engine, and we
subtract the offset from the buffer size there. So we end up with
the descriptor control field telling the space available for the frame payload.
So I think the correct code would be
ctrl = (DESC_CTRL_LEN & (RX_PKT_BUF_SZ - RX_PKT_OFFSET));
> if (dest_idx == (B44_RX_RING_SIZE - 1))
> ctrl |= DESC_CTRL_EOT;
>
> dp = &bp->rx_ring[dest_idx];
> dp->ctrl = cpu_to_le32(ctrl);
> - dp->addr = cpu_to_le32((u32) mapping + RX_PKT_OFFSET + bp->dma_offset);
> + dp->addr = cpu_to_le32((u32) mapping + bp->dma_offset);
This looks correct.
--
Greetings, Michael.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] b44: fix misalignment and wasted space in rx handling
2009-01-09 22:25 ` Michael Buesch
@ 2009-01-09 22:55 ` Felix Fietkau
0 siblings, 0 replies; 4+ messages in thread
From: Felix Fietkau @ 2009-01-09 22:55 UTC (permalink / raw)
To: Michael Buesch; +Cc: netdev, jgarzik, zambrano
Michael Buesch wrote:
> On Friday 09 January 2009 13:39:57 Felix Fietkau wrote:
>> --- a/drivers/net/b44.c
>> +++ b/drivers/net/b44.c
>> @@ -73,8 +73,8 @@
>> (BP)->tx_cons - (BP)->tx_prod - TX_RING_GAP(BP))
>> #define NEXT_TX(N) (((N) + 1) & (B44_TX_RING_SIZE - 1))
>>
>> -#define RX_PKT_OFFSET 30
>> -#define RX_PKT_BUF_SZ (1536 + RX_PKT_OFFSET + 64)
>> +#define RX_PKT_OFFSET (RX_HEADER_LEN + 2)
>> +#define RX_PKT_BUF_SZ (1536 + RX_PKT_OFFSET)
>>
>> /* minimum number of free TX descriptors required to wake up TX process */
>> #define B44_TX_WAKEUP_THRESH (B44_TX_RING_SIZE / 4)
>> @@ -682,7 +682,6 @@ static int b44_alloc_rx_skb(struct b44 *
>> }
>>
>> rh = (struct rx_header *) skb->data;
>> - skb_reserve(skb, RX_PKT_OFFSET);
>
> Looks correct.
>
>> rh->len = 0;
>> rh->flags = 0;
>> @@ -693,13 +692,13 @@ static int b44_alloc_rx_skb(struct b44 *
>> if (src_map != NULL)
>> src_map->skb = NULL;
>>
>> - ctrl = (DESC_CTRL_LEN & (RX_PKT_BUF_SZ - RX_PKT_OFFSET));
>> + ctrl = (DESC_CTRL_LEN & RX_PKT_BUF_SZ);
>
> Are you sure this is right? b43 has the same DMA engine, and we
> subtract the offset from the buffer size there. So we end up with
> the descriptor control field telling the space available for the frame payload.
Quote from a Datasheet for the BCM5365 that I found online:
"This field is the length, in bytes, of the data buffer associated with this
descriptor. A descriptor with a BufCount value greater than 0x1000 causes a
descriptor protocol error."
I checked the public hnddma.c code and it also appears to not subtract
the packet offset either, so I think my version is correct.
- Felix
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] b44: fix misalignment and wasted space in rx handling
2009-01-09 12:39 [PATCH v2] b44: fix misalignment and wasted space in rx handling Felix Fietkau
2009-01-09 22:25 ` Michael Buesch
@ 2009-01-10 7:08 ` David Miller
1 sibling, 0 replies; 4+ messages in thread
From: David Miller @ 2009-01-10 7:08 UTC (permalink / raw)
To: nbd; +Cc: netdev, mb
From: Felix Fietkau <nbd@openwrt.org>
Date: Fri, 09 Jan 2009 13:39:57 +0100
> Broadcom 4400 puts a header of configurable size (apparently needs
> to be at least 28 bytes) in front of received packets. When handling
> this, the previous code accidentally added the offset 30 *twice* for
> the software and once for the hardware, thereby cancelling out the
> IP alignment effect of the 30 byte padding and wasting an additional
> 30 bytes of memory per packet.
>
> This patch fixes this problem and improves routing throughput by
> about 30% on MIPS, where unaligned access is expensive.
>
> Signed-off-by: Felix Fietkau <nbd@openwrt.org>
Applied.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2009-01-10 7:08 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-01-09 12:39 [PATCH v2] b44: fix misalignment and wasted space in rx handling Felix Fietkau
2009-01-09 22:25 ` Michael Buesch
2009-01-09 22:55 ` Felix Fietkau
2009-01-10 7:08 ` David Miller
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).