Re: [PATCH net-next v2 9/9] net: ethernet: mtk-star-emac: separate tx/rx handling with two NAPIs

[Biao Huang <biao.huang@mediatek.com>]

Dear Jakub,
	Thanks for your comments~

On Fri, 2022-01-28 at 07:44 -0800, Jakub Kicinski wrote:
> On Fri, 28 Jan 2022 15:05:27 +0800 Biao Huang wrote:
> > > > + * Description : this is the driver interrupt service routine.
> > > > + * it mainly handles:
> > > > + *  1. tx complete interrupt for frame transmission.
> > > > + *  2. rx complete interrupt for frame reception.
> > > > + *  3. MAC Management Counter interrupt to avoid counter
> > > > overflow.
> > > >   */
> > > >  static irqreturn_t mtk_star_handle_irq(int irq, void *data)
> > > >  {
> > > > -	struct mtk_star_priv *priv;
> > > > -	struct net_device *ndev;
> > > > +	struct net_device *ndev = data;
> > > > +	struct mtk_star_priv *priv = netdev_priv(ndev);
> > > > +	unsigned int intr_status = mtk_star_intr_ack_all(priv);
> > > > +	unsigned long flags = 0;
> > > > +
> > > > +	if (intr_status & MTK_STAR_BIT_INT_STS_FNRC) {
> > > > +		if (napi_schedule_prep(&priv->rx_napi)) {
> > > > +			spin_lock_irqsave(&priv->lock, flags);
> > > > +			/* mask Rx Complete interrupt */
> > > > +			mtk_star_disable_dma_irq(priv, true,
> > > > false);
> > > > +			spin_unlock_irqrestore(&priv->lock,
> > > > flags);
> > > > +			__napi_schedule_irqoff(&priv->rx_napi);
> > > > +		}
> > > > +	}
> > > >  
> > > > -	ndev = data;
> > > > -	priv = netdev_priv(ndev);
> > > > +	if (intr_status & MTK_STAR_BIT_INT_STS_TNTC) {
> > > > +		if (napi_schedule_prep(&priv->tx_napi)) {
> > > > +			spin_lock_irqsave(&priv->lock, flags);
> > > > +			/* mask Tx Complete interrupt */
> > > > +			mtk_star_disable_dma_irq(priv, false,
> > > > true);
> > > > +			spin_unlock_irqrestore(&priv->lock,
> > > > flags);
> > > > +			__napi_schedule_irqoff(&priv->tx_napi);
> > > > +		}
> > > > +	}  
> > > 
> > > Seems a little wasteful to retake the same lock twice if two IRQ
> > > sources fire at the same time.  
> > 
> > The TX/RX irq control bits are in the same register,
> > but they are triggered independently.
> > So it seems necessary to protect the register
> > access with a spin lock.
> 
> This is what I meant:
> 
> rx = (status & RX) && napi_schedule_prep(rx_napi);
> tx = (status & TX) && napi_schedule_prep(tx_napi);
> 
> if (rx || tx) {
> 	spin_lock()
> 	disable_irq(priv, rx, tx);	
> 	spin_unlock();
> 	if (rx)
> 		__napi_schedule_irqoff(rx_napi)
> 	if (tx)
> 		__napi_schedule_irqoff(tx_napi)
> }
> 
OK, We'll adopt your suggestion, and corresponding modification will be
added in next send.
> > > >  	desc_data.dma_addr = mtk_star_dma_map_tx(priv, skb);
> > > >  	if (dma_mapping_error(dev, desc_data.dma_addr))
> > > > @@ -1050,18 +1103,10 @@ static int
> > > > mtk_star_netdev_start_xmit(struct sk_buff *skb,
> > > >  
> > > >  	desc_data.skb = skb;
> > > >  	desc_data.len = skb->len;
> > > > -
> > > > -	spin_lock_bh(&priv->lock);
> > > > 
> > > >  	mtk_star_ring_push_head_tx(ring, &desc_data);
> > > >  
> > > >  	netdev_sent_queue(ndev, skb->len);
> > > >  
> > > > -	if (mtk_star_ring_full(ring))
> > > > -		netif_stop_queue(ndev);  
> > > 
> > > Are you stopping the queue in advance somewhere else now? Did you
> > > only
> > > test this with BQL enabled? Only place that stops the ring also
> > > prints
> > > a loud warning now AFAICS..  
> > 
> > No.
> > 
> > We modify the ring full condition, and will not invoke
> > netif_stop_queue
> > if queue is already stopped.
> 
> I don't understand what you're saying.
> 
> > Test pass no matter whether BQL is enabled or disabled.
> > 
> > It's much safer to judge queue is full or not at the beginning of
> > start_xmit() to avoid invalid setting.
> 
> Drivers are expected to stop their queues at the end of xmit routine
> if
> the ring can't accommodate another frame. It's more efficient to stop
> the queues early than have to put skbs already dequeued from the
> qdisc
> layer back into the qdiscs.
Yes, if descriptors ring is full, it's meaningful to stop the queue 
at the end of xmit; 
But driver seems hard to know how many descriptors the next skb will
request, e.g. 3 descriptors are available for next round send, but the
next skb may need 4 descriptors, in this case, we still need judge
whether descriptors are enough for skb transmission, then decide stop
the queue or not, at the beginning of xmit routine.

Maybe we should judge ring is full or not at the beginning and the end
of xmit routine(seems a little redundancy).

Regards~