Re: [dpdk-dev] [PATCH v2 1/4] lib: introduce IF Proxy library

["Morten Brørup" <mb@smartsharesystems.com>]

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andrzej Ostruszka
> [C]
> Sent: Tuesday, July 7, 2020 10:14 PM
> 
> First of all, please excuse me Stephen for late reply.
> 
> On 02/07/2020 02:34, Stephen Hemminger wrote:
> > I had great hopes for this library, because such code exists in
> > almost every real world application. But the current version falls
> > short.
> 
> Sorry to disappoint.
> 
> > So what this library does is turn a message based protocol (netlink)
> > into a set of callbacks. Not sure if that is all that useful
> processing
> > messages is often easier.
> 
> Callbacks or queued notifications.  I'm not sure I understand why you
> are saying that it would be easier for application to do everything on
> its own when it can now just pass a callback or dequeue notification
> with all essential information extracted.  Could you elaborate here?
> 
> > It would be more useful if the library took the netlink and
> maintained
> > a set of tables (interfaces, neighbours, routes) using DPDK
> infrastructure
> > and did it as efficiently as possible with RCU.
> 
> I'm pretty sure it would, but that's a bit like complaining about new
> toy that it doesn't have batteries included.  And BTW "no batteries"
> was
> a conscientious decision.  I wanted something basic that would ease
> sniffing of the network configuration and IMHO it does that.
> 
> I don't have access to environment where heavy netlink processing would
> be required.  And apart from that I can imagine that every application
> has its own particular needs so it will be difficult to please
> everybody.  So instead I thought it would be better to come up with a
> starting point upon which this thing can grow - with input from the
> community/users.
> 

Please pay close attention to Stephen's feedback.

It may be harsh, but I think you should go back to scratch and take a top-down approach to the library design.

The design process should start with an example application, emulating a router running BGP or similar.

Such an application would reveal what an IF proxy library should provide, and how it should be provided.

And then Thomas' sidetrack could come into play... A generic library for the best common practice for interaction between the data plane and the control plane... The DPDK documentation doesn't say anything about how to handle this interaction. From Stephen's comment below, I guess that the interrupt thread is not the best way to go.

> > On a real world router with 1M routes, the processing of netlink can
> be
> > a significant chore. It has to be done efficiently. Probably needs to
> use
> > rte_ctrl_thread() and not overload the interrupt thread.
> 
> That is another voice against using interrupt thread.  I acknowledge
> your (plural) concerns and will think about changing it.  Thanks.  I'm
> not sure if I manage to make it before .08 though.

What is the use of getting it into .08 if it needs serious rewriting and a new API anyway?

Following all this negative feedback, I would like to provide some positive feedback too: Take note that we have high hopes for the IP proxy library, and we do recognize that it will be very useful for many applications (if done right). I think it is a great idea, and I appreciate your hard work on it!

> 
> > Also, please don't reinvent netlink parsing. Use a standard library
> > like libmnl.
> 
> I actually checked this and decided that this will not buy me much and
> will add an additional dependency.  I also checked how other DPDK parts
> (there are couple) and iproute2 utils are handling netlink and decided
> to follow their style of manual handling.  Could you give a hint here
> how hard you want to push for libmnl?  As I've mentioned, that was on
> my
> list of options, so if community does not mind adding libmnl dependency
> and would prefer using it instead of manual parsing then I might adapt
> to that.
> 
> > Please use doxygen format for API documentation
> >
> >> +/* This function should be called by the implementation whenever it
> notices
> >> + * change in the network configuration.  The arguments are:
> >> + * - ev : pointer to filled event data structure (all fields are
> expected to be
> >> + *     filled, with the exception of 'port_id' for all proxy/port
> related
> >> + *     events: this function clones the event notification for each
> bound port
> >> + *     and fills 'port_id' appropriately).
> >> + * - px : proxy node when given event is proxy/port related,
> otherwise pass NULL
> >> + */
> >> +void ifpx_notify_event(struct rte_ifpx_event *ev, struct
> ifpx_proxy_node *px);
> >> +
> 
> Will do - thank you.
> 
> Thank you for taking time to look at this.
> 
> With regards
> Andrzej Ostruszka