On Fri, Oct 30, 2020 at 12:38 AM Yunsheng Lin <linyunsh...@huawei.com> wrote: > > On 2020/10/30 3:05, Cong Wang wrote: > > > > I do not see how and why it should. synchronize_net() is merely an optimized > > version of synchronize_rcu(), it should wait for RCU readers, softirqs are > > not > > necessarily RCU readers, net_tx_action() does not take RCU read lock either. > > Ok, make sense. > > Taking RCU read lock in net_tx_action() does not seems to solve the problem, > what about the time window between __netif_reschedule() and net_tx_action()? > > It seems we need to re-dereference the qdisc whenever RCU read lock is > released > and qdisc is still in sd->output_queue or wait for the sd->output_queue to > drain?
Not suggesting you to take RCU read lock. We already wait for TX action with a loop of sleep. To me, the only thing missing is just moving the reset after that wait. > >>>> If we do any additional reset that is not related to qdisc in > >>>> dev_reset_queue(), we > >>>> can move it after some_qdisc_is_busy() checking. > >>> > >>> I am not suggesting to do an additional reset, I am suggesting to move > >>> your reset after the busy waiting. > >> > >> There maybe a deadlock here if we reset the qdisc after the > >> some_qdisc_is_busy() checking, > >> because some_qdisc_is_busy() may require the qdisc reset to clear the skb, > >> so that > > > > some_qdisc_is_busy() checks the status of qdisc, not the skb queue. > > Is there any reason why we do not check the skb queue in the dqisc? > It seems there may be skb left when netdev is deactivated, maybe at least warn > about that when there is still skb left when netdev is deactivated? > Is that why we call qdisc_reset() to clear the leftover skb in > qdisc_destroy()? > > > > > > >> some_qdisc_is_busy() can return false. I am not sure this is really a > >> problem, but > >> sch_direct_xmit() may requeue the skb when dev_hard_start_xmit return > >> TX_BUSY. > > > > Sounds like another reason we should move the reset as late as possible? > > Why? You said "sch_direct_xmit() may requeue the skb", I agree. I assume you mean net_tx_action() calls sch_direct_xmit() which does the requeue then races with reset. No? > > There current netdev down order is mainly below: > > netif_tx_stop_all_queues() > > dev_deactivate_queue() > > synchronize_net() > > dev_reset_queue() > > some_qdisc_is_busy() > > > You suggest to change it to below order, right? > > netif_tx_stop_all_queues() > > dev_deactivate_queue() > > synchronize_net() > > some_qdisc_is_busy() > > dev_reset_queue() Yes. > > > What is the semantics of some_qdisc_is_busy()? Waiting for flying TX action. > From my understanding, we can do anything about the old qdisc (including > destorying the old qdisc) after some_qdisc_is_busy() return false. But the current code does the reset _before_ some_qdisc_is_busy(). ;) Thanks.