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

[Jakub Kicinski <kuba@kernel.org>]

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
> > > +@_type 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.