On Feb 21, 2014, at 6:35 PM, mtewoodbury@gmail.com wrote:
Note that this greatly improves network reliability.
How did you determine that? How could we confirm that?
- if ( (n = recvmsg(fd, &hdr, wsa->flags)) == -1 )
- n = recvmsg(fd, &hdr, wsa->flags);
- if (n == -1 && (errno == EINTR || errno == EAGAIN))
n = recvmsg(fd, &hdr, wsa->flags);- if (n == -1)
return -1;
What's with that blank line between the last "if" and its corresponding statement?
- if (ret >= 0)
- {
- if (ret == -1 && (errno == EINTR || errno == EAGAIN))
ret = sendmsg(fd, &hdr, wsa->flags); /* Try it again quickly */- if (ret >= 0) {
Why did you change the existing lines? You could have just added your two lines.
In general:
Is this actually the second patch of a two-patch series as indicated by the "[PATCH 2/2]" in the subject? I didn't receive the first patch.
Have you tested to see if just retrying on EINTR is sufficient to improve the reliability? EAGAIN is not typically a situation where an immediate retry helps.
The callers of WS2_recv() and WS2_send() already have tests for EINTR and EAGAIN. The strategy for dealing with those should probably exist in just one place. For my part, I think that all interruptible syscalls should be in a loop that immediately retries for EINTR, but EAGAIN should usually go back to some select(), poll(), kevent() or similar wait function.
-Ken
On Sun, Feb 23, 2014 at 7:23 AM, Ken Thomases ken@codeweavers.com wrote:
On Feb 21, 2014, at 6:35 PM, mtewoodbury@gmail.com wrote:
Note that this greatly improves network reliability.
How did you determine that? How could we confirm that?
At least in wine socket test adding the patch with some traces for EINTR results in no output, maybe it requires more stressful use.
Bruno
On 02/23/2014 10:10 AM, Bruno Jesus wrote:
On Sun, Feb 23, 2014 at 7:23 AM, Ken Thomases ken@codeweavers.com wrote:
On Feb 21, 2014, at 6:35 PM, mtewoodbury@gmail.com wrote:
Note that this greatly improves network reliability.
How did you determine that? How could we confirm that?
At least in wine socket test adding the patch with some traces for EINTR results in no output, maybe it requires more stressful use.
Bruno
Yes, the environment where I was seeing this problem is pretty stressful. I know it is multi-threaded with a lot of graphics going on it the other thread.
On Wed, Feb 26, 2014 at 3:39 PM, Max Woodbury mtewoodbury@gmail.com wrote:
At least in wine socket test adding the patch with some traces for EINTR results in no output, maybe it requires more stressful use.
Yes, the environment where I was seeing this problem is pretty stressful. I know it is multi-threaded with a lot of graphics going on it the other thread.
Could you share more details? Like what application requires this? The patch helps by increasing the download/upload speed or something like that?
Bruno
On 02/26/2014 03:36 PM, Bruno Jesus wrote:
On Wed, Feb 26, 2014 at 3:39 PM, Max Woodbury mtewoodbury@gmail.com wrote:
At least in wine socket test adding the patch with some traces for EINTR results in no output, maybe it requires more stressful use.
Yes, the environment where I was seeing this problem is pretty stressful. I know it is multi-threaded with a lot of graphics going on it the other thread.
Could you share more details? Like what application requires this? The patch helps by increasing the download/upload speed or something like that?
Bruno
Guild Wars 2. See other sub-thread.
On Wed, Feb 26, 2014 at 6:30 PM, Max Woodbury mtewoodbury@gmail.com wrote:
Guild Wars 2. See other sub-thread.
Can you share the URL to the other sub-thread, please?
On 02/26/2014 08:10 PM, Bruno Jesus wrote:
On Wed, Feb 26, 2014 at 6:30 PM, Max Woodbury mtewoodbury@gmail.com wrote:
Guild Wars 2. See other sub-thread.
Can you share the URL to the other sub-thread, please?
Ken pointed out that I had forgotten to CC the list. I hope that I've fixed that now...
max
Please explain in words of one syllable why this has been rejected?
On Wed, Feb 26, 2014 at 4:26 PM, Max Woodbury mtewoodbury@gmail.com wrote:
Please explain in words of one syllable why this has been rejected?
Probably for the reasons Ken already mentioned: 1) Strange blank line after "if (n == -1)" 2) Unnecessary style change to K&R style ("if (ret >= 0) {") 3) "[PATCH 2/2]" with no "[PATCH 1/2]" 4) No explanation for his comment that EAGAIN is probably not needed. EINTR might want this behavior, but EGAIN should not. 5) If you bring this code "down" to where you have then you need to remove it from the "higher" level: http://source.winehq.org/source/dlls/ws2_32/socket.c#L4494 http://source.winehq.org/source/dlls/ws2_32/socket.c#L6500 6) It should be in a loop
Does that help?
Best, Erich
On 02/26/2014 06:36 PM, Erich E. Hoover wrote:
On Wed, Feb 26, 2014 at 4:26 PM, Max Woodbury mtewoodbury@gmail.com wrote:
Please explain in words of one syllable why this has been rejected?
Probably for the reasons Ken already mentioned:
- Strange blank line after "if (n == -1)"
- Unnecessary style change to K&R style ("if (ret >= 0) {")
- "[PATCH 2/2]" with no "[PATCH 1/2]"
- No explanation for his comment that EAGAIN is probably not needed.
EINTR might want this behavior, but EGAIN should not. 5) If you bring this code "down" to where you have then you need to remove it from the "higher" level: http://source.winehq.org/source/dlls/ws2_32/socket.c#L4494 http://source.winehq.org/source/dlls/ws2_32/socket.c#L6500 6) It should be in a loop
Does that help?
Best, Erich
So it's mainly on style:
- I've been reading other peoples code for 50 years and that aspect of K&R is much easier to handle in my opinion. Same with the extra blank line. Other parts of wine use the style and are much easier to read as a result.
- And the miscount can't simply be ignored?
If you read kernel code, you'll find that just returning to application level is sometimes enough to free the missing resource. That happens often enough that a single quick retry on EAGAIN is warranted.
Bringing the relevant part of the retry code down and applying it to only those parts where is has a strong likelihood of doing the most good is exactly what the patch does. If one retry is not enough, then higher level strategy is needed so no changes are called for on the higher level. A loop at this level is _not_ a good idea since there would have to be something to break the loop if it runs too long.
In other words, this fiddle is based on several decades of experience with kernel and bottom level application debugging experience. At least look at it and test it before rejecting it on superficial grounds.
max
On Wed, Feb 26, 2014 at 6:10 PM, Max Woodbury mtewoodbury@gmail.com wrote:
... So it's mainly on style:
I've been reading other peoples code for 50 years and that aspect of K&R is much easier to handle in my opinion. Same with the extra blank line. Other parts of wine use the style and are much easier to read as a result.
And the miscount can't simply be ignored?
Very little of Wine uses a blank line after an if, and I've never seen it with a single line statement. That doesn't really matter though, the rest of the function (and really the whole file) is in Allman style and our current guidelines require matching the surrounding style. (Note: I am not in charge of this, so debating me on the topic is pointless)
If you read kernel code, you'll find that just returning to application level is sometimes enough to free the missing resource. That happens often enough that a single quick retry on EAGAIN is warranted.
I'm not familiar enough with what you're trying to do, but if the buffer is full and the kernel throws back EAGAIN then it would seem that you're not in such a time crunch that waiting for the higher level to handle it is going to be a problem. I could be wrong though, I don't do a lot of high-throughput socket programming.
Bringing the relevant part of the retry code down and applying it to only those parts where is has a strong likelihood of doing the most good is exactly what the patch does. If one retry is not enough, then higher level strategy is needed so no changes are called for on the higher level. A loop at this level is _not_ a good idea since there would have to be something to break the loop if it runs too long.
I believe that Ken was suggesting that there are ways to do this such that the higher level can handle EGAIN and the lower level EINTR. In my, admittedly limited, experience with this issue his suggestion seems reasonable, but I could be wrong. However, I am sure that if you put together a convincing test case that it will make getting the patch accepted a lot easier (though not guaranteed).
In other words, this fiddle is based on several decades of experience with kernel and bottom level application debugging experience. At least look at it and test it before rejecting it on superficial grounds.
Please do not be confused, I am not in charge of acceptance/rejection. I have, however, been involved in Wine long enough to know that if you don't address the issues mentioned on this list (either with an explanation, the addition of new tests, or changing the code to handle the case mentioned) then AJ will usually reject the patch.
Best, Erich
On 02/26/2014 08:32 PM, Erich E. Hoover wrote:
On Wed, Feb 26, 2014 at 6:10 PM, Max Woodbury mtewoodbury@gmail.com wrote:
... So it's mainly on style:
I've been reading other peoples code for 50 years and that aspect of K&R is much easier to handle in my opinion. Same with the extra blank line. Other parts of wine use the style and are much easier to read as a result.
And the miscount can't simply be ignored?
Very little of Wine uses a blank line after an if, and I've never seen it with a single line statement. That doesn't really matter though, the rest of the function (and really the whole file) is in Allman style and our current guidelines require matching the surrounding style. (Note: I am not in charge of this, so debating me on the topic is pointless)
Bluntly Allman style sucks. It is just plain hard to read. Also, the blank line is associated with the 'return' and is often used.
If you read kernel code, you'll find that just returning to application level is sometimes enough to free the missing resource. That happens often enough that a single quick retry on EAGAIN is warranted.
I'm not familiar enough with what you're trying to do, but if the buffer is full and the kernel throws back EAGAIN then it would seem that you're not in such a time crunch that waiting for the higher level to handle it is going to be a problem. I could be wrong though, I don't do a lot of high-throughput socket programming.
Not a buffers full problem. Internal locks that get released from a thread switch and things like that. Worth ONE retry to see if they clear. If its still stuck on the second attempt, punt. And it's not me, it's the game...
Bringing the relevant part of the retry code down and applying it to only those parts where is has a strong likelihood of doing the most good is exactly what the patch does. If one retry is not enough, then higher level strategy is needed so no changes are called for on the higher level. A loop at this level is _not_ a good idea since there would have to be something to break the loop if it runs too long.
I believe that Ken was suggesting that there are ways to do this such that the higher level can handle EGAIN and the lower level EINTR. In my, admittedly limited, experience with this issue his suggestion seems reasonable, but I could be wrong. However, I am sure that if you put together a convincing test case that it will make getting the patch accepted a lot easier (though not guaranteed).
TEST CASE?!? Not! Multi-thread with graphics throwing interrupts left and right! Disk interrupts. Real internet delays. Instead, just watch the game with the problem for a day or two. Better read a few good Unix programming text. Ask the impossible why dont'cha.
In other words, this fiddle is based on several decades of experience with kernel and bottom level application debugging experience. At least look at it and test it before rejecting it on superficial grounds.
Please do not be confused, I am not in charge of acceptance/rejection. I have, however, been involved in Wine long enough to know that if you don't address the issues mentioned on this list (either with an explanation, the addition of new tests, or changing the code to handle the case mentioned) then AJ will usually reject the patch.
Yep. If it were possible, I'd do just that, but it's not... On the other hand, forgetting a CC is bad. (Fixed. Want to compare years on the list? I've a few...)
Best, Erich
I have been looking at the use of the wined3d_mutex_lock and wined3d_mutex_unlock functions in the drawing code. I strongly suspect that it is being badly over-used.
In particular, user call back functions are being called in several places with this mutex held. If the callback starts another thread to do something graphical where the thread needs the mutex, and then waits for the thread to finish, both threads will hang until the mutex times out. There are also places where a function that takes out the mutex is called with the mutex already held.
Switching to another terminal window and back again sometimes breaks these deadlocks. It used to be that I had to do this several times doing a 'make test', but it just takes a long time now. (Yes, years ago I left the test running for several hours when I went out for a few hours and it was still hung when I got back. That does not happen now,)
This shows up running Guild Wars 2. I have a CPU monitor running while I play and I can see the game chomping on both cores, except that the game will sometimes hang for 30-45 seconds and then resume.
I am in the process of annotating the drawing routines use of the mutex, and there is simply a lot of code to go through. Could someone with a good understanding of the drawing part of wine comment please.
In particular, are there places where the lock-like action of the reference counts can be used in place of taking out the mutex?
max
-
Bruno Jesus -
Erich E. Hoover -
Ken Thomases -
Max Woodbury