Messages in this thread | | | From | Cong Wang <> | Date | Mon, 2 Nov 2020 08:55:32 -0800 | Subject | Re: [PATCH v2 net] net: sch_generic: aviod concurrent reset and enqueue op for lockless qdisc |
| |
On Fri, Oct 30, 2020 at 12:38 AM Yunsheng Lin <linyunsheng@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.
| |