On Tue, Jan 25, 2022 at 10:32:35AM +0100, Magnus Karlsson wrote:
> On Mon, Jan 24, 2022 at 8:38 PM Maciej Fijalkowski
> <maciej.fijalkowski@xxxxxxxxx> wrote:
> >
> > Apply the logic that was done for regular XDP from commit 9610bd988df9
> > ("ice: optimize XDP_TX workloads") to the ZC side of the driver. On top
> > of that, introduce batching to Tx that is inspired by i40e's
> > implementation with adjustments to the cleaning logic - take into the
> > account NAPI budget in ice_clean_xdp_irq_zc().
> >
> > Separating the stats structs onto separate cache lines seemed to improve
> > the performance.
>
> Nice one, thanks! Just one smaller comment below.
>
> Acked-by: Magnus Karlsson <magnus.karlsson@xxxxxxxxx>
>
> > Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@xxxxxxxxx>
> > ---
> > drivers/net/ethernet/intel/ice/ice_txrx.c | 2 +-
> > drivers/net/ethernet/intel/ice/ice_txrx.h | 2 +-
> > drivers/net/ethernet/intel/ice/ice_xsk.c | 256 ++++++++++++++--------
> > drivers/net/ethernet/intel/ice/ice_xsk.h | 27 ++-
> > 4 files changed, 188 insertions(+), 99 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.c b/drivers/net/ethernet/intel/ice/ice_txrx.c
> > index 73f60493209d..7d8824b4c8ff 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_txrx.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_txrx.c
> > @@ -1462,7 +1462,7 @@ int ice_napi_poll(struct napi_struct *napi, int budget)
> > bool wd;
> >
> > if (tx_ring->xsk_pool)
> > - wd = ice_clean_tx_irq_zc(tx_ring, budget);
> > + wd = ice_xmit_zc(tx_ring, ICE_DESC_UNUSED(tx_ring), budget);
> > else if (ice_ring_is_xdp(tx_ring))
> > wd = true;
> > else
> > diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.h b/drivers/net/ethernet/intel/ice/ice_txrx.h
> > index 611dd7c4a631..ea6c9cc02a1a 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_txrx.h
> > +++ b/drivers/net/ethernet/intel/ice/ice_txrx.h
> > @@ -322,9 +322,9 @@ struct ice_tx_ring {
> > u16 count; /* Number of descriptors */
> > u16 q_index; /* Queue number of ring */
> > /* stats structs */
> > + struct ice_txq_stats tx_stats;
> > struct ice_q_stats stats;
> > struct u64_stats_sync syncp;
> > - struct ice_txq_stats tx_stats;
> >
> > /* CL3 - 3rd cacheline starts here */
> > struct rcu_head rcu; /* to avoid race on free */
>
> Seems like these comments are wrong these days. Your move indeed moves
> the tx_stats to another cache line as seen in the pahole dump below,
> but that is not obvious with the comments that point to the opposite.
> Maybe update the cacheline start comments to the correct locations?
Indeed it's off. It seems there are minor things to improve here and
there, so let me send a v5.
Thanks!
>
> <snip>
> u16 q_index; /* 94 2 */
> struct ice_txq_stats tx_stats; /* 96 32 */
>
> /* XXX last struct has 4 bytes of padding */
>
> /* --- cacheline 2 boundary (128 bytes) --- */
> struct ice_q_stats stats; /* 128 16 */
> struct u64_stats_sync syncp; /* 144 0 */
> struct callback_head rcu __attribute__((__aligned__(8))); /*
> 144 16 */
> long unsigned int xps_state[1]; /* 160 8 */
> <snip>
>
(...)
