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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 28 Jan 2022 07:44:54 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Biao Huang <biao.huang@...iatek.com>
Cc:     David Miller <davem@...emloft.net>,
        Rob Herring <robh+dt@...nel.org>,
        Bartosz Golaszewski <brgl@...ev.pl>,
        Fabien Parent <fparent@...libre.com>,
        Felix Fietkau <nbd@....name>, John Crispin <john@...ozen.org>,
        Sean Wang <sean.wang@...iatek.com>,
        Mark Lee <Mark-MC.Lee@...iatek.com>,
        "Matthias Brugger" <matthias.bgg@...il.com>,
        <netdev@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-mediatek@...ts.infradead.org>,
        Yinghua Pan <ot_yinghua.pan@...iatek.com>,
        <srv_heupstream@...iatek.com>,
        Macpaul Lin <macpaul.lin@...iatek.com>
Subject: Re: [PATCH net-next v2 9/9] net: ethernet: mtk-star-emac: separate
 tx/rx handling with two NAPIs

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)
}

> > >  	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.