May 2014 - Wine-Devel - list.winehq.org

Re: ntdll: Use lockfree implementation for get_cached_fd.

by Sebastian Lackner

Hi Ken, thanks for taking a look at it and for the detailed answer. [...] > > Thanks for working on this. Interesting stuff. > > I think there are still some issues. > > In order to ensure a consistent view of the ordering of memory operations, memory barriers have to be used in both the writer and the reader. The writer issues a write barrier after it has written and the reader issues a read barrier before it reads. > > The appropriate barriers are implicit in interlocked_xchg_add() for incrementing fd_cache_epoch, but the first read of it in get_cached_fd() has no read barrier before it. I think this is benign, though, because I think this can only result in the code thinking the epoch changed when it hasn't. It may retry unnecessarily. get_cached_fd() will only have to run multiple iterations if a second thread updated the cache in the meantime. As it is more likely to have a cache hit I would assume that its okay, when fd_cache_epoch returns a cached value - but of course this depends on the exact program you're running. > More problematic is the writing of the fd and its attributes. There are barriers implicit in the interlocked_xchg() which clears the fd, but there's no write barrier after the final write to it. There's no read barrier before the first read of them. > > You mention the order of writing, attributes first then fd, as being important. But there's no barrier in between the two operations, so either the compiler or the CPU could reorder either the writes or the reads, thwarting your design. The compiler can also reorder the reading of the fd_cache_epoch with respect to the reading of the fd or attributes. I think you can get this as the effective sequence: > > thread1 / get_cached_fd / read fd, get old fd > thread2 / add_fd_to_cache / clear fd > thread2 / add_fd_to_cache / increment epoch > thread2 / add_fd_to_cache / set fd > thread1 / get_cached_fd / read epoch > thread1 / get_cached_fd / check if epoch has changed, conclude it hasn't > thread1 / get_cached_fd / return old fd > > I think it's sufficient to have a write barrier after writing the fd and then a read barrier just before reading it. I _think_ that this obviates the need for the barrier at the bottom of the do-while loop. That is, you can just move that barrier up a line. I'm not 100% confident of my analysis of that. (Well, frankly, I'm never 100% certain of my analysis of this sort of code.) I have to agree that this is a bit tricky - but I don't think that moving the line one up would completely solve it. The compiler could still reorder (read epoch) and (read attributes), which could result in using wrong attributes. Moreover it there is no barrier at the original place it could still reorder (read epoch 2) and (read fd). To be completely sure it would be necessary to add compiler memory barriers everywhere inbetween ... which would make the code look very ugly... xD Nevertheless, the example above couldn't happen that way: As add_fd_to_cache() is only used to set the file descriptor, not to replace it, the old fd would be (-1) in this case. ;) > > In the email thread regarding Daniel Horn's patch, I suggested an approach using interlocked_cmpxchg() to check if the fd had changed after reading the attributes. Was there a reason you rejected that approach? You mentioned an approach you experimented with using interlocked_cmpxchg64(). What was that about? Were you treating the fd and attributes combined as a single 64-bit value and then checking them both? Was that safer than my suggestion for some reason? > > If the only advantage is a slight performance improvement (especially in light of the huge difference that either approach has to the status quo, judging by your chart), then I'd argue for the simpler approach that's more easily analyzed for correctness. > My approach was indeed treating both the fd and the attributes as a single 64-bit number. This way we can read/write both fd and attributes at the same time, which definitely solves all multithreading issues. I still think that this is probably the best way to do it - with the only disadvantage that it doesn't work on PowerPC. :/ I have attached a patch showing this approach to the mail. Using interlocked_cmpxchg() to verify only the fd doesn't seem to be safe to me: thread1 / get_cached_fd / read fd and attributes thread2 / release old handle thread2 / allocate new object with the same handle (unlikely, but ...) thread1 / get_cached_fd / compares fd one more time -> looks like everything okay This basically means that applications could assume that wrong attributes are associated with a specific file handle. I know that this is very unlikely (especially because the handles contain the generation in the upper 16bit), but it is still possible... Or is there any reason why we can be sure that this doesn't happen? > > If the above issue is addressed, that leaves one remaining problem: get_cached_fd() can encounter a false negative: > > thread1 / add_fd_to_cache / set fd to -1 > thread1 / add_fd_to_cache / increment epoch > thread2 / get_cached_fd / read epoch > thread2 / get_cached_fd / read attributes > thread2 / get_cached_fd / read fd, gets -1 > thread2 / get_cached_fd / check if epoch has changed, conclude it hasn't > thread1 / add_fd_to_cache / set attributes > thread1 / add_fd_to_cache / set fd > thread2 / get_cached_fd / return -1 > thread2 / server_get_unix_fd / see get_cached_fd as having failed since it returns -1 > thread2 / server_get_unix_fd / do get_handle_fd server request > thread2 / server_get_unix_fd / call add_fd_to_cache to cache the new fd > > I think you want to try get_cached_fd() again, inside the critical section, if the first call fails. This should be fixed in try 2. >> > * Replace interlocked_xchg() with an assert(). The original code looks >> > like its valid that the fd is nonzero at this point, but in fact this >> > would cause releasing a file descriptor which is still in use. > I'll leave it to others to comment on that. > >> > Replacing with an assert() also saves a couple of CPU cycles. > I understand that this is an attempt to improve performance, but I don't think it's necessary to squeeze every last CPU cycle out of this code. This was not done to improve the performance, but instead to detect errors more easily. If the fd is already set it always means that something is going wrong. The current code hides these kind of errors by just closing the file handle and replacing it - that was also the reason why I assumed one call to get_cached_fd() is sufficient, as it looks like the code handles already the case if fd is set - but in fact it will close a handle which is still in use... > > -Ken > What do you think about the alternative (slightly slower, but much easier to verify) solution attached to this mail? Regards, Sebastian

May 31, 2014

Re: ntdll: Use lockfree implementation for get_cached_fd. (try 2)

by Ken Thomases

On May 31, 2014, at 12:05 PM, Sebastian Lackner wrote: > Changes in try 2: > > * Always try to access the fd cache two times - this ensures that we > don't release a file descriptor when its still in use by a second thread. Heh. Well, you can ignore that part of my reply to your previous try. :) > * Replace interlocked_xchg() with an assert(). The original code looks > like its valid that the fd is nonzero at this point, but in fact this > would cause releasing a file descriptor which is still in use. I'll leave it to others to comment on that. > Replacing with an assert() also saves a couple of CPU cycles. I understand that this is an attempt to improve performance, but I don't think it's necessary to squeeze every last CPU cycle out of this code. -Ken

May 31, 2014

Re: ntdll: Use lockfree implementation for get_cached_fd.

by Ken Thomases

On May 31, 2014, at 10:28 AM, Sebastian Lackner wrote: > Explanation: The basic idea of this patch is that we have a global > counter 'fd_cache_epoch' which is incremented, whenever the file > descriptor cache is updated. get_cached_fd() first reads the counter, > then the fd entry, and afterwards the counter again - if the counter has > changed we know that a second thread has maybe modified the content, and > have to read it again. The memory barrier is there to ensure that > compilers don't optimize away the two reads of 'fd_cache_epoch'. Please > note that: > > * the order of the write instructions when updating the content is > important - this way we ensure that threads only see either (0, *) = not > set, or a valid entry. > > * I assume that reading/writing up to the pointer size can be done as an > atomic operation (threads either see the old or new value, but no > partial updates). This assumption is also used in a lot other parts of > wine, for example here: > http://source.winehq.org/source/dlls/ntdll/sync.c#L1295 > [ It would probably be more safe to introduce special > interlocked_get/interlocked_set macros/functions for that, but not sure > what other people think about that... ] Thanks for working on this. Interesting stuff. I think there are still some issues. In order to ensure a consistent view of the ordering of memory operations, memory barriers have to be used in both the writer and the reader. The writer issues a write barrier after it has written and the reader issues a read barrier before it reads. The appropriate barriers are implicit in interlocked_xchg_add() for incrementing fd_cache_epoch, but the first read of it in get_cached_fd() has no read barrier before it. I think this is benign, though, because I think this can only result in the code thinking the epoch changed when it hasn't. It may retry unnecessarily. More problematic is the writing of the fd and its attributes. There are barriers implicit in the interlocked_xchg() which clears the fd, but there's no write barrier after the final write to it. There's no read barrier before the first read of them. You mention the order of writing, attributes first then fd, as being important. But there's no barrier in between the two operations, so either the compiler or the CPU could reorder either the writes or the reads, thwarting your design. The compiler can also reorder the reading of the fd_cache_epoch with respect to the reading of the fd or attributes. I think you can get this as the effective sequence: thread1 / get_cached_fd / read fd, get old fd thread2 / add_fd_to_cache / clear fd thread2 / add_fd_to_cache / increment epoch thread2 / add_fd_to_cache / set fd thread1 / get_cached_fd / read epoch thread1 / get_cached_fd / check if epoch has changed, conclude it hasn't thread1 / get_cached_fd / return old fd I think it's sufficient to have a write barrier after writing the fd and then a read barrier just before reading it. I _think_ that this obviates the need for the barrier at the bottom of the do-while loop. That is, you can just move that barrier up a line. I'm not 100% confident of my analysis of that. (Well, frankly, I'm never 100% certain of my analysis of this sort of code.) In the email thread regarding Daniel Horn's patch, I suggested an approach using interlocked_cmpxchg() to check if the fd had changed after reading the attributes. Was there a reason you rejected that approach? You mentioned an approach you experimented with using interlocked_cmpxchg64(). What was that about? Were you treating the fd and attributes combined as a single 64-bit value and then checking them both? Was that safer than my suggestion for some reason? If the only advantage is a slight performance improvement (especially in light of the huge difference that either approach has to the status quo, judging by your chart), then I'd argue for the simpler approach that's more easily analyzed for correctness. If the above issue is addressed, that leaves one remaining problem: get_cached_fd() can encounter a false negative: thread1 / add_fd_to_cache / set fd to -1 thread1 / add_fd_to_cache / increment epoch thread2 / get_cached_fd / read epoch thread2 / get_cached_fd / read attributes thread2 / get_cached_fd / read fd, gets -1 thread2 / get_cached_fd / check if epoch has changed, conclude it hasn't thread1 / add_fd_to_cache / set attributes thread1 / add_fd_to_cache / set fd thread2 / get_cached_fd / return -1 thread2 / server_get_unix_fd / see get_cached_fd as having failed since it returns -1 thread2 / server_get_unix_fd / do get_handle_fd server request thread2 / server_get_unix_fd / call add_fd_to_cache to cache the new fd I think you want to try get_cached_fd() again, inside the critical section, if the first call fails. -Ken

May 31, 2014

Re: ntdll/tests: Fix exception test failures on x86_64.

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at https://testbot.winehq.org/JobDetails.pl?Key=7354 Your paranoid android. === w2000pro (32 bit exception) === exception.c:406: Test failed: 42: Wrong exception address 00950000/00950001 exception: unhandled exception 80000004 at 00950000 exception.c:406: Test failed: 42: Wrong exception address 7C5C20B0/00950001 === wxppro (32 bit exception) === exception.c:406: Test failed: 42: Wrong exception address 00A30000/00A30001 exception: unhandled exception 80000004 at 00A30000 exception.c:406: Test failed: 42: Wrong exception address 7C80DF3C/00A30001 === wvistau64 (32 bit exception) === exception.c:406: Test failed: 42: Wrong exception address 003D0000/003D0001 exception: unhandled exception 80000004 at 003D0000 exception.c:406: Test failed: 42: Wrong exception address 7751ED97/003D0001 === w2008s64 (32 bit exception) === exception.c:406: Test failed: 42: Wrong exception address 003C0000/003C0001 exception: unhandled exception 80000004 at 003C0000 exception.c:406: Test failed: 42: Wrong exception address 77E5ED97/003C0001 === w7u (32 bit exception) === exception.c:406: Test failed: 42: Wrong exception address 003C0000/003C0001 exception: unhandled exception 80000004 at 003C0000 exception.c:406: Test failed: 42: Wrong exception address 77CDE34F/003C0001 === w8 (32 bit exception) === exception.c:406: Test failed: 42: Wrong exception address 00290000/00290001 exception: unhandled exception 80000004 at 00290000 exception.c:406: Test failed: 42: Wrong exception address 77174D17/00290001

May 31, 2014

Re: dmscript/tests: Test some IPersistStream methods of DMScriptTrack.

by Michael Stefaniuc

On 05/30/2014 11:37 PM, Marvin wrote: > Hi, > > While running your changed tests on Windows, I think I found new failures. > Being a bot and all I'm not very good at pattern recognition, so I might be > wrong, but could you please double-check? > Full results can be found at > https://testbot.winehq.org/JobDetails.pl?Key=7339 > > Your paranoid android. > > > === build (build) === > Patch failed to apply Correct as it depends on a previously sent patch that is not committed yet. Here are the "all green" results from the cumulative patch: https://testbot.winehq.org/JobDetails.pl?Key=7334 bye michael

May 31, 2014

Re: dmscript/tests: Test some IPersistStream methods of DMScriptTrack.

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at https://testbot.winehq.org/JobDetails.pl?Key=7339 Your paranoid android. === build (build) === Patch failed to apply

May 30, 2014

Re: wined3d: Recognize cards that expose ARB_shader_texture_lod as DX10 capable even if they don't support EXT_gpu_shader4 (try 3)

by Stefan Dösinger

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Am 2014-05-30 19:51, schrieb Andrei Slăvoiu: > I find it strange that there are 2 completely different code paths > for determining card capabilities. So I was thinking to share the > code between d3d_level_from_gl_info and shader_glsl_get_caps. Yes, the double implementation of those similar functionalities is unfortunate and fixing this is a good idea. I'd say migrating to MESA_query_renderer supersedes this work and is the better long-term goal. > Since such shared code needs to be able to differentiate between > d3d 9 with SM 2 and d3d 9 with SM 3 I was wondering if it would be > a good idea to expose SM info to the current callers of > d3d_level_from_gl_info to pick Radeon 9500 for SM2 or Radeon X1600 > for SM3. I think just looking at the vertex shader version should work, as I suggested in an earlier mail. I don't think this needs a DirectX version at all. Also, don't call shader_glsl_get_caps directly - call the get_caps method exported by the selected shader backend. There are two caveats: To separate version 7 from version 6 you'll need information from the fixed function fragment pipeline. This shouldn't be hard to do. The other thing to keep in mind is that by calling the shader backend, the card may present itself with a different PCI ID when switching between ARB and GLSL shaders. Arguably this is even the correct thing to do, as the ARB shader backend supports only Shader Model 2 on most cards. Reporting a Radeon 9500 instead of Radeon HD 8000 when the user selects ARB shaders sounds like a good idea in general. It may make debugging shader backend differences harder though. -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJTiMfCAAoJEN0/YqbEcdMwcnMP/iWFXx0L/765OI73ZrNtOGfK V7eeo1e/NwcNrqjmyux2AiaGAEpeDIqIPr3hUlvJtOSe7Vm6LnY0XNUtGGV0Hdlh XEIKm/Obl6GrUbjPWCOnWIRmvPRj/SS3r48pG/shYy81dEUBMLhRrXmVNqvacMdR 7xP5NNEBhJI8uI8EYgTlcgmyZ6Iv8an+uu5afJTsnrvBZBNJzM1Kh6RhHjb5r7Qq REB6HzO3ryu3zMv3wSN6iNMJo+Tod8edO+IEpDfblPD+GiBSQScXX7UWPdPpnpUw QVdxA4crGBlu1/+a3sbI1M4Q66EGnlATWT7NwKI3ubj6VEjMqFkSC3QZzY3wLnX3 MxR8MJWww5IgwCSS5VTyhLqsmuFSl5ch0P1Iq2P35M3Tq85g1i1av/B8x5PuxufT J6lmX1ohcyMsEAR6qtb84OCpofeddUQgxVlAjpVuv3i4lzLZjc5WgW46t3pYxM3f /lgxMF8iA7D+TwuUZ2RC+wJM4qAcB7jpwMucFgWuIk4AEBlDEgJR5q7k71HaCpFK wp2fIX9jLU9UJb4d9wxcULhiA6rg+SgS2eFM7pSrKsvyhqQMcGhQDV1xvKRt71SW r3tY6Ov/7VSAdfT5MLfYoGmHqMuK9tSaekmqGRKw/yeMUECfL52DfFPbQl/7uQik OsKveCiQsOtsSN5QUiao =lXuc -----END PGP SIGNATURE-----

May 30, 2014

Re: wined3d: Recognize cards that expose ARB_shader_texture_lod as DX10 capable even if they don't support EXT_gpu_shader4 (try 3)

by Andrei Slăvoiu

În ziua de Vin 30 Mai 2014, la 09:36:01, Stefan Dösinger a scris: > Am 2014-05-29 20:53, schrieb Andrei Slăvoiu: > > After reading the message again I think I finally understood what > > you meant. I'll send a try 4 that adds back glsl_version check (try > > 2 was missing a brace). Do you think returning 93 (meaning d3d > > level 9_3 or d3d 9 with SM 3) would be too ugly? So wine can choose > > X1300 as card if ARB_shader_texture_lod is exposed but not GLSL > > 1.30. > > What's wrong with 9 in that case? I find it strange that there are 2 completely different code paths for determining card capabilities. So I was thinking to share the code between d3d_level_from_gl_info and shader_glsl_get_caps. Since such shared code needs to be able to differentiate between d3d 9 with SM 2 and d3d 9 with SM 3 I was wondering if it would be a good idea to expose SM info to the current callers of d3d_level_from_gl_info to pick Radeon 9500 for SM2 or Radeon X1600 for SM3. Alternatively I could leave it return 9 as it does now and add an extra parameter that will return the shader model. Probably that would be cleaner.

May 30, 2014

Re: Video memory size detection in D3D

by Stefan Dösinger

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Am 2014-05-30 15:25, schrieb Ruslan Kabatsayev: > Hmm, this is an interesting point. But, for > GLX_MESA_query_renderer, which appeared relatively recently, one > can't say it's experimental or cannot return the total memory. Can > its use be accepted by Wine? Maybe at some point Nvidia/AMD will > implement it too (looks unlikely though), but at least for > Mesa-powered drivers it would work. Yes, I see no reason why we can't use GLX_MESA_query_renderer. We still need the fallback path for drivers that do not support it though. It's important to note that wined3d itself cannot use it. Wined3d uses WGL, not GLX. winex11.drv has to use GLX_MESA_query_renderer, and it can export its functionality to wined3d in the form of a WGL extension that is similar or equal to GLX_MESA_query_renderer. winemac.drv can export the same extension. There are OSX functions to query the PCI IDs and video memory size. (I forgot the name though). As far as I know the NV-CONTROL X11 (not GLX) extension exposes the same information, but doesn't have the issue that it's specification isn't finalized. Winex11.drv could in theory use it if GLX_MESA_query_renderer is not available. -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJTiIk6AAoJEN0/YqbEcdMw+GwP/2ia45TTEJPLthGMoReA13iX 8M2NplPwR+SHaPSSR6U9jUre80Q5p8amFh8gM896Rw8uoq0qB6Y5c8VtMlkPdYgp WzfKqjxJJC4Jt8csTT/m245f82lf78qLsWRfZTMYwiX5921NmCqC4SEbs7FnYGp9 FpIdDEmTQ1NLS9C5bdCBUtQbY9jBMekyM9x8+5acLyhDgjw+GwDIRHPgbKokNix3 c0RdYoXFcRl3aUKGTR7zM7CHjS9e56Oi6HApfaxBr0ElMh7uM40pjug/pHMz6hqE dBWUuViOwnt6vJqUBGcSTc6nmEAFVvW+Gnpxo3wKxjsByUQ9BiviGGOkf1QOm7HI /RCRAD2UL888ujFOglRyA7/Ss72qFKXTEy6utuX1qne2gJrjbnfSLD/Hb2UZV7f7 WrDJ9Mmma3xh+xMtuqL/3HHRUyEc3z+4TamOwFwlYeNMoUH8DJ2dWPRcjD5VfuG8 sb+MTtkONNnWjH5R28TVhJFKqNjN9CL2rwfShw15CbG2I/HJ+OWxK1lSREbne2Er WNjtPOh03e7I+KTm6bB5H6xr+aLO+DjLvKmmaqek+Ap+8ebqfLBGh9EtoHBxRfAs wtl8ImBXxo/M0O6sIo9N2OG6CefQ5bcpsHABjj0noKEF2Zqv8L+2kPwadr2DasLf 9rLNpWS58G3lN3BjtxXQ =ifL5 -----END PGP SIGNATURE-----

May 30, 2014

Re: Video memory size detection in D3D

by Ruslan Kabatsayev

On Fri, May 30, 2014 at 5:06 PM, Bruno Jesus <00cpxxx(a)gmail.com> wrote: > On Fri, May 30, 2014 at 5:36 AM, Stefan Dösinger > <stefandoesinger(a)gmail.com> wrote: >> -----BEGIN PGP SIGNED MESSAGE----- >> Hash: SHA1 >> >> Am 2014-05-30 08:01, schrieb Ruslan Kabatsayev: >>> It'd be good if Wine could utilize these. Is there any work >>> towards this already? >> See the discussion between Andrei Slăvoiu and myself on this list. I >> don't know if Andrei will decide to have a look at the extension, it's >> his choice. Other than that I don't know of any efforts in this area. >> You're welcome to chime in :-) . > > Reminds me of some old discussions like: > http://www.winehq.org/pipermail/wine-devel/2012-April/094994.html > (last question/answer). Hmm, this is an interesting point. But, for GLX_MESA_query_renderer, which appeared relatively recently, one can't say it's experimental or cannot return the total memory. Can its use be accepted by Wine? Maybe at some point Nvidia/AMD will implement it too (looks unlikely though), but at least for Mesa-powered drivers it would work.

May 30, 2014