On Thu, Nov 28, 2013 at 3:58 PM, Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
> 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.
>
Now I think I get your point. If it's struct fbr_lookup fbr[2], it's
good to combine the kmalloc, but it's struct fbr_lookup *fbr[2], so
it's not wise to combine the kmalloc.
I will resend the patch.
> 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
