On Fri, 2006-03-03 at 00:03 +0100, Tilman Schmidt wrote:
> 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);
atomic_read is special since it's not actually an atomic operation ;)
but.. think about it: you do 2 atomic reads, however there is ZERO
guarantee that the reads are atomic with respect to eachother; eg your
head and tail are not an atomic "snapshot" of these 2 variables!
> >>+#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.
you can also use WARN_ON() and BUG_ON() for that, you then get a more
readable oops message (with filename and line information)
>
> >> +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.)
msg should have the __user atribute here since it can be in userspace...
sometimes. It is the "sometimes" that is the bad idea!
>
> >> + 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.
ok fair enough ;)
>
> > (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?
well that's the other extreme. But if it's going to be a major source of
memory allocations, yes. If it's only sometimes, or "a few", then no.
For example if your tasklet function allocates one, and then frees it
before being done, I don't see a problem. It becomes a problem when
there will be many of these, and when they have longer lifetimes,
because then the vm can become starved of memory before it has a chance
to do correct the memory imbalance.
(GFP_ATOMIC is like borrowing from the VM, the VM will be in slight
imbalance afterwards. With GFP_KERNEL you allow the kernel to fix this
imbalance. A slight imbalance is fine and not a problem. Especially if
you give it the memory back soon. But if the imbalance can accumulate,
for example because you keep allocating and free the memory much later,
it can become a problem)
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[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]
