Re: [dpdk-dev] [PATCH 5/6] net/mlx5: support PCI device DMA map and unmap

[Shahaf Shuler <shahafs@mellanox.com>]

Thursday, February 14, 2019 12:22 PM, Gaëtan Rivet:
> Subject: Re: [PATCH 5/6] net/mlx5: support PCI device DMA map and unmap
> 
> On Wed, Feb 13, 2019 at 07:11:35PM +0000, Shahaf Shuler wrote:
> > Wednesday, February 13, 2019 1:44 PM, Gaëtan Rivet:
> > > Subject: Re: [PATCH 5/6] net/mlx5: support PCI device DMA map and
> > > unmap
> > >
> > > On Wed, Feb 13, 2019 at 12:35:04PM +0100, Gaëtan Rivet wrote:
> > > > On Wed, Feb 13, 2019 at 11:10:25AM +0200, Shahaf Shuler wrote:
> > > > > The implementation reuses the external memory registration work
> > > > > done
> >
> > [..]
> >
> > > > > +
> > > > > +	/**
> > > > > +	 *  We really need to iterate all eth devices regardless of
> > > > > +	 *  their owner.
> > > > > +	 */
> > > > > +	while (port_id < RTE_MAX_ETHPORTS) {
> > > > > +		port_id = rte_eth_find_next(port_id);
> > > > > +		if (port_id >= RTE_MAX_ETHPORTS)
> > > > > +			break;
> > > > > +		dev = &rte_eth_devices[port_id];
> > > > > +		drv_name = dev->device->driver->name;
> > > > > +		if (!strncmp(drv_name, MLX5_DRIVER_NAME,
> > > > > +			     sizeof(MLX5_DRIVER_NAME) + 1) &&
> > > > > +		    pdev == RTE_DEV_TO_PCI(dev->device)) {
> > > > > +			/* found the PCI device. */
> > > > > +			return dev;
> > > > > +		}
> > > > > +	}
> > > > > +	return NULL;
> > > > > +}
> > > >
> > > > Might I interest you in the new API?
> >
> > Good suggestion, will have a look on it in depth.
> >
> > > >
> > > >    {
> > > >            struct rte_dev_iterator it;
> > > >            struct rte_device *dev;
> > > >
> > > >            RTE_DEV_FOREACH(dev, "class=eth", &it)
> > > >                    if (dev == &pdev->device)
> > > >                            return it.class_device;
> > > >            return NULL;
> > > >    }
> > > >
> > >
> > > On that note, this could be in the PCI bus instead?

Looking in more depth into it. it looks like ethdev is the only device class which register its type.
So putting a generic iterator for every class on the PCI bus looks an overkill to me at this point. 

I think I will take the above suggestion to replace the internal PMD code. 

> >
> > We can put it on the PCI bus, but it would mean the PCI bus will not be
> device agnostic.
> > Currently, I couldn't find any reference to eth_dev on the PCI bus, besides
> a single macro which convert to pci device that doesn't really do type checks.
> >
> > Having it in, would mean the PCI will need start to distinguish between
> ethdev, crypto dev and what ever devices exists on its bus.
> >
> 
> I think it's worth thinking about it.
> It can stay class-agnostic:
> 
>     void *
>     rte_pci_device_class(struct rte_pci_device *pdev, const char *class)
>     {
>             char devstr[15+strlen(class)];
>             struct rte_dev_iterator it;
>             struct rte_device *dev;
> 
>             snprintf(devstr, sizeof(devstr), "bus=pci/class=%s", class);
>             RTE_DEV_FOREACH(dev, devstr, &it)
>                     if (dev == &pdev->device)
>                             return it.class_device;
>             return NULL;
>     }
> 
> (not a fan of the stack VLA but whatever.)
> then:
> 
>     eth_dev = rte_pci_device_class(pdev, "eth");
> 
> Whichever type of device could be returned. Only limit is that you have to
> know beforehand what is the device type of the PCI device you are querying
> about, but that's necessary anyway.
> 
> And if it was instead a crypto dev, it would return NULL.
> 
> --
> Gaëtan Rivet
> 6WIND