RE: [net-next,v12,2/7] r8169: refactor RX path to prepare for multi-queue

[Javen]

>Critical: 0 · High: 0 · Medium: 3 · Low: 0
>
>- [Medium] Undisclosed functional change folded into a patch whose
>  changelog promises a pure refactor: `R8169_RX_RING_BYTES` is changed…
>- [Medium] State introduced by this patch that is never consumed, and a
>  netdev queue count advertised to the core that never matches reality.
>- [Medium] IRQF_NO_THREAD is silently dropped for MSI/MSI-X vectors.
>
>--
>
>r8169: refactor RX path to prepare for multi-queue
>
>This patch introduces struct rtl8169_rx_ring and moves the RX ring state out of
>rtl8169_private into a per-queue array, and switches the netdev allocation to
>devm_alloc_etherdev_mqs() with up to 8 RX queues while num_rx_rings stays
>at 1.
>
>> diff --git a/drivers/net/ethernet/realtek/r8169_main.c
>> b/drivers/net/ethernet/realtek/r8169_main.c
>> index 87eb10616a0c..9311a0cab4eb 100644
>> --- a/drivers/net/ethernet/realtek/r8169_main.c
>> +++ b/drivers/net/ethernet/realtek/r8169_main.c
>> @@ -74,9 +74,19 @@
>>  #define NUM_TX_DESC  256     /* Number of Tx descriptor registers */
>>  #define NUM_RX_DESC  256     /* Number of Rx descriptor registers */
>>  #define R8169_TX_RING_BYTES  (NUM_TX_DESC * sizeof(struct TxDesc))
>> -#define R8169_RX_RING_BYTES  (NUM_RX_DESC * sizeof(struct RxDesc))
>> +
>> +/*
>> + * Workaround for the hardware DMA prefetcher. The H/W might
>> +aggressively
>> + * fetch one more descriptor even after hitting the RingEnd mark. We
>> + * allocate this extra dummy space as padding to prevent
>> +out-of-bounds
>> + * access and potential IOMMU faults.
>> + */
>> +#define R8169_RX_RING_BYTES  ((NUM_RX_DESC + 1) * sizeof(struct
>> +RxDesc))
>
>[Severity: Medium]
>The changelog describes this patch as "a preparatory refactoring of the RX
>path", but this macro changes behaviour for every supported chip.  Two
>questions about it.
>
>First, if the hardware really does "fetch one more descriptor even after hitting
>the RingEnd mark", then the driver before this patch lets the device DMA past
>the end of the ring on all chip generations.  Would that make this an
>independent fix that wants its own patch with a Fixes: tag, so that stable trees
>pick it up?  As submitted the change is invisible to backporters.
>
>Second, sizeof(struct RxDesc) is 16 and NUM_RX_DESC is 256, so the old
>request was exactly 4096 bytes and the new one is 4112 bytes:
>
>        rtl8169_alloc_rx_desc()
>          dma_alloc_coherent(&pdev->dev, R8169_RX_RING_BYTES, ...)
>
>Since dma_alloc_coherent() rounds up to a power-of-two page order, does this
>turn the per-ring coherent allocation from one page into two, doubling the
>coherent DMA footprint per RX ring (and eight times that once the rest of the
>series enables multiple rings)?  Would allocating one page and reserving the
>last descriptor as the pad, or documenting the extra page, be preferable?
>
>>  #define R8169_TX_STOP_THRS   (MAX_SKB_FRAGS + 1)
>>  #define R8169_TX_START_THRS  (2 * R8169_TX_STOP_THRS)
>> +#define R8169_MAX_RX_QUEUES  8
>> +#define R8169_DEFAULT_RX_QUEUES      1
>> +#define R8169_MAX_TX_QUEUES  1
>>
>>  #define OCP_STD_PHY_BASE     0xa400
>>
>
>[ ... ]
>
>> @@ -728,6 +739,15 @@ enum rtl_dash_type {
>>       RTL_DASH_25_BP,
>>  };
>>
>> +struct rtl8169_rx_ring {
>> +     u32 cur_rx;
>> +     u32 dirty_rx;
>
>[Severity: Medium]
>Is dirty_rx used anywhere?  Grepping the tree at the end of this series returns
>only this declaration and the single store in
>rtl8169_init_ring_indexes() below - there is no comparison against cur_rx and
>no refill or doorbell logic reading it.  Should the field be dropped until the
>patch that actually needs it?
>
>> +     struct RxDesc *rx_desc_array;
>> +     dma_addr_t rx_desc_phy_addr[NUM_RX_DESC];
>> +     dma_addr_t rx_phy_addr;
>> +     struct page *rx_databuff[NUM_RX_DESC]; };
>> +
>
>[ ... ]
>
>> @@ -2620,9 +2638,26 @@ static void rtl_init_rxcfg(struct rtl8169_private
>*tp)
>>       }
>>  }
>>
>> +static void rtl8169_rx_desc_init(struct rtl8169_private *tp) {
>> +     for (int i = 0; i < tp->num_rx_rings; i++) {
>> +             struct rtl8169_rx_ring *ring = &tp->rx_ring[i];
>> +
>> +             memset(ring->rx_desc_array, 0x0, R8169_RX_RING_BYTES);
>> +     }
>> +}
>> +
>>  static void rtl8169_init_ring_indexes(struct rtl8169_private *tp)  {
>> -     tp->dirty_tx = tp->cur_tx = tp->cur_rx = 0;
>> +     tp->dirty_tx = 0;
>> +     tp->cur_tx = 0;
>> +
>> +     for (int i = 0; i < tp->num_rx_rings; i++) {
>> +             struct rtl8169_rx_ring *ring = &tp->rx_ring[i];
>> +
>> +             ring->dirty_rx = 0;
>> +             ring->cur_rx = 0;
>> +     }
>>  }
>>
>
>[ ... ]
>
>> @@ -4184,55 +4240,107 @@ static void rtl8169_rx_clear(struct
>> rtl8169_private *tp)
>
>[ ... ]
>
>> +static int rtl8169_alloc_rx_desc(struct rtl8169_private *tp) {
>> +     struct pci_dev *pdev = tp->pci_dev;
>> +     struct rtl8169_rx_ring *ring;
>> +
>> +     for (int i = 0; i < tp->num_rx_rings; i++) {
>> +             ring = &tp->rx_ring[i];
>> +             ring->rx_desc_array = dma_alloc_coherent(&pdev->dev,
>> +                                                      R8169_RX_RING_BYTES,
>> +                                                      &ring->rx_phy_addr,
>> +                                                      GFP_KERNEL);
>> +             if (!ring->rx_desc_array)
>> +                     return -ENOMEM;
>> +     }
>> +     return 0;
>> +}
>> +
>
>[ ... ]
>
>> @@ -5688,7 +5812,10 @@ static int rtl_init_one(struct pci_dev *pdev, const
>struct pci_device_id *ent)
>>       u32 txconfig;
>>       u32 xid;
>>
>> -     dev = devm_alloc_etherdev(&pdev->dev, sizeof (*tp));
>> +     dev = devm_alloc_etherdev_mqs(&pdev->dev, sizeof(*tp),
>> +                                   R8169_MAX_TX_QUEUES,
>> +                                   R8169_MAX_RX_QUEUES);
>> +
>
>[Severity: Medium]
>This asks the core for 8 RX queues on every chip, and alloc_netdev_mqs() sets
>dev->real_num_rx_queues = rxqs:
>
>net/core/dev.c:alloc_netdev_mqs() {
>        ...
>        dev->real_num_rx_queues = rxqs;
>        ...
>}
>
>rtl_setup_rx_params() keeps tp->num_rx_rings at 1, and there is no
>netif_set_real_num_rx_queues() call in the driver, either here or at the end of
>the series.  Does that leave every r8169 device advertising 8 RX queues to
>userspace - /sys/class/net/<dev>/queues/rx-0 through rx-7 with their RPS/RFS
>knobs, plus the netdev-genl queue enumeration - while only ring 0 ever
>receives packets?
>
>Should rtl_init_one() call netif_set_real_num_rx_queues(dev,
>tp->num_rx_rings) once the ring count is known?

This is incorrect. The driver actually DOES call netif_set_real_num_queues()