Re: [dpdk-dev] [RFC PATCH v1] regexdev: introduce regexdev subsystem

[Shahaf Shuler <shahafs@mellanox.com>]

Friday, September 27, 2019 5:46 PM, Jerin Jacob Kollanukkaran:
> subsystem
> 
> > -----Original Message-----
> > From: Jerin Jacob Kollanukkaran
> > Sent: Tuesday, September 10, 2019 4:33 PM
> > To: Shahaf Shuler <shahafs@mellanox.com>; Thomas Monjalon
> > <thomas@monjalon.net>; dev@dpdk.org
> > Cc: Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com>; Hemant
> > Agrawal <hemant.agrawal@nxp.com>; Opher Reviv
> <opher@mellanox.com>;
> > Alex Rosenbaum <alexr@mellanox.com>; Dovrat Zifroni
> > <dovrat@marvell.com>; Prasun Kapoor <pkapoor@marvell.com>; Nipun
> Gupta
> > <nipun.gupta@nxp.com>; Wang, Xiang W <xiang.w.wang@intel.com>;
> > Richardson, Bruce <bruce.richardson@intel.com>; yang.a.hong@intel.com;
> > harry.chang@intel.com; gu.jian1@zte.com.cn;
> > shanjiangh@chinatelecom.cn; zhangy.yun@chinatelecom.cn;
> > lixingfu@huachentel.com; wushuai@inspur.com; yuyingxia@yxlink.com;
> > fanchenggang@sunyainfo.com; davidfgao@tencent.com;
> > liuzhong1@chinaunicom.cn; zhaoyong11@huawei.com; oc@yunify.com;
> > jim@netgate.com; hongjun.ni@intel.com; j.bromhead@titan-ic.com;
> > deri@ntop.org; fc@napatech.com; arthur.su@lionic.com
> > Subject: RE: [dpdk-dev] [RFC PATCH v1] regexdev: introduce regexdev
> > subsystem
> >
> > > Hi Jerin,
> >
> > Hi Shahaf,
> >
> > > > + *
> > > > + * @see rte_regex_rule_db_import(), rte_regex_rule_db_export()
> > > > +*/ uint16_t rte_regex_rule_db_update(uint8_t dev_id, const struct
> > > > +rte_regex_rule
> > > > *rules,
> > > > +			 uint16_t nb_rules);
> > >
> > > I think the function name is not too informative. If this function
> > > meant to compile the rule then it should be explicit on the function
> name.
> >
> > It is meant to be compile the rules and then  update the rule database.
> >
> > I think, we can have either 1 or 2. Let me know your preference or If
> > you have any name suggestion. I will change it accordingly.
> >
> > 1. rte_regex_rule_db_compile()
> > 2. rte_regex_rule_db_compile_update()
> 
> 
> @Shahaf Shuler, Thoughts?

IMO we should have two separate functions - one to only compile. One to only update. 

So I would prefer #1, with addition (if not already present) of API to update rules. 

> 
> 
> >
> >
> > > > +
> > > > + */
> > > > +struct rte_regex_ops {
> > > > +
> > > > +	/* W4 */
> > > > +	RTE_STD_C11
> > > > +	union {
> > > > +		uint64_t user_id;
> > > > +		/**< Application specific opaque value. An application may
> > > > use
> > > > +		 * this field to hold application specific value to share
> > > > +		 * between dequeue and enqueue operation.
> > > > +		 * Implementation should not modify this field.
> > > > +		 */
> > > > +		void *user_ptr;
> > > > +		/**< Pointer representation of *user_id* */
> > > > +	};
> > >
> > > Since we target the regex subsystem for both regex and DPI I think
> > > it will be good to add another uint64_t field called connection_id.
> > > Device that support DPI can refer to it as another match able field
> > > when looking up for matches on the given buffer.
> > >
> > > This field is different from the user_id, as it is not opaque for the device.
> >
> > Is this driver specific storage place where application should not touch it?
> >
> > If not, Could you share the data flow of this field? Ie. Who "write"
> > this Field and who "read" this field.

Application writes to the field. Device reads from this fields. 
Unlike the user_ptr which is complete opaque to the device, connection_id field will have some meaning (e.g. DPI rules can apply on it). 

> 
> @Shahaf Shuler Thoughts?
> 
> Based on your input, I will update the next version.
> 
> >
> > This is just for documentation, In any event we can add new fields.
> >
> > If it is only for driver usage then I think, some driver may need more
> > 8B Storage. In that case I think, each driver can add its on field
> > After W4(i.e existing user_id) and introduce new field called
> > match_offset in struct rte_regex_ops
> >
> > ie. struct rte_regex_match *matches == ops + ops-> match_offset; so
> > that, Each driver can add enough driver specific metadata.
> >
> >
> >