Re: [PATCH net-next v3 1/2] net: implement dma cache skb allocator

[Ben Hutchings <ben@decadent.org.uk>]

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

On Tue, 2015-03-10 at 23:13 +0530, Govindarajulu Varadarajan wrote:
[...]
> +static struct netdev_dma_node *netdev_dma_alloc(struct netdev_dma_head *nc_head,
> +						size_t sz)
> +{
> +	struct netdev_dma_node *nc = nc_head->nc;
> +	int offset;
> +
> +	if (unlikely(!nc)) {
> +refill:
> +		__dma_cache_refill(nc_head, sz);
> +		nc = nc_head->nc;
> +		if (unlikely(!nc))
> +			return NULL;
> +	}
> +
> +	offset = nc->frag.offset - sz;
> +	if (offset < 0) {
> +		if (!atomic_sub_and_test(nc->pagecnt_bias,
> +					 &nc->frag.page->_count)) {
> +			/* the caller has processed all the frags belonging to
> +			 * this page. Since page->_count is not 0 and
> +			 * nc->dma_count is 0 these frags should be in stack.
> +			 * We should unmap the page here.

Unmapping potentially copies data back from a bounce buffer (even if
it's already been synchronised).  This is OK in the case of a buffer
that's attached to the skb as a page fragment, because in that case it's
read-only for the stack.  It's not OK for a buffer that has been passed
to build_skb(), as this may legitimately have been changed by the stack
and this will overwrite the changes.

So you have to choose between DMA buffer recycling and build_skb().

> +			 */
> +			if (!nc->dma_count) {
> +				dma_unmap_single(nc_head->dev, nc->dma_addr,
> +						 nc->frag.size,
> +						 DMA_FROM_DEVICE);
[...]

Don't mix dma_map_page() and dma_unmap_single().

Ben.

-- 
Ben Hutchings
Q.  Which is the greater problem in the world today, ignorance or apathy?
A.  I don't know and I couldn't care less.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]