Re: [dpdk-dev] [PATCH v1 2/4] regexdev: add regex core h file

[Ori Kam <orika@mellanox.com>]

> > 
> > From: Jerin Jacob <jerinjacobk@gmail.com> 
> > Sent: Wednesday, April 8, 2020 10:49 AM
> > 
> > 
> > 
> > On Wed, Apr 8, 2020 at 1:07 PM Ori Kam <mailto:orika@mellanox.com> wrote:
> > 
> > 
> > > -----Original Message-----
> > > From: dev <mailto:dev-bounces@dpdk.org> On Behalf Of Jerin Jacob
> > > Sent: Tuesday, April 7, 2020 7:27 PM
> > > Subject: Re: [dpdk-dev] [PATCH v1 2/4] regexdev: add regex core h file
> > > 
> > > On Tue, Apr 7, 2020 at 9:46 PM Ori Kam <mailto:orika@mellanox.com> wrote:
> > > 
> > > > Hi Guy,
> > > >
> > > > > -----Original Message-----
> > > > > From: dev <mailto:dev-bounces@dpdk.org> On Behalf Of Guy Kaneti
> > > > > Sent: Tuesday, April 7, 2020 11:54 AM
> > > > > > +
> > > > > > +/**
> > > > > > + * @internal
> > > > > > + * The generic data structure associated with each RegEx device.
> > > > > > + *
> > > > > > + * Pointers to burst-oriented packet receive and transmit functions
> > > > are
> > > > > > + * located at the beginning of the structure, along with the pointer
> > > > to
> > > > > > + * where all the data elements for the particular device are stored in
> > > > > > +shared
> > > > > > + * memory. This split allows the function pointer and driver data to
> > > > be
> > > > > > +per-
> > > > > > + * process, while the actual configuration data for the device is
> > > > shared.
> > > > > > + */
> > > > > > +struct rte_regexdev {
> > > > > > +   regexdev_enqueue_t enqueue;
> > > > > > +   regexdev_dequeue_t dequeue;
> > > > > > +   const struct rte_regexdev_ops *dev_ops;
> > > > > > +   /**< Functions exported by PMD */
> > > > > > +   struct rte_device *device; /**< Backing device */ }
> > > > > > +__rte_cache_aligned;
> > > > > > +
> > > > >
> > > > > What about a handle for the PMD private data such as
> > > > >       struct rte_eventdev_data *data;
> > > > >       /**< Pointer to device data */
> > > > >
> > > > >       struct rte_cryptodev_data *data;
> > > > >       /**< Pointer to device data */
> > > >
> > > > I was thinking about new approach. To use container of.
> > > > Meaning each PMD will create like normal its priv structure.
> > > > In this structure there will be a regex_dev member.
> > > > For example:
> > > > struct mlx5_regex_priv {
> > > >         struct rte_regex_dev regex_dev;
> > > >
> > > 
> > > The  rte_regex_dev which has  enqueue() and  dequeue() function pointer
> > > should not be NOT allocated from hugepage
> > > as per process it will have different enqueue() and dequeue() function
> > > pointer value. Making it hugepage, another process
> > > overwrites it.
> > > 
> > > 
> > 
> > I didn't say this structure should be allocated from huge page.
> > Unless I'm missing something, from memory this is exactly the same
> > as if we had pointer to the priv.
> > 
> > Private data should be allocated from the hugepage so that multiple processes can access it.
> > Whereas the memory that contains the  enqueue() and dequeue() should not be  from hugepage.
> > So both can not be from the same memory. Right?
> > 
> > 
> > 
> > 
> > > 
> > > >         //private fields
> > > >         ...
> > > >         ...
> > > > }
> > > > On registration the PMD will give the rte_regexdev the reference to the
> > > > regex_dev.
> > > > The PMD will use container_of
> > > >
> > > > This approach  hides the private data from the application,
> > > > saves malloc, a bit faster, and saves the use of references.
> > > >
> 
> 
> > > The  rte_regex_dev which has  enqueue() and  dequeue() function pointer
> > > should not be NOT allocated from hugepage
> > > as per process it will have different enqueue() and dequeue() function
> > > pointer value. Making it hugepage, another process
> > > overwrites it.
> > > 
> > > 
> > 
> > I didn't say this structure should be allocated from huge page.
> > Unless I'm missing something, from memory this is exactly the same
> > as if we had pointer to the priv.
> > 
> > Private data should be allocated from the hugepage so that multiple processes can access it.
> > Whereas the memory that contains the  enqueue() and dequeue() should not be  from hugepage.
> > So both can not be from the same memory. Right?
> > 
> 
> Yes you are right, in current implementation the idea was to support only single process.
> But I will update this code, to make it more like ethdev.
> 
> Thanks for understanding. We would like to avoid rework when we add multi-process.
> Please check the [re]configure the function  and it memory allocation requirement for storing the queue pointers as well
> from ethdev subsystem(in fact, all existing subsystem has same scheme).
> 

Will fix.

> > > > So a better approach 😊 also this approach is in use by the rte_device.
> > > >
> > > >
> > > >
> >