Dev, Vasu wrote: >> -----Original Message----- >> From: Mike Christie [mailto:[EMAIL PROTECTED] >> Sent: Monday, August 04, 2008 5:45 PM >> To: Dev, Vasu >> Cc: [email protected] >> Subject: Re: [Open-FCoE] [PATCH 1/1] libfc: remove shared tag map and fix >> timer usage in fc_fcp.c (take 2) >> >> Dev, Vasu wrote: >>>> -----Original Message----- >>>> From: Mike Christie [mailto:[EMAIL PROTECTED] >>>> Sent: Sunday, August 03, 2008 11:51 PM >>>> To: Dev, Vasu >>>>> and that is a separate issue but in general what do you think on >>>>> calling into fc_exch.c with or without any lock held ? >>>>> >>>> For fc_fcp.c we just have those issues where we want to set the seq_ptr >>>> after we call exch_seq_send, so if there was some crazy case where we >>>> send the sequence, and it completes on a another thread before >>>> exch_seq_send returns the fc_fcp.c response handler is going to get >>>> confused. Does fc_exch.c prevent this type of race for the upper layer? >>> No, fc_exch.c cannot ensure this in current implementation. >>> >>> I think I didn't explain my self well in last response, my intent was >>> to get clarification to have any lock held or not before >>> calling into fc_exch.c functions including (exch_seq_send()). >>> >>> In fact, currently local port lock is held almost all the time when >>> calling into fc_exch.c, so not just changed fc_lun_reset_send() >>> calling into fc_exch.c with pkt lock held and some other code >>> doing same. Rob is re-working on local port locking which >>> may not require lock across fc_exch.c but not sure. >>> >>> I get your point that in some case upper layers need locking >>> across fc_exch.c and perhaps this cannot be avoided. >>> >> Hey so what is the exact issue here? > > No specific known issue to me in this regard, sees more > comments below on this. > >> Does the network layer sleep > > We are using dev_alloc_skb() in xmit() paths so network layer sleep > is not an issue AFAIK. > >> or is >> it something in our code or is it just a locking dependency issue? The >> fc_queuecommand/fc_fcp_send_cmd path can be run from the scsi softirrq >> so it cannot sleep. > > No specific known issue to me and I was just seeking for your > comments on calling into fc_exch.c with any lock held v/s not. > You have already answered your opinion on this in last response > http://www.open-fcoe.org/pipermail/devel/2008-August/000472.html. > > I was getting impression that you guys are heading for no lock held > in fc_exch.c xmit path since :- > > 1) Earlier you added this code in queuecommand call path :- > > spin_unlock_irq(lp->host->host_lock); > rc = fc_fcp_send_cmd(fsp); > spin_lock_irq(lp->host->host_lock); >
I didn't actually add this. I just moved it from fc_queuecommand. > However fps pkt lock is still held in this code path but later you > mentioned in one of the email to drop fsp pkt lock and then call > fc_exch.c. > > "I was tinkering with something like del_timer_sync where the fc_fcp.c > resp function would hold the pkt lock, set some state bit indicating > it is cleaning up the exchange, drop the pkt lock then call a fc_exch.c > function to clean up the exachnge" > > 2) I think Rob is also looking into not to lock local port when > sending frame down via fc_exch.c. However not fully sure on this. > > So I thought you guys are heading for no lock held in fc_exch.c > xmit path. Therefore I asked question to get > clarification to have lock or no lock before calling into > fc_exch.c or you got some plan on this. > Ah yeah, I have not been trying to remove the locking exactly, but that may be the best option. I mean I have not got that far yet on my todo/review comments. For the pkt lock we should be calling the _bh spin lock versions in process context because the timer and queuecommand function can run from softirq context. The timer function only needs to call the spin lock version, and actually the fc_queuecommand version can be called from the block soft irq or the block layer work queue. The problem with just leaving the irqs as they are and just using spin_lock/unlock in the fc_queuecommand path is that dev_queue_xmit needs irqs on due to how it messes with the bottom halves. And the problem with just doing spin_lock_bh from process context and calling into dev_queue_xmit is that I do not think it would be doing what we wanted it to. So yeah, the locking is screwed up in there and it is probably easiest to make it so we do not need to hold a lock. I just was not working on that yet. I was still working on the eh fixes :) > Now if I look back on thread start and I see that > I didn't write enough description of what I was thinking with > recent email info and changes from you and sought for your > general/design comments in wrong context since I can see > fsp pkt is still locked in other places (fc_queuecommand path) > apart from recently added fsp pkt lock in fc_lun_reset_send(), > so not any issue with this change. My bad for causing any > confusion on this. > No problem. _______________________________________________ devel mailing list [email protected] http://www.open-fcoe.org/mailman/listinfo/devel
