DPDK patches and discussions
 help / color / mirror / Atom feed
From: Morten Brørup <mb@smartsharesystems.com>
To: "Andrzej Ostruszka [C]" <aostruszka@marvell.com>,
	"Stephen Hemminger" <stephen@networkplumber.org>
Cc: <dev@dpdk.org>, "Thomas Monjalon" <thomas@monjalon.net>
Subject: Re: [dpdk-dev] [PATCH v2 1/4] lib: introduce IF Proxy library
Date: Wed, 8 Jul 2020 18:07:51 +0200
Message-ID: <98CBD80474FA8B44BF855DF32C47DC35C6110C@smartserver.smartshare.dk> (raw)
In-Reply-To: <78526a41-7db0-4eb6-266d-f024b0118593@marvell.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

  reply	other threads:[~2020-07-08 16:07 UTC|newest]

Thread overview: 64+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-06 16:41 [dpdk-dev] [PATCH 0/4] Introduce IF proxy library Andrzej Ostruszka
2020-03-06 16:41 ` [dpdk-dev] [PATCH 1/4] lib: introduce IF Proxy library Andrzej Ostruszka
2020-03-31 12:36   ` Harman Kalra
2020-03-31 15:37     ` Andrzej Ostruszka [C]
2020-04-01  5:29   ` Varghese, Vipin
2020-04-01 20:08     ` Andrzej Ostruszka [C]
2020-04-08  3:04       ` Varghese, Vipin
2020-04-08 18:13         ` Andrzej Ostruszka [C]
2020-03-06 16:41 ` [dpdk-dev] [PATCH 2/4] if_proxy: add library documentation Andrzej Ostruszka
2020-03-06 16:41 ` [dpdk-dev] [PATCH 3/4] if_proxy: add simple functionality test Andrzej Ostruszka
2020-03-06 16:41 ` [dpdk-dev] [PATCH 4/4] if_proxy: add example application Andrzej Ostruszka
2020-03-06 17:17 ` [dpdk-dev] [PATCH 0/4] Introduce IF proxy library Andrzej Ostruszka
2020-03-10 11:10 ` [dpdk-dev] [PATCH v2 " Andrzej Ostruszka
2020-03-10 11:10   ` [dpdk-dev] [PATCH v2 1/4] lib: introduce IF Proxy library Andrzej Ostruszka
2020-07-02  0:34     ` Stephen Hemminger
2020-07-07 20:13       ` Andrzej Ostruszka [C]
2020-07-08 16:07         ` Morten Brørup [this message]
2020-07-09  8:43           ` Andrzej Ostruszka [C]
2020-07-22  0:40             ` Thomas Monjalon
2020-07-22  8:45               ` Jerin Jacob
2020-07-22  8:56                 ` Thomas Monjalon
2020-07-22  9:09                   ` Jerin Jacob
2020-07-22  9:27                     ` Thomas Monjalon
2020-07-22  9:54                       ` Jerin Jacob
2020-07-23 14:09                         ` [dpdk-dev] [EXT] " Andrzej Ostruszka [C]
2020-03-10 11:10   ` [dpdk-dev] [PATCH v2 2/4] if_proxy: add library documentation Andrzej Ostruszka
2020-03-10 11:10   ` [dpdk-dev] [PATCH v2 3/4] if_proxy: add simple functionality test Andrzej Ostruszka
2020-03-10 11:10   ` [dpdk-dev] [PATCH v2 4/4] if_proxy: add example application Andrzej Ostruszka
2020-03-25  8:08   ` [dpdk-dev] [PATCH v2 0/4] Introduce IF proxy library David Marchand
2020-03-25 11:11     ` Morten Brørup
2020-03-26 17:42       ` Andrzej Ostruszka
2020-04-02 13:48         ` Andrzej Ostruszka [C]
2020-04-03 17:19           ` Thomas Monjalon
2020-04-03 19:09             ` Jerin Jacob
2020-04-03 21:18               ` Morten Brørup
2020-04-03 21:57                 ` Thomas Monjalon
2020-04-04 10:18                   ` Jerin Jacob
2020-04-10 10:41                     ` Morten Brørup
2020-04-04 18:30             ` Andrzej Ostruszka [C]
2020-04-04 19:58               ` Thomas Monjalon
2020-04-10 10:03                 ` Morten Brørup
2020-04-10 12:28                   ` Jerin Jacob
2020-03-26 12:41     ` Andrzej Ostruszka
2020-03-30 19:23       ` Andrzej Ostruszka
2020-04-03 21:42   ` Thomas Monjalon
2020-04-04 18:07     ` Andrzej Ostruszka [C]
2020-04-04 19:51       ` Thomas Monjalon
2020-04-16 16:11 ` [dpdk-dev] [PATCH " Stephen Hemminger
2020-04-16 16:49   ` Jerin Jacob
2020-04-16 17:04     ` Stephen Hemminger
2020-04-16 17:26       ` Andrzej Ostruszka [C]
2020-04-16 17:27       ` Jerin Jacob
2020-04-16 17:12     ` Andrzej Ostruszka [C]
2020-04-16 17:19       ` Stephen Hemminger
2020-05-04  8:53 ` [dpdk-dev] [PATCH v3 " Andrzej Ostruszka
2020-05-04  8:53   ` [dpdk-dev] [PATCH v3 1/4] lib: introduce IF Proxy library Andrzej Ostruszka
2020-05-04  8:53   ` [dpdk-dev] [PATCH v3 2/4] if_proxy: add library documentation Andrzej Ostruszka
2020-05-04  8:53   ` [dpdk-dev] [PATCH v3 3/4] if_proxy: add simple functionality test Andrzej Ostruszka
2020-05-04  8:53   ` [dpdk-dev] [PATCH v3 4/4] if_proxy: add example application Andrzej Ostruszka
2020-06-22  9:21 ` [dpdk-dev] [PATCH v4 0/4] Introduce IF proxy library Andrzej Ostruszka
2020-06-22  9:21   ` [dpdk-dev] [PATCH v4 1/4] lib: introduce IF Proxy library Andrzej Ostruszka
2020-06-22  9:21   ` [dpdk-dev] [PATCH v4 2/4] if_proxy: add library documentation Andrzej Ostruszka
2020-06-22  9:21   ` [dpdk-dev] [PATCH v4 3/4] if_proxy: add simple functionality test Andrzej Ostruszka
2020-06-22  9:21   ` [dpdk-dev] [PATCH v4 4/4] if_proxy: add example application Andrzej Ostruszka

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=98CBD80474FA8B44BF855DF32C47DC35C6110C@smartserver.smartshare.dk \
    --to=mb@smartsharesystems.com \
    --cc=aostruszka@marvell.com \
    --cc=dev@dpdk.org \
    --cc=stephen@networkplumber.org \
    --cc=thomas@monjalon.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git