[PATCH CLOUD-DEV v5] Modular pktio ops #139

heyi-arm · 2017-08-23T05:10:46Z

Part one (3 commits): Apply modular framework to the pktio ops subsystem

Previously we register pktio ops in a static array pktio_if_ops[];
This converts it into a modular and extensible manner.

Part two (7 commits): Minor code re-factory on each pktio ops implementation

Cleanup the odp_packet_io_internal.h header file
Move each impl specific data structure into a dedicated header file
Consistent the header file names and data structure names.
Dpdk pktio ops demonstrates to hide the impl private data locally.

Part three Makefiles re-organization

Read [PATCH v6] Continue working on improving build system #127 to learn Dmitry's working on build system: no conflict or duplicate works
Wrote a Makefile framework which allows new subsystem and module writers to easily add their works by providing brief descriptions only, and Makefile rules and targets etc will be automatically generated.
In internal review process

GBalakrishna · 2017-08-23T11:56:49Z

platform/linux-generic/pktio/subsystem.c

+
+#ifdef ODP_PKTIO_DPDK
+	enable_link_dpdk_pktio_ops = 1;
+#endif


I feel we don't need PKTIO_DPDK anylonger as we now already have it from linux-dpdk. I have sent a PR https://github.com/Linaro/odp/pull/140 with necessary updates.

Yes, Krishna, as I replied in another email, I suggest an approach that add workable solutions first and after then remove obsolete code.

With PR#140 it looks likes that we removed linux-generic dpdk pktio and lost dpdk pktio at all, now wait for a new design which seems still in discussion.

I'll comment these in your PR as a review suggestion.

We have the working solution currently in linux-dpdk. Question is, why we still want to have it in linux-generic ?

Bill-Fischofer-Linaro · 2017-08-23T20:17:23Z

Note that this PR appears to have broken the linux-dpdk build, as reported by Travis. We can discuss how best to resolve this during tomorrow's call.

heyi-arm · 2017-08-25T08:14:45Z

While work on 3rd part of Makefile re-organization, I met a difficulty and very like to share with you, and maybe you have more experience with automake and can help provide suggestions:

I'm trying to simplify subsystem and module writers effort to add their code into Makefile infra, I create a Makefile-paths.include, allow writers to do only two steps:

1st add the new subsystem into variable SUBSYSTEMS
SUBSYSTEMS += new
2nd list the modules (implementations) of this subsystem, and can specify the selection of one module as active(default) by appending an ":active" suffix
__subsystem_new_MODULES = one:active two three four

And that's all, I wrote a Makefile-subsystems.include which automatically generate all targets and rules for build the subsystem and modules based on above description.

Unfortunately it does not work with automake, the works are pushed:

https://github.com/heyi-linaro/odp/tree/modular-pktio-ops-Makefiles

In the branch you will find the platform/linux-generic/Makefile-paths.inc and Makefile-subsystems.inc described above.

The reason I figured currently is because automake statically analyze Makefile.am and generate Makefile.in, without run any of the dynamic expansions included in Makefile-subsystems.inc. But what I needed is exactly that.

I'll continue back next week, sorry today will not be able to send out internal review.

Thanks and best regards, Yi

joseppc · 2017-08-24T12:45:03Z

platform/linux-generic/include/odp_pktio_ops_subsystem.h

+
+/* Subsystem APIs declarations */
+ODP_SUBSYSTEM_API(pktio_ops, int, open, odp_pktio_t,
+		  pktio_entry_t *, const char *, odp_pool_t);


Isn't this going to declare an external function prototype called odp_pktio_ops_open(...)? However the implementation won't exist, right?
I understand we need to define the function types (odp_pktio_ops_open_t) for use in the structure below, but do we need to declare all those function prototypes in this header file?

Yes, here we only use the ODP_SUBSYSTEM_API() macro to typedef prototype function pointers and leave the prototype declarations no defines.

I suggest we accept this as is and consider how to handle this kind of tasks more properly in terms of autogen. as described in heyi-arm/pure-interface#6

joseppc · 2017-08-25T15:16:19Z

platform/linux-generic/pktio/subsystem.c

