2013/10/29 Nathan Hintz <[email protected]>:
> On Mon, 28 Oct 2013 18:42:22 +0100
> Rafał Miłecki <[email protected]> wrote:
>
> Hi:
>
> A few questions/comments inline...
>
> Nathan
>
>> Copying whole packets with skb_copy_from_linear_data_offset is a pretty
>> bad idea. CPU was spending time in __copy_user_common and network
>> performance was lower. With the new solution iperf-measured speed
>> increased from 116Mb/s to 134Mb/s.
>>
>> Another way to improve performance could be switching to build_skb. It
>> is cache-specific, so will require testing of various devices.
>>
>> Signed-off-by: Rafał Miłecki <[email protected]>
>> ---
>> drivers/net/ethernet/broadcom/bgmac.c | 71
>> ++++++++++++++++++++-------------
>> 1 file changed, 44 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/broadcom/bgmac.c
>> b/drivers/net/ethernet/broadcom/bgmac.c
>> index 6b7541f..fde9a11 100644
>> --- a/drivers/net/ethernet/broadcom/bgmac.c
>> +++ b/drivers/net/ethernet/broadcom/bgmac.c
>> @@ -307,7 +307,6 @@ static int bgmac_dma_rx_read(struct bgmac *bgmac, struct
>> bgmac_dma_ring *ring,
>> struct device *dma_dev = bgmac->core->dma_dev;
>> struct bgmac_slot_info *slot = &ring->slots[ring->start];
>> struct sk_buff *skb = slot->skb;
>> - struct sk_buff *new_skb;
>> struct bgmac_rx_header *rx;
>> u16 len, flags;
>>
>> @@ -320,38 +319,56 @@ static int bgmac_dma_rx_read(struct bgmac *bgmac,
>> struct bgmac_dma_ring *ring,
>> len = le16_to_cpu(rx->len);
>> flags = le16_to_cpu(rx->flags);
>>
>> - /* Check for poison and drop or pass the packet */
>> - if (len == 0xdead && flags == 0xbeef) {
>> - bgmac_err(bgmac, "Found poisoned packet at slot %d,
>> DMA issue!\n",
>> - ring->start);
>> - } else {
>> + do {
>
> "old_skb" duplicates "skb" stored above, can that one be used (renamed)
> instead of creating a new one here?
I've focused on clean code too much. That won't be needed anyway when
I rebase my patch on top of yours.
>> + struct sk_buff *old_skb = slot->skb;
>> + dma_addr_t old_dma_addr = slot->dma_addr;
>> + int err;
>> +
>> + /* Check for poison and drop or pass the packet */
>> + if (len == 0xdead && flags == 0xbeef) {
>> + bgmac_err(bgmac, "Found poisoned packet at
>> slot %d, DMA issue!\n",
>> + ring->start);
>> + dma_sync_single_for_device(dma_dev,
>> + slot->dma_addr,
>> + BGMAC_RX_BUF_SIZE,
>> + DMA_FROM_DEVICE);
>
> Nothing in the buffer has been changed by the cpu yet, so is this sync
> necessary?
I've moved your comment a line below, so it comments the code *above*.
I'm using LDD3 to understand DMA and it contains this explanation:
> void dma_sync_single_for_cpu(struct device *dev, dma_handle_t bus_addr,
> size_t size, enum dma_data_direction direction);
>
> This function should be called before the processor accesses a streaming DMA
> buffer. Once the call has been made, the CPU “owns” the DMA buffer and can
> work with it as needed. Before the device accesses the buffer, however,
> ownership should be transferred back to it with:
>
> void dma_sync_single_for_device(struct device *dev, dma_handle_t bus_addr,
> size_t size, enum dma_data_direction direction);
So even if I didn't change anything in the buffer, I believe we still
need to "sync" it back to make it accessible to the hardware.
> I've sent a separate patch against "bgmac_dma_rx_skb_for_slot" to not corrupt
> the slot at all if an
> error occurs (skb alloc or dma mapping), and free the skb that was allocated
> if a dma mapping error
> occurs. Assuming that patch is accepted, then the following two lines would
> not be needed.
> With "bgmac_dma_rx_skb_for_slot" as it currently exists, this would leak an
> skb for a dma mapping
> error (this was pre-existing to the changes in this patch).
Thanks, I'll rebase my patch.
_______________________________________________
openwrt-devel mailing list
[email protected]
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
