netdev - Re: [PATCH bpf-next v4 7/8] ice: xsk: improve AF

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Ye/dqylvNNa72wI8@boxer>
Date:   Tue, 25 Jan 2022 12:23:23 +0100
From:   Maciej Fijalkowski <maciej.fijalkowski@...el.com>
To:     Magnus Karlsson <magnus.karlsson@...il.com>
Cc:     bpf <bpf@...r.kernel.org>, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Network Development <netdev@...r.kernel.org>,
        "Karlsson, Magnus" <magnus.karlsson@...el.com>,
        Alexander Lobakin <alexandr.lobakin@...el.com>
Subject: Re: [PATCH bpf-next v4 7/8] ice: xsk: improve AF_XDP ZC Tx and use
 batching API

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@...el.com> 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@...el.com>
> 
> > Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
> > ---
> >  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>
> 

(...)