On Wed, Jul 05, 2023 at 01:42:31AM +0800, Chengfeng Ye wrote: > > Plus, we already in context where interrupts are stopped. > > Indeed they can be called from .ndo_start_xmit callback and > the document said it is with bh disabled. > > But I found some call chain from the user process that seems could > be called from irq disabled context. For sdma_send_txlist(), > there is a call chain. > > -> hfi1_write_iter() (.write_iter callback) > -> hfi1_user_sdma_process_request() > -> user_sdma_send_pkts() > -> sdma_send_txlist() > > The .write_iter seems not to disable irq by default, as mentioned by > https://www.kernel.org/doc/Documentation/filesystems/vfs.txt > And I didn't find any explicit disabling or bh or irq along the call path, > and also see several copy_from_usr() which cannot be invoked under > irq context. > > > For sdma_send_txreq(), there is a call chain. > > -> qp_priv_alloc() > -> iowait_init() (register _hfi1_do_tid_send() as a work queue) > -> _hfi1_do_tid_send() (workqueue) > -> hfi1_do_tid_send() > -> hfi1_verbs_send() > -> sr(qp, ps, 0) (sr could points to hfi1_verbs_send_dm()) > -> hfi1_verbs_send_dma() > -> sdma_send_txreq() > > _hfi1_do_tid_send() is a work queue without irq disabled by default, > I also check the remaining call path and also found that there is no explicit > irq disable, instead the call site of hfi1_verbs_send() is exactly after > spin_lock_irq_restore(), seems like a hint that it is probably called withirq > enable. Right, that path is called in process context and can sleep, there is no need in irq disabled variant there. > > Another hint is that the lock acquisition of > spin_lock_irqsave(&sde->tail_lock, flags); > just before my patch in the same function also disable irq, seems like another > hint that this function could be called with interrupt disable, Exactly, we already called to spin_lock_irqsave(), there is no value in doing it twice. void f() { spin_lock_irqsave(...) spin_lock_irqsave(...) .... spin_unlock_irqrestore(...) spin_unlock_irqrestore(...) } is exactly the same as void f() { spin_lock_irqsave(...) spin_lock(...) .... spin_unlock(...) spin_unlock_irqrestore(...) } Thanks
