Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PATCH v13] Misc fixes #377

Closed
wants to merge 15 commits into from
Closed

[PATCH v13] Misc fixes #377

wants to merge 15 commits into from

Conversation

lumag
Copy link

@lumag lumag commented Dec 31, 2017

Several small fixes produced while working on updating ODP packaging.

Dmitry Eremin-Solenikov added 4 commits December 31, 2017 02:38
libodphelper.so uses symbols from libpthread and libodp-linux.so, link
it aganst those libraries.

Signed-off-by: Dmitry Eremin-Solenikov <dmitry.ereminsolenikov@linaro.org>
Omitting those images in Makefile.am made them disappear from
distribution tarball resulting in an improperly-built documentation
files.

Signed-off-by: Dmitry Eremin-Solenikov <dmitry.ereminsolenikov@linaro.org>
Omitting those images in Makefile.am made them disappear from
distribution tarball resulting in an improperly-built documentation
files.

Signed-off-by: Dmitry Eremin-Solenikov <dmitry.ereminsolenikov@linaro.org>
Receieved -> Received.

Signed-off-by: Dmitry Eremin-Solenikov <dmitry.ereminsolenikov@linaro.org>
@muvarov muvarov changed the title Misc fixes [PATCH v1] Misc fixes Dec 31, 2017
@codecov
Copy link

codecov bot commented Dec 31, 2017

Codecov Report

Merging #377 into master will not change coverage.
The diff coverage is 100%.

@@            Coverage Diff            @@
##            master      #377   +/-   ##
=========================================
  Coverage   78.677%   78.677%           
=========================================
  Files          196       196           
  Lines        34616     34616           
=========================================
  Hits         27235     27235           
  Misses        7381      7381
Impacted Files Coverage Δ
test/validation/api/time/time.c 96.017% <100%> (ø) ⬆️
test/performance/odp_pktio_ordered.c 71.138% <0%> (-0.204%) ⬇️
platform/linux-generic/odp_traffic_mngr.c 78.98% <0%> (+0.042%) ⬆️

@muvarov muvarov changed the title [PATCH v1] Misc fixes [PATCH v2] Misc fixes Dec 31, 2017
@muvarov muvarov changed the title [PATCH v2] Misc fixes [PATCH v3] Misc fixes Dec 31, 2017
Copy link
Contributor

@Bill-Fischofer-Linaro Bill-Fischofer-Linaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed-by: Bill Fischofer bill.fischofer@linaro.org

@muvarov
Copy link
Contributor

muvarov commented Jan 1, 2018

--disable-static-link should be added to Travis also.

@muvarov
Copy link
Contributor

muvarov commented Jan 1, 2018

and also status to ./configure output status is needed.

configure.ac Outdated
[static_link=$enableval],
[static_link=yes])
AM_CONDITIONAL([STATIC_LINK], [test "x$static_link" != "xno"])

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At his time if --disable-static-link is specified then linking is static. If it's not specified that it's dynamic. I'm looking on odp_generator.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, no. Please recheck. I will update description to mention that is means static linking of libodp-linux into examples.

@muvarov muvarov changed the title [PATCH v3] Misc fixes [PATCH v4] Misc fixes Jan 1, 2018
@lumag
Copy link
Author

lumag commented Jan 1, 2018

@muvarov updated

@muvarov
Copy link
Contributor

muvarov commented Jan 1, 2018

@lumag for v3:

./configure --disable-static-link
make
ldd ./example/generator/odp_generator
	not a dynamic executable

Dmitry Eremin-Solenikov added 2 commits January 4, 2018 00:54
Populate test/Makefile.inc with more variables, allowing us to drop
platform test/Makefile.inc.

Signed-off-by: Dmitry Eremin-Solenikov <dmitry.ereminsolenikov@linaro.org>
LDADD variable can be used to pass other libraries (like DPDK). Stop
overriding LDADD entirely. Instead use PRELDADD variable or appending to
LDADD.

