Re: [dpdk-dev] [PATCH v4 1/4] ethdev: rename memzones allocated for DMA

[Andrew Rybchenko <arybchenko@solarflare.com>]

On October 12, 2018 20:21:32 Thomas Monjalon <thomas@monjalon.net> wrote:

> 12/10/2018 19:18, Thomas Monjalon:
>> 12/10/2018 18:46, Andrew Rybchenko:
>> > On 10/12/18 7:42 PM, Andrew Rybchenko wrote:
>> > > On 10/12/18 7:40 PM, Thomas Monjalon wrote:
>> > >> 12/10/2018 09:53, Andrew Rybchenko:
>> > >>> On 10/12/18 12:02 AM, Thomas Monjalon wrote:
>> > >>>> The helper rte_eth_dma_zone_reserve() is called by PMDs
>> > >>>> when probing a new port.
>> > >>>> It creates a new memzone with an unique name.
>> > >>>> The name of this memzone was using the name of the driver
>> > >>>> doing the probe.
>> > >>>>
>> > >>>> In order to avoid assigning the driver before the end of the probing
>> > >>>> (next patch), the driver name is removed from these memzone names.
>> > >>>> The ethdev name (data->name) is not used because it may be too long
>> > >>>> and may be not set at this stage of probing.
>> > >>>>
>> > >>>> Syntax of old name: <driver>_<ring>_<port>_<queue>
>> > >>>> Syntax of new name: eth_p<port>_q<queue>_<ring>
>> > >>>>
>> > >>>> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
>> > >>>> ---
>> > >>>>    lib/librte_ethdev/rte_ethdev.c | 5 ++---
>> > >>>>    1 file changed, 2 insertions(+), 3 deletions(-)
>> > >>>>
>> > >>>> diff --git a/lib/librte_ethdev/rte_ethdev.c
>> > >>>> b/lib/librte_ethdev/rte_ethdev.c
>> > >>>> index ef99f7068..ec443def5 100644
>> > >>>> --- a/lib/librte_ethdev/rte_ethdev.c
>> > >>>> +++ b/lib/librte_ethdev/rte_ethdev.c
>> > >>>> @@ -3441,9 +3441,8 @@ rte_eth_dma_zone_reserve(const struct
>> > >>>> rte_eth_dev *dev, const char *ring_name,
>> > >>>>        char z_name[RTE_MEMZONE_NAMESIZE];
>> > >>>>        const struct rte_memzone *mz;
>> > >>>>    -    snprintf(z_name, sizeof(z_name), "%s_%s_%d_%d",
>> > >>>> -         dev->device->driver->name, ring_name,
>> > >>>> -         dev->data->port_id, queue_id);
>> > >>>> +    snprintf(z_name, sizeof(z_name), "eth_p%d_q%d_%s",
>> > >>>> +         dev->data->port_id, queue_id, ring_name);
>> > >>>>           mz = rte_memzone_lookup(z_name);
>> > >>>>        if (mz)
>> > >>> LGTM, but I've found more places where the pattern is duplicate
>> > >>> and testpmd frightens me:
>> > >>>    - app/test-pmd/config.c ring_dma_zone_lookup() which is used
>> > >>>      to look at descriptors (looks like Intel specific since has
>> > >>>      RTE_LIBRTE_I40E_16BYTE_RX_DESC conditional code)
>> > >> >From what I see there is no access to rte_device.driver here,
>> > >> except one in exit function.
>> > >
>> > > Yes, but testpmd will fail to find the memzone and command to
>> > > take a look at descriptors will be broken.
>> > >
>> > > May be it is already broken etc. I think someone from Intel should
>> > > comment it.
>> >
>> > Potentially the following drivers may use it:
>> > $ git grep -w \"rx_ring\" drivers/net/
>> > drivers/net/avf/avf_rxtx.c:380: mz = rte_eth_dma_zone_reserve(dev,
>> > "rx_ring", queue_idx,
>> > drivers/net/axgbe/axgbe_rxtx.c:91:      dma =
>> > rte_eth_dma_zone_reserve(dev, "rx_ring", queue_idx, size, 12
>> > drivers/net/cxgbe/sge.c:1878:            fwevtq ? "fwq_ring" : "rx_ring",
>> > drivers/net/e1000/em_rxtx.c:1437:       rz =
>> > rte_eth_dma_zone_reserve(dev, "rx_ring", queue_idx, rsize,
>> > drivers/net/e1000/igb_rxtx.c:1734:      rz =
>> > rte_eth_dma_zone_reserve(dev, "rx_ring", queue_idx, size,
>> > drivers/net/fm10k/fm10k_ethdev.c:1878:  mz =
>> > rte_eth_dma_zone_reserve(dev, "rx_ring", queue_id,
>> > drivers/net/i40e/i40e_rxtx.c:1853:      rz =
>> > rte_eth_dma_zone_reserve(dev, "rx_ring", queue_idx,
>> > drivers/net/ixgbe/ixgbe_rxtx.c:2966:    rz =
>> > rte_eth_dma_zone_reserve(dev, "rx_ring", queue_idx,
>> > drivers/net/nfp/nfp_net.c:1516: tz = rte_eth_dma_zone_reserve(dev,
>> > "rx_ring", queue_idx,
>>
>> Excuse me, I really don't understand what you mean.
>
> Oh, got it!
> You mean the call to rte_memzone_lookup() must expect the new memzone name.
> So I must update it here too, right?
Yes