Hi, > -----Original Message----- > From: Ohad Ben-Cohen [mailto:ohad@xxxxxxxxxx] > Sent: Monday, June 07, 2010 4:41 PM > To: Chitriki Rudramuni, Deepak; Guzman Lugo, Fernando; Ramirez Luna, Omar; > Kanigeri, Hari > Cc: linux-omap@xxxxxxxxxxxxxxx; Hiroshi Doyu > Subject: Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo > > Hi Deepak, > > On Mon, Jun 7, 2010 at 1:52 PM, Deepak Chitriki <deepak.chitriki@xxxxxx> > wrote: > > With this patch I observed "inconsistent lock state" warning. > > Thanks for the report! > > > Kfifo is acccessed in omap_mbox_msg_send() and mbox_tx_tasklet() > > functions.In order to protect this critical section we need to protect > by > > using spin_lock_bh() so that the tasklet cannot preempt > > omap_mobx_msg_send(). > > This is actually not the problem: it's ok if mbox_tx_tasklet preempts > omap_mbox_msg_send. In fact, such a use case is even ok if we don't > take a spinlock at all: kfifo is designed in a way that if you have > only 1 consumer and 1 producer, they can both access kfifo > simultaneously without any locking. That's why we don't take the > spinlock in the mbox_tx_tasklet. The reason, btw, that we take a > spinlock in omap_mbox_msg_send is to allow multiple simultaneous > sending contexts (taking a spinlock there ensures serialization of > those multiple simultaneous sending contexts). > > The problem here lies in the fact (that I've just learnt) that > dspbridge also sends mbox messages from a tasklet context > (dpc_tasklet). Lockdep identifies that the spinlock is taken in a > softirq context (dspbridge's dpc_tasklet) after it was previously > taken in a softirq-enabled context. Your proposed fix will solve the > lockdep issue here, but: I think the best thing to do here is remove the spinlock, if not, you are preventing that omap_mbox_msg_send be executed from a tasklet or isr context. That maybe in this moment it is not called but it could be needed in the future. The caller of omap_mbox_msg_send should be take care of that function can not be call simultaneously. > > Do we really want to have tasklets in dspbridge ? Are we that > performance critical ? We are facing some issues when the DSP send the request for hibernation and we get that message and start processing the request (because it is done in a workqueue and there some latency). I issue had been manage in other ways but maybe we would have that issue it the hibernation request could be handle immediately. Why not remove the workqueue at all and let the mailbox module user be the judge for using tasklet, workqueue or do the job in the same isr (if it is small task and need to be fast)? > > In general I'd prefer to switch to workqueues in both dspbridge and > mailbox. I'm really not convinced we have to use tasklets in those > modules, and workqueues are much more system friendly. This way we > would also not have to stop all bottom halves when sending a mailbox > message. > > Somewhat relevant note about mailbox performance: omap_mbox_msg_send > often (i.e. when the kfifo is empty) can just send the message > directly, without triggering the tasklet to do that. Applying such a > change will substantially improve the mailbox performance, and will > make the shift to workqueues even more reasonable. I've got a patch > for that, I'll post it soon. What would be really good! Regards, Fernando. > > What do you think (looping in Fernando, Omar and Hari) ? > > Thanks, > Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
