netdev - Re: [PATCH net-next 03/10] net: mvneta: bm: add support for hardware buffer management

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]

Message-ID: <87lh7twfac.fsf@free-electrons.com>
Date:	Wed, 13 Jan 2016 18:38:35 +0100
From:	Gregory CLEMENT <gregory.clement@...e-electrons.com>
To:	Marcin Wojtas <mw@...ihalf.com>
Cc:	"David S. Miller" <davem@...emloft.net>,
	linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
	Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>,
	Florian Fainelli <f.fainelli@...il.com>,
	Jason Cooper <jason@...edaemon.net>,
	Andrew Lunn <andrew@...n.ch>,
	Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>,
	"linux-arm-kernel\@lists.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	Lior Amsalem <alior@...vell.com>,
	Nadav Haklai <nadavh@...vell.com>,
	Simon Guinot <simon.guinot@...uanux.org>,
	Ezequiel Garcia <ezequiel.garcia@...e-electrons.com>,
	Maxime Ripard <maxime.ripard@...e-electrons.com>,
	Boris BREZILLON <boris.brezillon@...e-electrons.com>,
	Russell King - ARM Linux <linux@....linux.org.uk>,
	Willy Tarreau <w@....eu>, Arnd Bergmann <arnd@...db.de>
Subject: Re: [PATCH net-next 03/10] net: mvneta: bm: add support for hardware buffer management

Hi Marcin,
 
 On mar., janv. 12 2016, Marcin Wojtas <mw@...ihalf.com> wrote:

> Hi Gregory,
>
> I have two remarks to my own code. Please let me know before patch v2,
> I will provide you with corrected version (I already did it locally).

OK I will asked your version before the next submitting.

Thanks,

Gregory

>
>> @@ -1556,17 +1777,20 @@ static void mvneta_rxq_drop_pkts(struct mvneta_port *pp,
>>         int rx_done, i;
>>
>>         rx_done = mvneta_rxq_busy_desc_num_get(pp, rxq);
>> +       if (rx_done)
>> +               mvneta_rxq_desc_num_update(pp, rxq, rx_done, rx_done);
>> +
>> +       if (pp->bm_priv)
>> +               return;
>> +
>
> This is wrong - buffers that are supposed to be dropped should return
> to bm_pool.
>
>> @@ -1587,23 +1812,35 @@ static int mvneta_rx(struct mvneta_port *pp, int rx_todo,
>>
>>         rx_done = 0;
>>
>> +       bm_in_use = pp->bm_priv ? true : false;
>> +
>>         /* Fairness NAPI loop */
>>         while (rx_done < rx_todo) {
>>                 struct mvneta_rx_desc *rx_desc = mvneta_rxq_next_desc_get(rxq);
>> +               struct mvneta_bm_pool *bm_pool = NULL;
>>                 struct sk_buff *skb;
>>                 unsigned char *data;
>>                 dma_addr_t phys_addr;
>> -               u32 rx_status;
>> +               u32 rx_status, frag_size;
>>                 int rx_bytes, err;
>> +               u8 pool_id;
>>
>>                 rx_done++;
>>                 rx_status = rx_desc->status;
>>                 rx_bytes = rx_desc->data_size - (ETH_FCS_LEN + MVNETA_MH_SIZE);
>>                 data = (unsigned char *)rx_desc->buf_cookie;
>>                 phys_addr = rx_desc->buf_phys_addr;
>> +               if (bm_in_use) {
>> +                       pool_id = MVNETA_RX_GET_BM_POOL_ID(rx_desc);
>> +                       bm_pool = &pp->bm_priv->bm_pools[pool_id];
>> +               }
>>
>>                 if (!mvneta_rxq_desc_is_first_last(rx_status) ||
>>                     (rx_status & MVNETA_RXD_ERR_SUMMARY)) {
>> +                       /* Return the buffer to the pool */
>> +                       if (bm_in_use)
>> +                               mvneta_bm_pool_put_bp(pp->bm_priv, bm_pool,
>> +                                                     rx_desc->buf_phys_addr);
>>                 err_drop_frame:
>>                         dev->stats.rx_errors++;
>>                         mvneta_rx_error(pp, rx_desc);
>> @@ -1633,25 +1870,38 @@ static int mvneta_rx(struct mvneta_port *pp, int rx_todo,
>>                         rcvd_pkts++;
>>                         rcvd_bytes += rx_bytes;
>>
>> +                       /* Return the buffer to the pool */
>> +                       if (bm_in_use)
>> +                               mvneta_bm_pool_put_bp(pp->bm_priv, bm_pool,
>> +                                                     rx_desc->buf_phys_addr);
>> +
>>                         /* leave the descriptor and buffer untouched */
>>                         continue;
>>                 }
>>
>>                 /* Refill processing */
>> -               err = mvneta_rx_refill(pp, rx_desc);
>> +               err = bm_in_use ? mvneta_bm_pool_refill(pp->bm_priv, bm_pool) :
>> +                                 mvneta_rx_refill(pp, rx_desc);
>>                 if (err) {
>>                         netdev_err(dev, "Linux processing - Can't refill\n");
>>                         rxq->missed++;
>>                         goto err_drop_frame;
>
> Wrong - on refill fail, original buffer should be returned to bm_pool.
> Same case is, when netdev_alloc_skb_ip_align fail inside copybreak
> (this should also be fixed).
>
> Best regards,
> Marcin

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com