| Message ID | 20210415003013.19717-1-anthony.l.nguyen@intel.com |
|---|---|
| Headers | show |
| Series | 100GbE Intel Wired LAN Driver Updates 2021-04-14 | expand |
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).
> -----Original Message----- > From: Jakub Kicinski <kuba@kernel.org> > Sent: Thursday, April 15, 2021 9:47 AM > To: Nguyen, Anthony L <anthony.l.nguyen@intel.com> > Cc: davem@davemloft.net; Keller, Jacob E <jacob.e.keller@intel.com>; > netdev@vger.kernel.org; sassmann@redhat.com; Brandeburg, Jesse > <jesse.brandeburg@intel.com>; Brelinski, TonyX <tonyx.brelinski@intel.com> > 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..
On Thu, 15 Apr 2021 17:03:23 +0000 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.