netdev - Re: [PATCH v2 net 2/2] dpaa2-eth: Fix TX FQID values

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] [day] [month] [year] [list]

Message-ID: <VI1PR0402MB28001CD49E92E461531DC24BE0930@VI1PR0402MB2800.eurprd04.prod.outlook.com>
Date:   Tue, 15 Oct 2019 20:07:49 +0000
From:   Ioana Ciornei <ioana.ciornei@....com>
To:     Andrew Lunn <andrew@...n.ch>,
        Jakub Kicinski <jakub.kicinski@...ronome.com>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Ioana Ciocoi Radulescu <ruxandra.radulescu@....com>
Subject: Re: [PATCH v2 net 2/2] dpaa2-eth: Fix TX FQID values

On 10/15/19 10:47 PM, Andrew Lunn wrote:
> On Tue, Oct 15, 2019 at 12:40:17PM -0700, Jakub Kicinski wrote:
>> On Tue, 15 Oct 2019 21:29:23 +0200, Andrew Lunn wrote:
>>>> diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
>>>> index 5acd734a216b..c3c2c06195ae 100644
>>>> --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
>>>> +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
>>>> @@ -1235,6 +1235,8 @@ static void dpaa2_eth_set_rx_taildrop(struct dpaa2_eth_priv *priv, bool enable)
>>>>   	priv->rx_td_enabled = enable;
>>>>   }
>>>>   
>>>> +static void update_tx_fqids(struct dpaa2_eth_priv *priv);
>>>> +
>>>
>>> Hi Ioana and Ioana
>>>
>>> Using forward declarations is generally not liked. Is there something
>>> which is preventing you from having it earlier in the file?
>>
>> Ha! I was just about to ask the same question 😊
>>
>> +out_err:
>> +	netdev_info(priv->net_dev,
>> +		    "Error reading Tx FQID, fallback to QDID-based enqueue");
>> +	priv->enqueue = dpaa2_eth_enqueue_qd;
>> +}
>>
>> Here dpaa2_eth_enqueue_qd is a function pointer which is is defined
>> towards the end of the file :S
> 
> Hi Jakub
> 
> Thanks, i was too lazy to look.
> 
> So this is O.K. for net, since fixes should be minimal.
> 
> Ioana's please could you submit a patch to net-next, once this has
> been merged, to move the code around to remove the forward
> declarations.


Yes, I'll cleanup this in net-next.

Also, no more Ioana's for a while, just me :)

> 
>> Also can I point out that this:
>>
>> static inline int dpaa2_eth_enqueue_qd(struct dpaa2_eth_priv *priv,
>>                                         struct dpaa2_eth_fq *fq,
>>                                         struct dpaa2_fd *fd, u8 prio)
>> {
>>          return dpaa2_io_service_enqueue_qd(fq->channel->dpio,
>>                                             priv->tx_qdid, prio,
>>                                             fq->tx_qdbin, fd);
>> }
>>                                                                                  
>> static inline int dpaa2_eth_enqueue_fq(struct dpaa2_eth_priv *priv,
>>                                         struct dpaa2_eth_fq *fq,
>>                                         struct dpaa2_fd *fd, u8 prio)
>> {
>>          return dpaa2_io_service_enqueue_fq(fq->channel->dpio,
>>                                             fq->tx_fqid[prio], fd);
>> }
>>                                                                                  
>> static void set_enqueue_mode(struct dpaa2_eth_priv *priv)
>> {
>>          if (dpaa2_eth_cmp_dpni_ver(priv, DPNI_ENQUEUE_FQID_VER_MAJOR,
>>                                     DPNI_ENQUEUE_FQID_VER_MINOR) < 0)
>>                  priv->enqueue = dpaa2_eth_enqueue_qd;
>>          else
>>                  priv->enqueue = dpaa2_eth_enqueue_fq;
>> }
>>
>> Could be the most pointless use of the inline keyword possible :)
>> Both dpaa2_eth_enqueue_qd() and dpaa2_eth_enqueue_fq() are only ever
>> called via a pointer..

I think that's another change for net-next.

> 
> Is priv->enqueue used on the hotpath? SPECTRA/Meltdown etc make that
> expensive.
> 

Yes, the priv->enqueue is used on the Tx path to enqueue a skb 
(transformed into a frame descriptor) into a queue.
I need to benchmark if transforming the callback into an if-else 
statement is helping.

Ioana C

> 	Andrew
>