netdev - Re: [RFC 5/9] staging: dpaa2-switch: handle Rx path on control interface

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: <20201105112258.sgbr2fq66u47vokr@skbuf>
Date:   Thu, 5 Nov 2020 13:22:58 +0200
From:   Ioana Ciornei <ciorneiioana@...il.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Ioana Ciornei <ciorneiioana@...il.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
        Ioana Ciornei <ioana.ciornei@....com>
Subject: Re: [RFC 5/9] staging: dpaa2-switch: handle Rx path on control
 interface

On Thu, Nov 05, 2020 at 01:45:16AM +0100, Andrew Lunn wrote:
> > +/* Manage all NAPI instances for the control interface.
> > + *
> > + * We only have one RX queue and one Tx Conf queue for all
> > + * switch ports. Therefore, we only need to enable the NAPI instance once, the
> > + * first time one of the switch ports runs .dev_open().
> > + */
> > +
> > +static void dpaa2_switch_enable_ctrl_if_napi(struct ethsw_core *ethsw)
> > +{
> > +	int i;
> > +
> > +	/* a new interface is using the NAPI instance */
> > +	ethsw->napi_users++;
> > +
> > +	/* if there is already a user of the instance, return */
> > +	if (ethsw->napi_users > 1)
> > +		return;
> 
> Does there need to be any locking here? Or does it rely on RTNL?
> Maybe a comment would be nice, or a check that RTNL is actually held.
> 

It relies on the RTNL. I'll add an assert on the RTNL lock and a comment
to go with that.

> > +
> > +	if (!dpaa2_switch_has_ctrl_if(ethsw))
> > +		return;
> > +
> > +	for (i = 0; i < DPAA2_SWITCH_RX_NUM_FQS; i++)
> > +		napi_enable(&ethsw->fq[i].napi);
> > +}
> 
> > +static void dpaa2_switch_rx(struct dpaa2_switch_fq *fq,
> > +			    const struct dpaa2_fd *fd)
> > +{
> > +	struct ethsw_core *ethsw = fq->ethsw;
> > +	struct ethsw_port_priv *port_priv;
> > +	struct net_device *netdev;
> > +	struct vlan_ethhdr *hdr;
> > +	struct sk_buff *skb;
> > +	u16 vlan_tci, vid;
> > +	int if_id = -1;
> > +	int err;
> > +
> > +	/* prefetch the frame descriptor */
> > +	prefetch(fd);
> 
> Does this actually do any good, given that the next call:
> 
> > +
> > +	/* get switch ingress interface ID */
> > +	if_id = upper_32_bits(dpaa2_fd_get_flc(fd)) & 0x0000FFFF;
> 
> is accessing the frame descriptor? The idea of prefetch is to let it
> bring it into the cache while you are busy doing something else,
> hopefully with something which is already cache hot.
> 

I'll check w and w/o the prefetch but, most probably, it doesn't help.
Thanks.

Ioana