Re: [PATCH net-next 05/15] ice: replace custom AIM algorithm with kernel's DIM library

2021-04-15 Thread Jakub Kicinski
On Thu, 15 Apr 2021 17:03:23 + Keller, Jacob E wrote:
> > On Wed, 14 Apr 2021 17:30:03 -0700 Tony Nguyen wrote:  
> > > +static void ice_tx_dim_work(struct work_struct *work)
> > > +{
> > > + struct ice_ring_container *rc;
> > > + struct ice_q_vector *q_vector;
> > > + struct dim *dim;
> > > + u16 itr, intrl;
> > > +
> > > + dim = container_of(work, struct dim, work);
> > > + rc = container_of(dim, struct ice_ring_container, dim);
> > > + q_vector = container_of(rc, struct ice_q_vector, tx);
> > > +
> > > + if (dim->profile_ix >= ARRAY_SIZE(tx_profile))
> > > + dim->profile_ix = ARRAY_SIZE(tx_profile) - 1;
> > > +
> > > + /* look up the values in our local table */
> > > + itr = tx_profile[dim->profile_ix].itr;
> > > + intrl = tx_profile[dim->profile_ix].intrl;
> > > +
> > > + ice_write_itr(rc, itr);
> > > + ice_write_intrl(q_vector, intrl);
> > > +
> > > + dim->state = DIM_START_MEASURE;  
> > 
> > Are you only doing register writes in ice_write_itr/intrl or talk to FW?
> > Scheduler is expensive so you can save real cycles if you don't have to
> > rely on a work to do the programming (not sure how hard that is with
> > DIM, but since you're already sorta poking at the internals I thought
> > I'd ask).  
> 
> Hmm. I believe we only have to do register writes. If I recall, at
> least based on reading the other DIMLIB implementations, they seem to
> have mostly moved to a work item for apparently moving these changes
> out of the hot path.. but maybe that's not really an issue. Ofcourse
> the current dim implementation enforces use of the work queue, so I
> think it would require refactoring the library to support doing
> immediate application as opposed to using the work item..

I think it may be because of FW programming which needs to sleep. The
extra scheduler work because of this async mechanism does impact real
workloads (admittedly not more than 1%), but up to you.


RE: [PATCH net-next 05/15] ice: replace custom AIM algorithm with kernel's DIM library

2021-04-15 Thread Keller, Jacob E
> -Original Message-
> From: Jakub Kicinski 
> Sent: Thursday, April 15, 2021 9:47 AM
> To: Nguyen, Anthony L 
> Cc: da...@davemloft.net; Keller, Jacob E ;
> netdev@vger.kernel.org; sassm...@redhat.com; Brandeburg, Jesse
> ; Brelinski, TonyX 
> Subject: Re: [PATCH net-next 05/15] ice: replace custom AIM algorithm with
> kernel's DIM library
> 
> On Wed, 14 Apr 2021 17:30:03 -0700 Tony Nguyen wrote:
> > +static void ice_tx_dim_work(struct work_struct *work)
> > +{
> > +   struct ice_ring_container *rc;
> > +   struct ice_q_vector *q_vector;
> > +   struct dim *dim;
> > +   u16 itr, intrl;
> > +
> > +   dim = container_of(work, struct dim, work);
> > +   rc = container_of(dim, struct ice_ring_container, dim);
> > +   q_vector = container_of(rc, struct ice_q_vector, tx);
> > +
> > +   if (dim->profile_ix >= ARRAY_SIZE(tx_profile))
> > +   dim->profile_ix = ARRAY_SIZE(tx_profile) - 1;
> > +
> > +   /* look up the values in our local table */
> > +   itr = tx_profile[dim->profile_ix].itr;
> > +   intrl = tx_profile[dim->profile_ix].intrl;
> > +
> > +   ice_write_itr(rc, itr);
> > +   ice_write_intrl(q_vector, intrl);
> > +
> > +   dim->state = DIM_START_MEASURE;
> 
> Are you only doing register writes in ice_write_itr/intrl or talk to FW?
> Scheduler is expensive so you can save real cycles if you don't have to
> rely on a work to do the programming (not sure how hard that is with
> DIM, but since you're already sorta poking at the internals I thought
> I'd ask).

Hmm. I believe we only have to do register writes. If I recall, at least based 
on reading the other DIMLIB implementations, they seem to have mostly moved to 
a work item for apparently moving these changes out of the hot path.. but maybe 
that's not really an issue. Ofcourse the current dim implementation enforces 
use of the work queue, so I think it would require refactoring the library to 
support doing immediate application as opposed to using the work item..


Re: [PATCH net-next 05/15] ice: replace custom AIM algorithm with kernel's DIM library

2021-04-15 Thread Jakub Kicinski
On Wed, 14 Apr 2021 17:30:03 -0700 Tony Nguyen wrote:
> +static void ice_tx_dim_work(struct work_struct *work)
> +{
> + struct ice_ring_container *rc;
> + struct ice_q_vector *q_vector;
> + struct dim *dim;
> + u16 itr, intrl;
> +
> + dim = container_of(work, struct dim, work);
> + rc = container_of(dim, struct ice_ring_container, dim);
> + q_vector = container_of(rc, struct ice_q_vector, tx);
> +
> + if (dim->profile_ix >= ARRAY_SIZE(tx_profile))
> + dim->profile_ix = ARRAY_SIZE(tx_profile) - 1;
> +
> + /* look up the values in our local table */
> + itr = tx_profile[dim->profile_ix].itr;
> + intrl = tx_profile[dim->profile_ix].intrl;
> +
> + ice_write_itr(rc, itr);
> + ice_write_intrl(q_vector, intrl);
> +
> + dim->state = DIM_START_MEASURE;

Are you only doing register writes in ice_write_itr/intrl or talk to FW?
Scheduler is expensive so you can save real cycles if you don't have to
rely on a work to do the programming (not sure how hard that is with
DIM, but since you're already sorta poking at the internals I thought
I'd ask).