Re: [Intel-wired-lan] [PATCH iwl-next v2 08/14] idpf: refactor idpf to use libie controlq and Xn APIs

[Paul Menzel]

Dear Larysa,


Am 25.04.25 um 12:11 schrieb Larysa Zaremba:
> On Thu, Apr 24, 2025 at 05:32:17PM +0200, Paul Menzel wrote:

> > Am 24.04.25 um 13:32 schrieb Larysa Zaremba:
> > > From: Pavan Kumar Linga <pavan.kumar.linga@xxxxxxxxx>
> > >
> > > Support to initialize and configure controlq, Xn manager,
> > > MMIO and reset APIs was introduced in libie. As part of it,
> > > most of the existing controlq structures are renamed and
> > > modified. Use those APIs in idpf and make all the necessary changes.
> > >
> > > Previously for the send and receive virtchnl messages, there
> > > used to be a memcpy involved in controlq code to copy the buffer
> > > info passed by the send function into the controlq specific
> > > buffers. There was no restriction to use automatic memory
> > > in that case. The new implementation in libie removed copying
> > > of the send buffer info and introduced DMA mapping of the
> > > send buffer itself. To accommodate it, use dynamic memory for
> > > the send buffers. In case of receive, idpf receives a page pool
> > > buffer allocated by the libie and care should be taken to
> > > release it after use in the idpf.
> > >
> > > The changes are fairly trivial and localized, with a notable exception
> > > being the consolidation of idpf_vc_xn_shutdown and idpf_deinit_dflt_mbx
> > > under the latter name. This has some additional consequences that are
> > > addressed in the following patches.
> >
> > (You could reflow the text above to have consistent line length.)

Re-flowing would save at least one line.

> > Also, how can your patchset be verified?
>
> Just normal regression testing with kernel debug options enabled, a large
> portion of the touched code is covered by just loading-unloading the driver and
> doing a PCI reset, stuff like PTP needs to be checked separately, because it
> heavily uses control queue itself.
>
> > Does the module size change?
>
> idpf size does decrease, but overall size increases. It was 585728B for idpf,
> now it is 557056 + 16384 + 53248 [B], this amounts to +40KB of storage usage on
> systems that will not use ixd.
>
> After
> *********
> idpf                  557056  0
> ixd                    40960  0
> libie_pci              16384  2 ixd,idpf
> libie_cp               53248  2 ixd,idpf
> libeth                 16384  2 idpf,libie_cp
>
> Before
> *********
> idpf                  585728  0
> libeth                 16384  1 idpf
>
> > Is the resource usage different for certain test cases?
>
> We now pre-allocate small TX buffers, so that does increase the memory usage,
> but reduces the need to allocate. This results in additional 256 * 128B of
> memory permanently used, but our ctlq RX buffers need to be of size 4096B anyway
> (not changed by the patchset), so this is hardly noticable.
>
> The worst-case memory usage should stay almost the same + abovementioned 32KB.
> As for the timings, we are mostly limited by the HW response time, which is far
> from instant.

Thank you for all these details. I’d love to see those in the commit 
message too.

> > > Reviewed-by: Michal Kubiak <michal.kubiak@xxxxxxxxx>
> > > Signed-off-by: Pavan Kumar Linga <pavan.kumar.linga@xxxxxxxxx>
> > > Co-developed-by: Larysa Zaremba <larysa.zaremba@xxxxxxxxx>
> > > Signed-off-by: Larysa Zaremba <larysa.zaremba@xxxxxxxxx>
> > > ---
> > >    drivers/net/ethernet/intel/idpf/Kconfig       |    1 +
> > >    drivers/net/ethernet/intel/idpf/Makefile      |    2 -
> > >    drivers/net/ethernet/intel/idpf/idpf.h        |   42 +-
> > >    .../net/ethernet/intel/idpf/idpf_controlq.c   |  624 -------
> > >    .../net/ethernet/intel/idpf/idpf_controlq.h   |  130 --
> > >    .../ethernet/intel/idpf/idpf_controlq_api.h   |  177 --
> > >    .../ethernet/intel/idpf/idpf_controlq_setup.c |  171 --
> > >    drivers/net/ethernet/intel/idpf/idpf_dev.c    |   91 +-
> > >    drivers/net/ethernet/intel/idpf/idpf_lib.c    |   49 +-
> > >    drivers/net/ethernet/intel/idpf/idpf_main.c   |   87 +-
> > >    drivers/net/ethernet/intel/idpf/idpf_mem.h    |   20 -
> > >    drivers/net/ethernet/intel/idpf/idpf_txrx.h   |    2 +-
> > >    drivers/net/ethernet/intel/idpf/idpf_vf_dev.c |   89 +-
> > >    .../net/ethernet/intel/idpf/idpf_virtchnl.c   | 1622 ++++++-----------
> > >    .../net/ethernet/intel/idpf/idpf_virtchnl.h   |   89 +-
> > >    .../ethernet/intel/idpf/idpf_virtchnl_ptp.c   |  303 ++-
> > >    16 files changed, 886 insertions(+), 2613 deletions(-)
> > >    delete mode 100644 drivers/net/ethernet/intel/idpf/idpf_controlq.c
> > >    delete mode 100644 drivers/net/ethernet/intel/idpf/idpf_controlq.h
> > >    delete mode 100644 drivers/net/ethernet/intel/idpf/idpf_controlq_api.h
> > >    delete mode 100644 drivers/net/ethernet/intel/idpf/idpf_controlq_setup.c
> > >    delete mode 100644 drivers/net/ethernet/intel/idpf/idpf_mem.h
> >
> > […]

Kind regards,

Paul