netdev - Re: [net-next v3 13/15] iecm: Add ethtool

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: <20200629143114.64fca33e@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Mon, 29 Jun 2020 14:31:14 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     "Brady, Alan" <alan.brady@...el.com>
Cc:     "Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "Michael, Alice" <alice.michael@...el.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "nhorman@...hat.com" <nhorman@...hat.com>,
        "sassmann@...hat.com" <sassmann@...hat.com>,
        "Burra, Phani R" <phani.r.burra@...el.com>,
        "Hay, Joshua A" <joshua.a.hay@...el.com>,
        "Chittim, Madhu" <madhu.chittim@...el.com>,
        "Linga, Pavan Kumar" <pavan.kumar.linga@...el.com>,
        "Skidmore, Donald C" <donald.c.skidmore@...el.com>,
        "Brandeburg, Jesse" <jesse.brandeburg@...el.com>,
        "Samudrala, Sridhar" <sridhar.samudrala@...el.com>
Subject: Re: [net-next v3 13/15] iecm: Add ethtool

On Mon, 29 Jun 2020 21:00:57 +0000 Brady, Alan wrote:
> > > +/* Helper macro to define an iecm_stat structure with proper size and type.
> > > + * Use this when defining constant statistics arrays. Note that
> > > +@...pe expects
> > > + * only a type name and is used multiple times.
> > > + */
> > > +#define IECM_STAT(_type, _name, _stat) { \
> > > +	.stat_string = _name, \
> > > +	.sizeof_stat = sizeof_field(_type, _stat), \
> > > +	.stat_offset = offsetof(_type, _stat) \ }
> > > +
> > > +/* Helper macro for defining some statistics related to queues */
> > > +#define IECM_QUEUE_STAT(_name, _stat) \
> > > +	IECM_STAT(struct iecm_queue, _name, _stat)
> > > +
> > > +/* Stats associated with a Tx queue */ static const struct iecm_stats
> > > +iecm_gstrings_tx_queue_stats[] = {
> > > +	IECM_QUEUE_STAT("%s-%u.packets", q_stats.tx.packets),
> > > +	IECM_QUEUE_STAT("%s-%u.bytes", q_stats.tx.bytes), };
> > > +
> > > +/* Stats associated with an Rx queue */ static const struct
> > > +iecm_stats iecm_gstrings_rx_queue_stats[] = {
> > > +	IECM_QUEUE_STAT("%s-%u.packets", q_stats.rx.packets),
> > > +	IECM_QUEUE_STAT("%s-%u.bytes", q_stats.rx.bytes),
> > > +	IECM_QUEUE_STAT("%s-%u.generic_csum", q_stats.rx.generic_csum),
> > > +	IECM_QUEUE_STAT("%s-%u.basic_csum", q_stats.rx.basic_csum),  
> > 
> > What's basic and generic? perhaps given them the Linux names?  
> 
> I believe these should be hw_csum for basic_csum and csum_valid for generic_csum, will fix.

I was thinking of just saying csum_complete and csum_unnecessary.

But generic_sum doesn't seem to be incremented in this patch, so hard
to tell what it is :S

> > > +	q->itr.target_itr = coalesce_usecs;
> > > +	if (use_adaptive_coalesce)
> > > +		q->itr.target_itr |= IECM_ITR_DYNAMIC;
> > > +	/* Update of static/dynamic ITR will be taken care when interrupt is
> > > +	 * fired
> > > +	 */
> > > +	return 0;
> > > +}
> > > +
> > > +/**
> > > + * iecm_set_q_coalesce - set ITR values for specific queue
> > > + * @vport: vport associated to the queue that need updating
> > > + * @ec: coalesce settings to program the device with
> > > + * @q_num: update ITR/INTRL (coalesce) settings for this queue
> > > +number/index
> > > + * @is_rxq: is queue type Rx
> > > + *
> > > + * Return 0 on success, and negative on failure  */ static int
> > > +iecm_set_q_coalesce(struct iecm_vport *vport, struct ethtool_coalesce *ec,
> > > +		    int q_num, bool is_rxq)
> > > +{
> > > +	if (is_rxq) {
> > > +		struct iecm_queue *rxq = iecm_find_rxq(vport, q_num);
> > > +
> > > +		if (rxq && __iecm_set_q_coalesce(ec, rxq))
> > > +			return -EINVAL;
> > > +	} else {
> > > +		struct iecm_queue *txq = iecm_find_txq(vport, q_num);
> > > +
> > > +		if (txq && __iecm_set_q_coalesce(ec, txq))
> > > +			return -EINVAL;
> > > +	}  
> > 
> > What's the point? Callers always call this function with tx, then rx.
> > Just set both.  
> 
> As I understand it's possible to have a different number of TX and RX
> queues.  Theoretically iecm_find_Xq will just return NULL if there's
> no queue for some index so we could do both, but then we have to
> figure which one is greater etc etc.  It seems less error prone and
> clearer to me to just call it for the queues we need to.  We can make
> this iecm_set_q_coalesce function a little less terse, perhaps that
> is sufficient?

I don't feel strongly about this one, up to you.