+	/* Further initialization per subsystem */
+
+#ifdef ODP_PKTIO_DPDK
+	enable_link_dpdk_pktio_ops = 1;


how and where are these enable_link_* variables used?
How will this work with dynamically loadable modules?

This is temporary variables to make sure these modules are linked into ODP library.

Because the Makefiles have not been re-organized to build modules each as real independent module (.a or .so) and either link them with --whole-archive or --no-as-needed flags, this is necessary for current Makefiles.

joseppc · 2017-08-28T08:06:24Z

platform/linux-generic/pktio/dpdk.c

-	.init_global = dpdk_pktio_init_global,
-	.init_local = dpdk_pktio_init_local,
-	.term = NULL,
+static pktio_ops_module_t dpdk_pktio_ops = {


Can we find a solution so these structures are kept as const? If I get this correctly, apart from the linked list embedded in the "base class", there is nothing that should be mutable here, right?

Yes, I'm thinking how to achieve this. Can it be later patch improvements.

joseppc · 2017-08-28T08:25:36Z

platform/linux-generic/pktio/pcap.c

@@ -51,7 +51,8 @@ static const char pcap_mac[] = {0x02, 0xe9, 0x34, 0x80, 0x73, 0x04};

 static int pcapif_stats_reset(pktio_entry_t *pktio_entry);

-static int _pcapif_parse_devname(pkt_pcap_t *pcap, const char *devname)
+static int _pcapif_parse_devname(
+	pktio_ops_pcap_data_t *pcap, const char *devname)


indentation style

Run out of 80 characters?

Would be better to split it after static int if that's the problem, or at a parameter boundary. Splitting after the open paren looks odd. For example:

static int _pcapif_parse_devname(pktio_ops_pcap_data_t *pcap, const char *devname)

would be normal ODP style here.

joseppc · 2017-08-28T08:28:08Z

platform/linux-generic/pktio/pcap.c

@@ -163,7 +165,8 @@ static int pcapif_init(odp_pktio_t id ODP_UNUSED, pktio_entry_t *pktio_entry,

 static int pcapif_close(pktio_entry_t *pktio_entry)
 {
-	pkt_pcap_t *pcap = &pktio_entry->s.pkt_pcap;
+	pktio_ops_pcap_data_t *pcap =
+		&pktio_entry->ops_data(pcap);


declaration fins in one line (there are others in this file too)

Run out of 80 characters?

This doesn't seem to go anywhere near the 80-char limit so should be fine as a single line.

heyi-arm · 2017-09-01T08:22:37Z

Part three Makefiles re-organization

Read #127 to learn Dmitry's working on build system: no conflict or duplicate works
Wrote a Makefile framework which allows new subsystem and module writers to easily add their works by providing brief descriptions only, and Makefile rules and targets etc will be automatically generated.
In internal review process

heyi-arm · 2017-09-14T03:39:09Z

@Bill-Fischofer-Linaro @joseppc @nagarahalli @GBalakrishna

This PR has been fixed for both linux-dpdk build and checkpatch issues.
For those commits both I and Krishna contributed, I've added Signed-off-by stamps.

bogdanPricope · 2017-09-19T11:52:05Z

Reviewed-by: Bogdan Pricope bogdan.pricope@linaro.org

Push?

joseppc · 2017-09-19T13:11:10Z

Reviewed-by: Josep Puigdemont josep.puigdemont@linaro.org

Bill-Fischofer-Linaro

I'm OK with this PR as long as we don't change interface names as part of it (as noted in comments).

Bill-Fischofer-Linaro · 2017-09-19T22:36:24Z

platform/linux-dpdk/pktio/dpdk.c

+ * will remove in Makefile scheme changes.
+ */
+int enable_link_dpdk_pktio_ops = 0;
+


Please open an issue so that we don't forget about this "to do". We decided that this is a good use for GitHub issues.

Yes, created #184

Bill-Fischofer-Linaro · 2017-09-19T22:44:31Z

platform/linux-generic/pktio/loopback.c

+static pktio_ops_module_t loopback_pktio_ops = {
+	.base = {
+		.name = "loopback",
+		.init_local = NULL,


Why are we changing this pktio name? This will cause existing applications that use the "loop" interface to need to change, which seems unnecessary. I'd keep this PR strictly to modularization without any application impact. If we want to change the pktio name that should be a separate PR.

Yes, will change it back to the original.

Bill-Fischofer-Linaro · 2017-09-19T22:46:32Z

platform/linux-generic/pktio/loopback.c

+/* Temporary variable to enable link this module,
+ * will remove in Makefile scheme changes.
+ */
+int enable_link_loopback_pktio_ops = 0;


As noted, "to dos" should be tracked as issues. These need not be separate issues, as the main purpose is simply to remind us that pktio still has work to do.

Yes, created #184

Bill-Fischofer-Linaro · 2017-09-19T22:47:33Z

platform/linux-generic/pktio/netmap.c

+ * will remove in Makefile scheme changes.
+ */
+int enable_link_netmap_pktio_ops = 0;
+


Same "to do" comment here.

Bill-Fischofer-Linaro · 2017-09-19T22:49:53Z

platform/linux-generic/pktio/pcap.c

@@ -51,7 +51,8 @@ static const char pcap_mac[] = {0x02, 0xe9, 0x34, 0x80, 0x73, 0x04};

 static int pcapif_stats_reset(pktio_entry_t *pktio_entry);

-static int _pcapif_parse_devname(pkt_pcap_t *pcap, const char *devname)
+static int _pcapif_parse_devname(
+	pktio_ops_pcap_data_t *pcap, const char *devname)


Would be better to split it after static int if that's the problem, or at a parameter boundary. Splitting after the open paren looks odd. For example:

static int _pcapif_parse_devname(pktio_ops_pcap_data_t *pcap, const char *devname)

would be normal ODP style here.

Bill-Fischofer-Linaro · 2017-09-19T22:59:11Z

platform/linux-generic/pktio/socket.c

+/* Temporary variable to enable link this module,
+ * will remove in Makefile scheme changes.
+ */
+int enable_link_socket_pktio_ops = 0;


Same "to do" comment.

Yes, created #184

Bill-Fischofer-Linaro · 2017-09-19T23:00:28Z

platform/linux-generic/pktio/socket_mmap.c

-	.term = NULL,
+static pktio_ops_module_t socket_mmap_pktio_ops = {
+	.base = {
+		.name = "socket mmap",


Again you've changed the interface name here from socket_mmap to socket mmap. Keep the names unchanged as part of this PR to avoid application impact.

Will fix all these.

Bill-Fischofer-Linaro · 2017-09-19T23:00:58Z

platform/linux-generic/pktio/socket_mmap.c

+/* Temporary variable to enable link this module,
+ * will remove in Makefile scheme changes.
+ */
+int enable_link_socket_mmap_pktio_ops = 0;


Same "to do" comment.

Yes, created #184

Bill-Fischofer-Linaro · 2017-09-19T23:01:54Z

platform/linux-generic/pktio/tap.c

+/* Temporary variable to enable link this module,
+ * will remove in Makefile scheme changes.
+ */
+int enable_link_tap_pktio_ops = 0;


Same "to do" comment.

Yes, created #184

Bill-Fischofer-Linaro · 2017-09-19T23:04:16Z

platform/linux-dpdk/pktio/dpdk.c

-	.name = "odp-dpdk",
+static pktio_ops_module_t dpdk_pktio_ops = {
+	.base = {
+		.name = "dpdk",


Do not change interface names as part of this PR. Any name changes should be a separate PR where we can access application impact. In the case of DPDK devices this may be appropriate here, but not as part of this PR.

Yes, will fix all these.

Static SUBSYSTEM_FOREACH_TEMPLATE constraints the functions to be instantiated in the source files which invoke them, this causes code scatter shoots. Signed-off-by: Yi He <yi.he@linaro.org> Reviewed-by: Brian Brooks <brian.brooks@arm.com> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com> Reviewed-by: Kevin Wang <kevin.wang@arm.com> Reviewed-by: Bogdan Pricope <bogdan.pricope@linaro.org> Reviewed-by: Josep Puigdemont <josep.puigdemont@linaro.org> Reviewed-by: Bill Fischofer <bill.fischofer@linaro.org>

l2fwd_simple_run.sh waits 1 second to allow test application to forward more than 10 packets. It works in case pktios are listed statically and pcap takes precedence over socket pktios. But with dynamically registered pktios the order was not guaranteed and socket pktios may take precedence over pcap pktio, which spent more time in open operation and caused this test failed. Disable the socket pktios since they mistakenly take too long time in open operation, future code refactory should solve the problem thoroughly. Signed-off-by: Yi He <yi.he@linaro.org> Reviewed-by: Brian Brooks <brian.brooks@arm.com> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com> Reviewed-by: Kevin Wang <kevin.wang@arm.com> Reviewed-by: Bogdan Pricope <bogdan.pricope@linaro.org> Reviewed-by: Josep Puigdemont <josep.puigdemont@linaro.org> Reviewed-by: Bill Fischofer <bill.fischofer@linaro.org>

Apply modular framework to the pktio ops registration, convert the static array for impls registration into a dynamic and extensible modularization manner. Signed-off-by: Yi He <yi.he@linaro.org> Signed-off-by: Balakrishna Garapati <balakrishna.garapati@linaro.org> Reviewed-by: Brian Brooks <brian.brooks@arm.com> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com> Reviewed-by: Kevin Wang <kevin.wang@arm.com> Reviewed-by: Bogdan Pricope <bogdan.pricope@linaro.org> Reviewed-by: Josep Puigdemont <josep.puigdemont@linaro.org> Reviewed-by: Bill Fischofer <bill.fischofer@linaro.org>

Rename the source as loopback.c, move implementation specific data structure into dedicated header file. Signed-off-by: Yi He <yi.he@linaro.org> Signed-off-by: Balakrishna Garapati <balakrishna.garapati@linaro.org> Reviewed-by: Brian Brooks <brian.brooks@arm.com> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com> Reviewed-by: Bogdan Pricope <bogdan.pricope@linaro.org> Reviewed-by: Josep Puigdemont <josep.puigdemont@linaro.org> Reviewed-by: Bill Fischofer <bill.fischofer@linaro.org>

Move implementation specific data structure into dedicated header file. Signed-off-by: Yi He <yi.he@linaro.org> Signed-off-by: Balakrishna Garapati <balakrishna.garapati@linaro.org> Reviewed-by: Brian Brooks <brian.brooks@arm.com> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com> Reviewed-by: Bogdan Pricope <bogdan.pricope@linaro.org> Reviewed-by: Josep Puigdemont <josep.puigdemont@linaro.org> Reviewed-by: Bill Fischofer <bill.fischofer@linaro.org>

Move implementation specific data structure into dedicated header file. Signed-off-by: Yi He <yi.he@linaro.org> Reviewed-by: Brian Brooks <brian.brooks@arm.com> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com> Reviewed-by: Bogdan Pricope <bogdan.pricope@linaro.org> Reviewed-by: Josep Puigdemont <josep.puigdemont@linaro.org> Reviewed-by: Bill Fischofer <bill.fischofer@linaro.org>

Rename implementation specific data structure and its dedicated header file to accommodate uniformed name scheme. Signed-off-by: Yi He <yi.he@linaro.org> Signed-off-by: Balakrishna Garapati <balakrishna.garapati@linaro.org> Reviewed-by: Brian Brooks <brian.brooks@arm.com> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com> Reviewed-by: Bogdan Pricope <bogdan.pricope@linaro.org> Reviewed-by: Josep Puigdemont <josep.puigdemont@linaro.org> Reviewed-by: Bill Fischofer <bill.fischofer@linaro.org>

Rename implementation specific data structure and its dedicated header file to accommodate uniformed name scheme. Signed-off-by: Yi He <yi.he@linaro.org> Reviewed-by: Brian Brooks <brian.brooks@arm.com> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com> Reviewed-by: Bogdan Pricope <bogdan.pricope@linaro.org> Reviewed-by: Josep Puigdemont <josep.puigdemont@linaro.org> Reviewed-by: Bill Fischofer <bill.fischofer@linaro.org>

Rename implementation specific data structure and move its dedicated header file to impl local, decouple it from pktio_entry_t data structure. Signed-off-by: Yi He <yi.he@linaro.org> Signed-off-by: Balakrishna Garapati <balakrishna.garapati@linaro.org> Reviewed-by: Brian Brooks <brian.brooks@arm.com> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com> Reviewed-by: Bogdan Pricope <bogdan.pricope@linaro.org> Reviewed-by: Josep Puigdemont <josep.puigdemont@linaro.org> Reviewed-by: Bill Fischofer <bill.fischofer@linaro.org>

heyi-arm · 2017-09-21T01:55:20Z

merged and closed.

muvarov changed the title ~~Modular pktio ops - part one and two~~ [PATCH CLOUD-DEV v1] Modular pktio ops - part one and two Aug 23, 2017

muvarov added Email_sent labels Aug 23, 2017

GBalakrishna reviewed Aug 23, 2017

View reviewed changes

heyi-arm mentioned this pull request Aug 25, 2017

[PATCH CLOUD-DEV v1] linux-dpdk: introduce pool modular framework #138

Closed

joseppc reviewed Aug 28, 2017

View reviewed changes

heyi-arm changed the title ~~[PATCH CLOUD-DEV v1] Modular pktio ops - part one and two~~ [PATCH CLOUD-DEV v1] Modular pktio ops Sep 1, 2017

heyi-arm force-pushed the modular-pktio-ops branch from 6cbf07f to 55819b6 Compare September 13, 2017 08:44

muvarov changed the title ~~[PATCH CLOUD-DEV v1] Modular pktio ops~~ [PATCH CLOUD-DEV v2] Modular pktio ops Sep 13, 2017

muvarov added checkpatch and removed Email_sent labels Sep 13, 2017

heyi-arm force-pushed the modular-pktio-ops branch from 55819b6 to 10fed60 Compare September 14, 2017 02:54

muvarov changed the title ~~[PATCH CLOUD-DEV v2] Modular pktio ops~~ [PATCH CLOUD-DEV v3] Modular pktio ops Sep 14, 2017

muvarov added Email_sent and removed Email_sent labels Sep 14, 2017

heyi-arm force-pushed the modular-pktio-ops branch from 10fed60 to 4e521e7 Compare September 19, 2017 02:10

muvarov changed the title ~~[PATCH CLOUD-DEV v3] Modular pktio ops~~ [PATCH CLOUD-DEV v4] Modular pktio ops Sep 19, 2017

muvarov added Email_sent and removed Email_sent labels Sep 19, 2017

Bill-Fischofer-Linaro suggested changes Sep 19, 2017

View reviewed changes

heyi-arm mentioned this pull request Sep 20, 2017

TODO: remove temporary flags for pktio modules linkage #184

Closed

Yi He added 10 commits September 20, 2017 14:35

heyi-arm force-pushed the modular-pktio-ops branch from 4e521e7 to e89a0ed Compare September 20, 2017 08:22

muvarov changed the title ~~[PATCH CLOUD-DEV v4] Modular pktio ops~~ [PATCH CLOUD-DEV v5] Modular pktio ops Sep 20, 2017

muvarov added Email_sent and removed Email_sent labels Sep 20, 2017

Bill-Fischofer-Linaro approved these changes Sep 20, 2017

View reviewed changes

heyi-arm merged commit e89a0ed into OpenDataPlane:cloud-dev Sep 21, 2017

heyi-arm deleted the modular-pktio-ops branch September 21, 2017 01:55

[PATCH CLOUD-DEV v5] Modular pktio ops #139

[PATCH CLOUD-DEV v5] Modular pktio ops #139

Conversation

heyi-arm commented Aug 23, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bill-Fischofer-Linaro commented Aug 23, 2017

heyi-arm commented Aug 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heyi-arm commented Sep 1, 2017

heyi-arm commented Sep 14, 2017

bogdanPricope commented Sep 19, 2017

joseppc commented Sep 19, 2017

Bill-Fischofer-Linaro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heyi-arm commented Sep 21, 2017

heyi-arm commented Aug 23, 2017 •

edited