Re: [PATCH bpf-next v6 11/18] ice: put XDP meta sources assignment under a static key condition

[Larysa Zaremba <larysa.zaremba@intel.com>]

On Fri, Oct 20, 2023 at 06:32:13PM +0200, Maciej Fijalkowski wrote:
> On Thu, Oct 12, 2023 at 07:05:17PM +0200, Larysa Zaremba wrote:
> > Usage of XDP hints requires putting additional information after the
> > xdp_buff. In basic case, only the descriptor has to be copied on a
> > per-packet basis, because xdp_buff permanently resides before per-ring
> > metadata (cached time and VLAN protocol ID).
> > 
> > However, in ZC mode, xdp_buffs come from a pool, so memory after such
> > buffer does not contain any reliable information, so everything has to be
> > copied, damaging the performance.
> > 
> > Introduce a static key to enable meta sources assignment only when attached
> > XDP program is device-bound.
> > 
> > This patch eliminates a 6% performance drop in ZC mode, which was a result
> > of addition of XDP hints to the driver.
> > 
> > Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com>
> > ---
> >  drivers/net/ethernet/intel/ice/ice.h      |  1 +
> >  drivers/net/ethernet/intel/ice/ice_main.c | 14 ++++++++++++++
> >  drivers/net/ethernet/intel/ice/ice_txrx.c |  3 ++-
> >  drivers/net/ethernet/intel/ice/ice_xsk.c  |  3 +++
> >  4 files changed, 20 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
> > index 3d0f15f8b2b8..76d22be878a4 100644
> > --- a/drivers/net/ethernet/intel/ice/ice.h
> > +++ b/drivers/net/ethernet/intel/ice/ice.h
> > @@ -210,6 +210,7 @@ enum ice_feature {
> >  };
> >  
> >  DECLARE_STATIC_KEY_FALSE(ice_xdp_locking_key);
> > +DECLARE_STATIC_KEY_FALSE(ice_xdp_meta_key);
> >  
> >  struct ice_channel {
> >  	struct list_head list;
> > diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> > index 47e8920e1727..ee0df86d34b7 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_main.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> > @@ -48,6 +48,9 @@ MODULE_PARM_DESC(debug, "netif level (0=none,...,16=all)");
> >  DEFINE_STATIC_KEY_FALSE(ice_xdp_locking_key);
> >  EXPORT_SYMBOL(ice_xdp_locking_key);
> >  
> > +DEFINE_STATIC_KEY_FALSE(ice_xdp_meta_key);
> > +EXPORT_SYMBOL(ice_xdp_meta_key);
> > +
> >  /**
> >   * ice_hw_to_dev - Get device pointer from the hardware structure
> >   * @hw: pointer to the device HW structure
> > @@ -2634,6 +2637,11 @@ static int ice_xdp_alloc_setup_rings(struct ice_vsi *vsi)
> >  	return -ENOMEM;
> >  }
> >  
> > +static bool ice_xdp_prog_has_meta(struct bpf_prog *prog)
> > +{
> > +	return prog && prog->aux->dev_bound;
> > +}
> > +
> >  /**
> >   * ice_vsi_assign_bpf_prog - set or clear bpf prog pointer on VSI
> >   * @vsi: VSI to set the bpf prog on
> > @@ -2644,10 +2652,16 @@ static void ice_vsi_assign_bpf_prog(struct ice_vsi *vsi, struct bpf_prog *prog)
> >  	struct bpf_prog *old_prog;
> >  	int i;
> >  
> > +	if (ice_xdp_prog_has_meta(prog))
> > +		static_branch_inc(&ice_xdp_meta_key);
> 
> i thought boolean key would be enough but inc/dec should serve properly
> for example prog hotswap cases.
>

My thought process on using counting instead of boolean was: there can be 
several PFs that use the same driver, so therefore we need to keep track of how 
many od them use hints. And yes, this also looks better for hot-swapping, 
because conditions become more straightforward (we do not need to compare old 
and new programs).

> > +
> >  	old_prog = xchg(&vsi->xdp_prog, prog);
> >  	ice_for_each_rxq(vsi, i)
> >  		WRITE_ONCE(vsi->rx_rings[i]->xdp_prog, vsi->xdp_prog);
> >  
> > +	if (ice_xdp_prog_has_meta(old_prog))
> > +		static_branch_dec(&ice_xdp_meta_key);
> > +
> >  	if (old_prog)
> >  		bpf_prog_put(old_prog);
> >  }
> > diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.c b/drivers/net/ethernet/intel/ice/ice_txrx.c
> > index 4fd7614f243d..19fc182d1f4c 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_txrx.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_txrx.c
> > @@ -572,7 +572,8 @@ ice_run_xdp(struct ice_rx_ring *rx_ring, struct xdp_buff *xdp,
> >  	if (!xdp_prog)
> >  		goto exit;
> >  
> > -	ice_xdp_meta_set_desc(xdp, eop_desc);
> > +	if (static_branch_unlikely(&ice_xdp_meta_key))
> 
> My only concern is that we might be hurting in a minor way hints path now,
> no?

I have thought "unlikely" refers to the default state the code is compiled with 
and after static key incrementation this should be patched to "likely". Isn't 
this how static keys work?

> 
> > +		ice_xdp_meta_set_desc(xdp, eop_desc);
> >  
> >  	act = bpf_prog_run_xdp(xdp_prog, xdp);
> >  	switch (act) {
> > diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.c b/drivers/net/ethernet/intel/ice/ice_xsk.c
> > index 39775bb6cec1..f92d7d33fde6 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_xsk.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_xsk.c
> > @@ -773,6 +773,9 @@ static void ice_prepare_pkt_ctx_zc(struct xdp_buff *xdp,
> >  				   union ice_32b_rx_flex_desc *eop_desc,
> >  				   struct ice_rx_ring *rx_ring)
> >  {
> > +	if (!static_branch_unlikely(&ice_xdp_meta_key))
> > +		return;
> 
> wouldn't it be better to pull it out and avoid calling
> ice_prepare_pkt_ctx_zc() unnecessarily?
> 
> > +
> >  	XSK_CHECK_PRIV_TYPE(struct ice_xdp_buff);
> >  	((struct ice_xdp_buff *)xdp)->pkt_ctx = rx_ring->pkt_ctx;
> >  	ice_xdp_meta_set_desc(xdp, eop_desc);
> > -- 
> > 2.41.0
> >