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.
