Re: [PATCH 0/7] isdn4linux: add drivers for Siemens Gigaset ISDN DECT PABX

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 

Thank you very much, Arjan, for your review of our code and your
extensive comments. We are working on taking them into account for the
next attempt at submitting the driver. Most of them are quite clear and
don't need discussing. Just a few remarks and questions:

On 27.02.2006, Arjan van de Ven wrote:
> as a general review remark: you seem to use a LOT of atomic variables.
> This I think is not too good an approach in general, because you get
> into all kinds of race situations if you need to access multiple (and
> you do).

I see. We'll try to reduce our atomic consumption. :-)

> In addition I've seen a lot of your code using 2 or more
> atomics in the same function, at which point it's most likely cheaper to
> just have a spinlock instead... (yes a single atomic is same cost as a
> spinlock, but once you do multiple in the same function the price is
> thus higher than a spinlock ;)

So you are saying that, for example

	spin_lock_irqsave(&cs->ev_lock, flags);
	head = cs->ev_head;
	tail = cs->ev_tail;
	spin_unlock_irqrestore(&cs->ev_lock, flags);

is (mutatis mutandis) actually cheaper than

	head = atomic_read(&cs->ev_head);
	tail = atomic_read(&cs->ev_tail);

? That's interesting. I wouldn't have expected that after reading
Documentation/atomic_ops.txt and Documentation/spinlock.txt.

>>+#define IFNULL(a) \
>>+       if (unlikely(!(a)))
> 
> please please get rid of this!
> (same goes for the variants of this just below this)

Ok, these were mainly debugging aids. We'll just drop them and let the
oops mechanism catch the (hopefully non-existent) remaining cases of
pointers being unexpectedly NULL.

>> +void gigaset_dbg_buffer(enum debuglevel level, const unsigned char *msg,
>> +			size_t len, const unsigned char *buf, int from_user)
> 
> such "from_user" parameter is highly evil, and also breaks sparse and
> friends.. (btw please run sparse on the code and fix all warnings)

Are you referring to anything in particular? We do run sparse regularly,
and it did not emit any warnings for the submitted version, not even for
this function. (But heaps of them for other parts of the kernel, if you
pardon the remark.)

>> +       spin_lock_irqsave(&cs->lock, flags);
>> +       ret = kmalloc(sizeof(struct at_state_t), GFP_ATOMIC);
>> +       if (ret) {
>> +               gigaset_at_init(ret, NULL, cs, cid);
> 
> if you move the kmalloc one line up, can it use GFP_KERNEL ?

Sorry but no - this is executed within a tasklet.

> (GFP_ATOMIC is evil in the sense that spurious use of it gives trouble
> for the VM)

Does that mean that every function doing kmalloc() and which may be
called from both interrupt and non-interrupt context needs a gfp_t flags
argument?

Thanks again for your time and effort
Tilman

-- 
Tilman Schmidt                          E-Mail: [email protected]
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
Ungeöffnet mindestens haltbar bis: (siehe Rückseite)

Attachment: signature.asc
Description: OpenPGP digital signature

[Index of Archives]     [Kernel Newbies]     [Netfilter]     [Bugtraq]     [Photo]     [Stuff]     [Gimp]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Video 4 Linux]     [Linux for the blind]     [Linux Resources]
  Powered by Linux