netdev - RE: [PATCH net-next 3/4] net: stmmac: Let TX and RX interrupts be independently enabled/disabled

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 for Android: free password hash cracker in your pocket

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

Date:   Mon, 16 Dec 2019 09:18:50 +0000
From:   Jose Abreu <Jose.Abreu@...opsys.com>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Joao Pinto <Joao.Pinto@...opsys.com>,
        Giuseppe Cavallaro <peppe.cavallaro@...com>,
        Alexandre Torgue <alexandre.torgue@...com>,
        "David S. Miller" <davem@...emloft.net>,
        Maxime Ripard <mripard@...nel.org>,
        Chen-Yu Tsai <wens@...e.org>,
        Maxime Coquelin <mcoquelin.stm32@...il.com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-stm32@...md-mailman.stormreply.com" 
        <linux-stm32@...md-mailman.stormreply.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH net-next 3/4] net: stmmac: Let TX and RX interrupts be
 independently enabled/disabled

From: Jakub Kicinski <jakub.kicinski@...ronome.com>
Date: Dec/14/2019, 20:36:23 (UTC+00:00)

> On Tue, 10 Dec 2019 20:54:43 +0100, Jose Abreu wrote:
> > @@ -2278,10 +2286,14 @@ static void stmmac_tx_timer(struct timer_list *t)
> >  	 * If NAPI is already running we can miss some events. Let's rearm
> >  	 * the timer and try again.
> >  	 */
> > -	if (likely(napi_schedule_prep(&ch->tx_napi)))
> > +	if (likely(napi_schedule_prep(&ch->tx_napi))) {
> > +		unsigned long flags;
> > +
> > +		spin_lock_irqsave(&ch->lock, flags);
> > +		stmmac_disable_dma_irq(priv, priv->ioaddr, ch->index, 0, 1);
> > +		spin_unlock_irqrestore(&ch->lock, flags);
> >  		__napi_schedule(&ch->tx_napi);
> > -	else
> > -		mod_timer(&tx_q->txtimer, STMMAC_COAL_TIMER(10));
> 
> You should also remove the comment above the if statement if it's
> really okay to no longer re-arm the timer. No?

Yeah, agreed!

> 
> > +	}
> >  }
> >  
> >  /**
> 
> > @@ -3759,24 +3777,18 @@ static int stmmac_napi_poll_tx(struct napi_struct *napi, int budget)
> >  	struct stmmac_channel *ch =
> >  		container_of(napi, struct stmmac_channel, tx_napi);
> >  	struct stmmac_priv *priv = ch->priv_data;
> > -	struct stmmac_tx_queue *tx_q;
> >  	u32 chan = ch->index;
> >  	int work_done;
> >  
> >  	priv->xstats.napi_poll++;
> >  
> > -	work_done = stmmac_tx_clean(priv, DMA_TX_SIZE, chan);
> > -	work_done = min(work_done, budget);
> > -
> > -	if (work_done < budget)
> > -		napi_complete_done(napi, work_done);
> > +	work_done = stmmac_tx_clean(priv, budget, chan);
> > +	if (work_done < budget && napi_complete_done(napi, work_done)) {
> 
> Not really related to this patch, but this looks a little suspicious. 
> I think the TX completions should all be processed regardless of the
> budget. The budget is for RX.

Well but this is a TX NAPI ... Shouldn't it be limited to prevent CPU 
starvation ?

---
Thanks,
Jose Miguel Abreu