Signed-off-by: Dmitry Eremin-Solenikov <dmitry.ereminsolenikov@linaro.org>
@muvarov muvarov changed the title [PATCH v10] Misc fixes [PATCH v11] Misc fixes Jan 3, 2018
@lumag
Copy link
Author

lumag commented Jan 3, 2018

  • Using gettimeofday() instead of clock()
  • Dropped DPDK linkage / no-static-tests for now, will submit in separate PR.

@lumag
Copy link
Author

lumag commented Jan 8, 2018

@Bill-Fischofer-Linaro @muvarov PR was updated to drop linking PRs. Please review.

@@ -1,5 +1,7 @@
include $(top_srcdir)/example/Makefile.inc

LDADD += $(ATOMIC_LIBS)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This kind of thing which is better to avoid. If we are going to merge odp implementations platforms to common repo then it might be libs which are suitable for linux-generic are not needed for other platform. I think examples Makefiles have to be more common.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This example is not generic enough. It uses atomic primitives directly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, Than. Agree.

double sec_t, sec_c;
odp_time_t sec = time_from_ns(ODP_TIME_SEC_IN_NS);

c1 = clock();
i = gettimeofday(&tv1, NULL);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gettimeofday() may be changed on NTP/PTP and it's not good for measuring intervals. clock_gettime(CLOCK_MONOTONIC, ...) I think will be good one here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also please add link to bugz if it's bug fix to git comment.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice idea, I will update this PR.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@muvarov maybe we can reenable time_main test in Travis with this fix? What do you think wrt previous issue

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lumag yep, I think it has to pass now.

Dmitry Eremin-Solenikov added 5 commits January 15, 2018 02:42
ODP test for time API uses clock() to compare time against. However
clock() returns processor time used by program, which can differ between
runs. Use clock_gettime() as a time source to compare against.

Signed-off-by: Dmitry Eremin-Solenikov <dmitry.ereminsolenikov@linaro.org>
Fixes: https://bugs.linaro.org/show_bug.cgi?id=3572
This reverts commit 7a7e3ff. After
fixing time test to use clock_gettime() instead of clock(), this test
should not fail under Travis/coverage. Reenable the test.

Signed-off-by: Dmitry Eremin-Solenikov <dmitry.ereminsolenikov@linaro.org>
sysconfig is not used in TigerMoth, so let's drop the variable in
linux-gen's Makefile.

Signed-off-by: Dmitry Eremin-Solenikov <dmitry.ereminsolenikov@linaro.org>
libconfig is not used in TigerMoth, so let's drop the variable in
test's Makefile.inc.

Signed-off-by: Dmitry Eremin-Solenikov <dmitry.ereminsolenikov@linaro.org>
ipsec_api example installs program with the same name, as ipsec example
does. Remove file conflict between two examples.

Signed-off-by: Dmitry Eremin-Solenikov <dmitry.ereminsolenikov@linaro.org>
@muvarov muvarov changed the title [PATCH v11] Misc fixes [PATCH v12] Misc fixes Jan 14, 2018
@lumag
Copy link
Author

lumag commented Jan 14, 2018

@muvarov fixed. Added reference to new time_main bug

There is no need to pass PMDs when linking with shared DPDK library.
Just pass -ldpdk which will pick up all PMDS dynamically. When linking
with static DPKD library it is required to pass additional libraries
after whole PMD string to fix linking when Libtool will reorder flags.
So provide just two variables DPDK_LIBS and DPDK_LIBS_LT. Former should
be passed directly to gcc, while later should be passed to libtool.

Signed-off-by: Dmitry Eremin-Solenikov <dmitry.ereminsolenikov@linaro.org>
@muvarov muvarov changed the title [PATCH v12] Misc fixes [PATCH v13] Misc fixes Jan 15, 2018
@muvarov
Copy link
Contributor

muvarov commented Jan 15, 2018

Merged.

@muvarov muvarov closed this Jan 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants