On Thu, Nov 28, 2013 at 12:53:39AM +0800, ZHAO Gang wrote:
> @@ -2208,8 +2203,11 @@ static int et131x_rx_dma_memory_alloc(struct et131x_adapter *adapter)
> rx_ring = &adapter->rx_ring;
>
> /* Alloc memory for the lookup table */
> - rx_ring->fbr[0] = kmalloc(sizeof(struct fbr_lookup), GFP_KERNEL);
> - rx_ring->fbr[1] = kmalloc(sizeof(struct fbr_lookup), GFP_KERNEL);
> + rx_ring->fbr[0] = kmalloc(sizeof(struct fbr_lookup) * 2, GFP_KERNEL);
> + if (!rx_ring->fbr[0])
> + return -ENOMEM;
> +
> + rx_ring->fbr[1] = rx_ring->fbr[0] + 1;
>
This kind of double allocation is not very common in the kernel, but the
ones that exist make my life more complicated. My static checker
complains because it can't figure out the real "size" of fbr1. We
allocate a large buffer but really we want a small buffer. That's
confusing to static checkers and to people.
Doing confusing things makes sense if you have benchmarks to back it up.
Once you get your benchmarks then you put the confusing things in a
separate function with a comment.
int allocate_fbr_lookups(...)
{
/* Just do one allocation on the fast path */
}
int free_fbr_lookups(...)
{
}
That's how you do confusing things in the kernel.
Otherwise, if you don't have the benchmarks then just do two
allocations. It already annoys me when I see people do this and I don't
want to encourage more people to start "simplifying" code this way.
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
