You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by GitBox <gi...@apache.org> on 2020/06/10 22:39:47 UTC
[GitHub] [activemq-artemis-native] clebertsuconic opened a new pull request #7: ARTEMIS-2800 Fixes around Linux Kernel Bug
clebertsuconic opened a new pull request #7:
URL: https://github.com/apache/activemq-artemis-native/pull/7
This will contain several fixes around this bug:
https://bugzilla.redhat.com/show_bug.cgi?id=1845326
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis-native] clebertsuconic commented on a change in pull request #7: ARTEMIS-2800 Fixes around Linux Kernel Bug
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #7:
URL: https://github.com/apache/activemq-artemis-native/pull/7#discussion_r438767393
##########
File path: src/main/c/org_apache_activemq_artemis_nativo_jlibaio_LibaioContext.c
##########
@@ -129,9 +129,24 @@ static int user_io_getevents(io_context_t aio_ctx, unsigned int max,
struct io_event *events)
{
long i = 0;
- unsigned head;
+ unsigned head, tail, avail;
struct aio_ring *ring = to_aio_ring(aio_ctx);
+ // continue until the available values are <= max
+ do {
+ head = ring->head;
+ mem_barrier();
+ tail = ring->tail;
+
+ if (tail >= head)
+ avail = tail - head;
+ else
+ avail = ring->nr - (head - tail);
+
+ if (avail <= max)
+ break;
+ } while (1);
Review comment:
@JeffMoyer I will try to place that loop back. I had an issue on the testsuite and I thought it could be related to this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis-native] franz1981 commented on a change in pull request #7: ARTEMIS-2800 Fixes around Linux Kernel Bug
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #7:
URL: https://github.com/apache/activemq-artemis-native/pull/7#discussion_r438769566
##########
File path: src/main/c/org_apache_activemq_artemis_nativo_jlibaio_LibaioContext.c
##########
@@ -129,9 +129,24 @@ static int user_io_getevents(io_context_t aio_ctx, unsigned int max,
struct io_event *events)
{
long i = 0;
- unsigned head;
+ unsigned head, tail, avail;
struct aio_ring *ring = to_aio_ring(aio_ctx);
+ // continue until the available values are <= max
+ do {
+ head = ring->head;
+ mem_barrier();
+ tail = ring->tail;
+
+ if (tail >= head)
+ avail = tail - head;
+ else
+ avail = ring->nr - (head - tail);
+
+ if (avail <= max)
+ break;
+ } while (1);
Review comment:
@clebertsuconic @JeffMoyer I still think it worths to check that tail is not just changed but is "right" too ie < max expected size.
OR: perf-wise a `pause` instruction in the tight loop while reading tail seems a good idea too
##########
File path: src/main/c/org_apache_activemq_artemis_nativo_jlibaio_LibaioContext.c
##########
@@ -129,9 +129,24 @@ static int user_io_getevents(io_context_t aio_ctx, unsigned int max,
struct io_event *events)
{
long i = 0;
- unsigned head;
+ unsigned head, tail, avail;
struct aio_ring *ring = to_aio_ring(aio_ctx);
+ // continue until the available values are <= max
+ do {
+ head = ring->head;
+ mem_barrier();
+ tail = ring->tail;
+
+ if (tail >= head)
+ avail = tail - head;
+ else
+ avail = ring->nr - (head - tail);
+
+ if (avail <= max)
+ break;
+ } while (1);
Review comment:
@clebertsuconic @JeffMoyer I still think it worths to check that tail is not just changed but is "right" too ie < max expected size.
OT: perf-wise a `pause` instruction in the tight loop while reading tail seems a good idea too
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis-native] asfgit merged pull request #7: ARTEMIS-2800 Fixes around Linux Kernel Bug
Posted by GitBox <gi...@apache.org>.
asfgit merged pull request #7:
URL: https://github.com/apache/activemq-artemis-native/pull/7
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis-native] clebertsuconic commented on a change in pull request #7: ARTEMIS-2800 Fixes around Linux Kernel Bug
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #7:
URL: https://github.com/apache/activemq-artemis-native/pull/7#discussion_r439039650
##########
File path: src/main/c/org_apache_activemq_artemis_nativo_jlibaio_LibaioContext.c
##########
@@ -100,57 +125,59 @@ static inline struct aio_ring* to_aio_ring(io_context_t aio_ctx) {
return (struct aio_ring*) aio_ctx;
}
+static int user_io_getevents(io_context_t aio_ctx, unsigned int max,
+ struct io_event *events)
+{
+ long i = 0;
+ unsigned head, tail, avail;
+ struct aio_ring *ring = to_aio_ring(aio_ctx);
+
+ // continue until the available values are <= max
+ do {
+ head = ring->head;
+ mem_barrier();
+ tail = ring->tail;
+
+ if (tail >= head)
+ avail = tail - head;
+ else
+ avail = ring->nr - (head - tail);
+
Review comment:
I am doing something much simpler than that..
I'm keeping the original loop I know it works
and I'm now making a syscall if available >= max
this was enough to go around the issue.
a lot less stress.. everything is working again.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis-native] clebertsuconic commented on pull request #7: ARTEMIS-2800 Fixes around Linux Kernel Bug
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on pull request #7:
URL: https://github.com/apache/activemq-artemis-native/pull/7#issuecomment-642328191
I believe I will have to remove the workaround loop as I had a few hungs on the testsuite.
I will resume this tomorrow.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis-native] clebertsuconic commented on pull request #7: ARTEMIS-2800 Fixes around Linux Kernel Bug
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on pull request #7:
URL: https://github.com/apache/activemq-artemis-native/pull/7#issuecomment-642326114
@franz1981 review please?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis-native] franz1981 commented on a change in pull request #7: ARTEMIS-2800 Fixes around Linux Kernel Bug
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #7:
URL: https://github.com/apache/activemq-artemis-native/pull/7#discussion_r438768078
##########
File path: src/main/c/org_apache_activemq_artemis_nativo_jlibaio_LibaioContext.c
##########
@@ -129,9 +129,24 @@ static int user_io_getevents(io_context_t aio_ctx, unsigned int max,
struct io_event *events)
{
long i = 0;
- unsigned head;
+ unsigned head, tail, avail;
struct aio_ring *ring = to_aio_ring(aio_ctx);
+ // continue until the available values are <= max
+ do {
+ head = ring->head;
+ mem_barrier();
+ tail = ring->tail;
+
+ if (tail >= head)
+ avail = tail - head;
+ else
+ avail = ring->nr - (head - tail);
+
+ if (avail <= max)
+ break;
+ } while (1);
Review comment:
mmmm you're right, effectively this version won't show that there is just a single writer for `head` and deserve some more comments too, my bad
@clebertsuconic Please bud, we can use the original version proposed by @JeffMoyer
@JeffMoyer OT: do you think it would be better to replace the reaper loop using `%` with a `memcpy`? If yes I would send a different PR to address it too
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis-native] franz1981 commented on a change in pull request #7: ARTEMIS-2800 Fixes around Linux Kernel Bug
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #7:
URL: https://github.com/apache/activemq-artemis-native/pull/7#discussion_r438861227
##########
File path: src/main/c/org_apache_activemq_artemis_nativo_jlibaio_LibaioContext.c
##########
@@ -100,57 +125,59 @@ static inline struct aio_ring* to_aio_ring(io_context_t aio_ctx) {
return (struct aio_ring*) aio_ctx;
}
+static int user_io_getevents(io_context_t aio_ctx, unsigned int max,
+ struct io_event *events)
+{
+ long i = 0;
+ unsigned head, tail, avail;
+ struct aio_ring *ring = to_aio_ring(aio_ctx);
+
+ // continue until the available values are <= max
+ do {
+ head = ring->head;
+ mem_barrier();
+ tail = ring->tail;
+
+ if (tail >= head)
+ avail = tail - head;
+ else
+ avail = ring->nr - (head - tail);
+
Review comment:
if (avail == 0) {
return 0;
}
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis-native] franz1981 commented on a change in pull request #7: ARTEMIS-2800 Fixes around Linux Kernel Bug
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #7:
URL: https://github.com/apache/activemq-artemis-native/pull/7#discussion_r438769566
##########
File path: src/main/c/org_apache_activemq_artemis_nativo_jlibaio_LibaioContext.c
##########
@@ -129,9 +129,24 @@ static int user_io_getevents(io_context_t aio_ctx, unsigned int max,
struct io_event *events)
{
long i = 0;
- unsigned head;
+ unsigned head, tail, avail;
struct aio_ring *ring = to_aio_ring(aio_ctx);
+ // continue until the available values are <= max
+ do {
+ head = ring->head;
+ mem_barrier();
+ tail = ring->tail;
+
+ if (tail >= head)
+ avail = tail - head;
+ else
+ avail = ring->nr - (head - tail);
+
+ if (avail <= max)
+ break;
+ } while (1);
Review comment:
@clebertsuconic @JeffMoyer I still think it worths to check that tail is not just changed by is "right" too ie < max expected size
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis-native] JeffMoyer commented on a change in pull request #7: ARTEMIS-2800 Fixes around Linux Kernel Bug
Posted by GitBox <gi...@apache.org>.
JeffMoyer commented on a change in pull request #7:
URL: https://github.com/apache/activemq-artemis-native/pull/7#discussion_r438761134
##########
File path: src/main/c/org_apache_activemq_artemis_nativo_jlibaio_LibaioContext.c
##########
@@ -129,9 +129,24 @@ static int user_io_getevents(io_context_t aio_ctx, unsigned int max,
struct io_event *events)
{
long i = 0;
- unsigned head;
+ unsigned head, tail, avail;
struct aio_ring *ring = to_aio_ring(aio_ctx);
+ // continue until the available values are <= max
+ do {
+ head = ring->head;
+ mem_barrier();
+ tail = ring->tail;
+
+ if (tail >= head)
+ avail = tail - head;
+ else
+ avail = ring->nr - (head - tail);
+
+ if (avail <= max)
+ break;
+ } while (1);
Review comment:
Nothing should be changing the head pointer, so all you need to do is check for updates to tail. I think what I sent you was about as simple as you can make that loop. I also think this code change deserves a comment explaining the kernel bug that it is working around.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis-native] clebertsuconic commented on a change in pull request #7: ARTEMIS-2800 Fixes around Linux Kernel Bug
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #7:
URL: https://github.com/apache/activemq-artemis-native/pull/7#discussion_r439039650
##########
File path: src/main/c/org_apache_activemq_artemis_nativo_jlibaio_LibaioContext.c
##########
@@ -100,57 +125,59 @@ static inline struct aio_ring* to_aio_ring(io_context_t aio_ctx) {
return (struct aio_ring*) aio_ctx;
}
+static int user_io_getevents(io_context_t aio_ctx, unsigned int max,
+ struct io_event *events)
+{
+ long i = 0;
+ unsigned head, tail, avail;
+ struct aio_ring *ring = to_aio_ring(aio_ctx);
+
+ // continue until the available values are <= max
+ do {
+ head = ring->head;
+ mem_barrier();
+ tail = ring->tail;
+
+ if (tail >= head)
+ avail = tail - head;
+ else
+ avail = ring->nr - (head - tail);
+
Review comment:
@franz1981 : I am doing something much simpler than that..
I'm keeping the original loop I know it works
and I'm now making a syscall if available >= max
this was enough to go around the issue.
a lot less stress.. everything is working again.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis-native] franz1981 commented on a change in pull request #7: ARTEMIS-2800 Fixes around Linux Kernel Bug
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #7:
URL: https://github.com/apache/activemq-artemis-native/pull/7#discussion_r438769566
##########
File path: src/main/c/org_apache_activemq_artemis_nativo_jlibaio_LibaioContext.c
##########
@@ -129,9 +129,24 @@ static int user_io_getevents(io_context_t aio_ctx, unsigned int max,
struct io_event *events)
{
long i = 0;
- unsigned head;
+ unsigned head, tail, avail;
struct aio_ring *ring = to_aio_ring(aio_ctx);
+ // continue until the available values are <= max
+ do {
+ head = ring->head;
+ mem_barrier();
+ tail = ring->tail;
+
+ if (tail >= head)
+ avail = tail - head;
+ else
+ avail = ring->nr - (head - tail);
+
+ if (avail <= max)
+ break;
+ } while (1);
Review comment:
@clebertsuconic @JeffMoyer I still think it worths to check that tail is not just changed but is "right" too ie < max expected size and perf-wise a `pause` instruction in the tight loop while reading tail maybe
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis-native] franz1981 commented on a change in pull request #7: ARTEMIS-2800 Fixes around Linux Kernel Bug
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #7:
URL: https://github.com/apache/activemq-artemis-native/pull/7#discussion_r438769566
##########
File path: src/main/c/org_apache_activemq_artemis_nativo_jlibaio_LibaioContext.c
##########
@@ -129,9 +129,24 @@ static int user_io_getevents(io_context_t aio_ctx, unsigned int max,
struct io_event *events)
{
long i = 0;
- unsigned head;
+ unsigned head, tail, avail;
struct aio_ring *ring = to_aio_ring(aio_ctx);
+ // continue until the available values are <= max
+ do {
+ head = ring->head;
+ mem_barrier();
+ tail = ring->tail;
+
+ if (tail >= head)
+ avail = tail - head;
+ else
+ avail = ring->nr - (head - tail);
+
+ if (avail <= max)
+ break;
+ } while (1);
Review comment:
@clebertsuconic @JeffMoyer I still think it worths to check that tail is not just changed by is "right" too ie < max expected size and perf-wise a `pause` instruction in the tight loop while reading tail maybe
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis-native] JeffMoyer commented on a change in pull request #7: ARTEMIS-2800 Fixes around Linux Kernel Bug
Posted by GitBox <gi...@apache.org>.
JeffMoyer commented on a change in pull request #7:
URL: https://github.com/apache/activemq-artemis-native/pull/7#discussion_r438804048
##########
File path: src/main/c/org_apache_activemq_artemis_nativo_jlibaio_LibaioContext.c
##########
@@ -129,9 +129,24 @@ static int user_io_getevents(io_context_t aio_ctx, unsigned int max,
struct io_event *events)
{
long i = 0;
- unsigned head;
+ unsigned head, tail, avail;
struct aio_ring *ring = to_aio_ring(aio_ctx);
+ // continue until the available values are <= max
+ do {
+ head = ring->head;
+ mem_barrier();
+ tail = ring->tail;
+
+ if (tail >= head)
+ avail = tail - head;
+ else
+ avail = ring->nr - (head - tail);
+
+ if (avail <= max)
+ break;
+ } while (1);
Review comment:
> @JeffMoyer OT: do you think it would be better to replace the reaper loop using `%` with a `memcpy`? If yes I would send a different PR to address it too
I don't know what you mean. Maybe just post a diff of what you're proposing?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org