Messages in this thread | | | Subject | Re: [PATCH 0/7] isdn4linux: add drivers for Siemens Gigaset ISDN DECT PABX | From | Arjan van de Ven <> | Date | Fri, 03 Mar 2006 07:53:28 +0100 |
| |
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |