lkml.org 
[lkml]   [2006]   [Mar]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 0/7] isdn4linux: add drivers for Siemens Gigaset ISDN DECT PABX
    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: tilman@imap.cc
    Bonn, Germany
    Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
    Ungeöffnet mindestens haltbar bis: (siehe Rückseite)

    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2006-03-03 00:05    [W:3.172 / U:0.044 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site