[PATCH v2 1/4] staging: et131x: simplify rx dma code

ZHAO Gang gamerh2o at gmail.com
Thu Nov 28 08:59:05 UTC 2013


On Thu, Nov 28, 2013 at 3:58 PM, Dan Carpenter <dan.carpenter at oracle.com> 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
>


More information about the devel mailing list