Re: [PATCH v2 net-next 3/5] ionic: use per-queue xdp_prog

[Brett Creeley <bcreeley@amd.com>]

On 8/27/2024 4:57 AM, Larysa Zaremba wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> On Tue, Aug 27, 2024 at 01:44:10PM +0200, Larysa Zaremba wrote:
>> On Mon, Aug 26, 2024 at 11:44:20AM -0700, Brett Creeley wrote:
>>> From: Shannon Nelson <shannon.nelson@amd.com>
>>>
>>> We originally were using a per-interface xdp_prog variable to track
>>> a loaded XDP program since we knew there would never be support for a
>>> per-queue XDP program.  With that, we only built the per queue rxq_info
>>> struct when an XDP program was loaded and removed it on XDP program unload,
>>> and used the pointer as an indicator in the Rx hotpath to know to how build
>>> the buffers.  However, that's really not the model generally used, and
>>> makes a conversion to page_pool Rx buffer cacheing a little problematic.
>>>
>>> This patch converts the driver to use the more common approach of using
>>> a per-queue xdp_prog pointer to work out buffer allocations and need
>>> for bpf_prog_run_xdp().  We jostle a couple of fields in the queue struct
>>> in order to keep the new xdp_prog pointer in a warm cacheline.
>>>
>>> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
>>> Signed-off-by: Brett Creeley <brett.creeley@amd.com>
>>
>> Reviewed-by: Larysa Zaremba <larysa.zaremba@intel.com>
>>
> 
> I would like to rewoke the tag, see below why.
> 
>> If you happen to send another version, please include in a commit message a note
>> about READ_ONCE() removal. The removal itself is OK, but an indication that this
>> was intentional would be nice.
>>
>>> ---
>>>   .../net/ethernet/pensando/ionic/ionic_dev.h   |  7 +++++--
>>>   .../net/ethernet/pensando/ionic/ionic_lif.c   | 14 +++++++------
>>>   .../net/ethernet/pensando/ionic/ionic_txrx.c  | 21 +++++++++----------
>>>   3 files changed, 23 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_dev.h b/drivers/net/ethernet/pensando/ionic/ionic_dev.h
>>> index c647033f3ad2..19ae68a86a0b 100644
>>> --- a/drivers/net/ethernet/pensando/ionic/ionic_dev.h
>>> +++ b/drivers/net/ethernet/pensando/ionic/ionic_dev.h
>>> @@ -238,9 +238,8 @@ struct ionic_queue {
>>>      unsigned int index;
>>>      unsigned int num_descs;
>>>      unsigned int max_sg_elems;
>>> +
>>>      u64 features;
>>> -   unsigned int type;
>>> -   unsigned int hw_index;
>>>      unsigned int hw_type;
>>>      bool xdp_flush;
>>>      union {
>>> @@ -261,7 +260,11 @@ struct ionic_queue {
>>>              struct ionic_rxq_sg_desc *rxq_sgl;
>>>      };
>>>      struct xdp_rxq_info *xdp_rxq_info;
>>> +   struct bpf_prog *xdp_prog;
>>>      struct ionic_queue *partner;
>>> +
>>> +   unsigned int type;
>>> +   unsigned int hw_index;
>>>      dma_addr_t base_pa;
>>>      dma_addr_t cmb_base_pa;
>>>      dma_addr_t sg_base_pa;
>>> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
>>> index aa0cc31dfe6e..0fba2df33915 100644
>>> --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
>>> +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
>>> @@ -2700,24 +2700,24 @@ static int ionic_xdp_register_rxq_info(struct ionic_queue *q, unsigned int napi_
>>>
>>>   static int ionic_xdp_queues_config(struct ionic_lif *lif)
>>>   {
>>> +   struct bpf_prog *xdp_prog;
>>>      unsigned int i;
>>>      int err;
>>>
>>>      if (!lif->rxqcqs)
>>>              return 0;
>>>
>>> -   /* There's no need to rework memory if not going to/from NULL program.
>>> -    * If there is no lif->xdp_prog, there should also be no q.xdp_rxq_info
>>> -    * This way we don't need to keep an *xdp_prog in every queue struct.
>>> -    */
>>> -   if (!lif->xdp_prog == !lif->rxqcqs[0]->q.xdp_rxq_info)
>>> +   /* There's no need to rework memory if not going to/from NULL program.  */
>>> +   xdp_prog = READ_ONCE(lif->xdp_prog);
>>> +   if (!xdp_prog == !lif->rxqcqs[0]->q.xdp_prog)
>>>              return 0;
> 
> In a case when we replace a non-NULL program with another non-NULL program this
> would create a situation where lif and queues have different pointers on them.

Yeah, you are right. Good catch. We will get this fixed up in the next 
version.

Thanks,

Brett

> 
>>>
>>>      for (i = 0; i < lif->ionic->nrxqs_per_lif && lif->rxqcqs[i]; i++) {
>>>              struct ionic_queue *q = &lif->rxqcqs[i]->q;
>>>
>>> -           if (q->xdp_rxq_info) {
>>> +           if (q->xdp_prog) {
>>>                      ionic_xdp_unregister_rxq_info(q);
>>> +                   q->xdp_prog = NULL;
>>>                      continue;
>>>              }
>>>
>>> @@ -2727,6 +2727,7 @@ static int ionic_xdp_queues_config(struct ionic_lif *lif)
>>>                              i, err);
>>>                      goto err_out;
>>>              }
>>> +           q->xdp_prog = xdp_prog;
>>>      }
>>>
>>>      return 0;
> 
> [...]