| Summary: | [WK2][Qt] Multiple problems with MemoryMappedPool | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Kimmo Kinnunen <kimmo.t.kinnunen> | ||||||||||
| Component: | WebKit2 | Assignee: | Kimmo Kinnunen <kimmo.t.kinnunen> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | aroben, buildbot, commit-queue, hausmann, kbalazs, kenneth, kling, laszlo.gombos, ossy, s.mathur, webkit.review.bot, zalan | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | PC | ||||||||||||
| OS: | Linux | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Kimmo Kinnunen
2011-01-05 22:58:35 PST
> 1) Pass file descriptors to open but deleted files via IPC. > > Use this protocol to create the mmapped files: > -- process 0 > 1) fd = open temp file > 2) resize(fd) > 3) mmap(fd) > 4) delete(fd) > 5) send fd via local domain socket or fifo > -- process 1 > 1) handle ipc messaeg > 2) fd = decode fd > 3) mmap(fd) > 4) use fd. This is pretty similar than what we do right now. The only benefit with the procedure you mentioned that we can delete the file at the sender side. This is an argument to use OS level file handling and send the file descriptor. > This needs the ConnectionQt.cpp to be modified to use unix socket api or FIFO api to pass all the data. The Qt abstraction of QLocalServer and QLocalSocket cannot express sending FDs. However, I don't see this as a bad thing -- it should just remove unnecessary stuff. Sending the descriptor is just sending an integer. This is obviously possible with the current connection infrastructure. > 2) Race condition in sharing the buffers with variable isFree > > To have concurrently valid implementation, the access to the page variables should be controlled with cross-process semaphore. The isFree implements just a corner-case optimization for avoiding to call mmap and most likely this is not optimization at all. Thus usage should be removed. > > After sending a memory area to other process, contract should be that the memory region cannot be used at all. > > One scheme to achieve what isFree is doing, see below: > > have two variables: > > QList<MappedMemory*> m_usableRegions > > QList<MappedMemory*> m_sentRegions > > usableRegions gets appended when a process receives a memory region, uses it and then marks it disposed. These are the pages that can be sent back to the other process. > > sentRegions is the list that optimizes mmap: it stores list of regions that have been mapped to this process memory at some point. The memory chunk is used by both process. The role of the isFree member is that it is seen in both process at the same time. Having a list in both processes is not enough here. About the race condition: could you give an example when smg wrong happen? As I see the invariant here is only one of the processes is allowed to write it at a time. The only race condition is when the UI process set it to free and the same time the web process reading ifFree but that is a safe situation in all possible outcome. > 4) Implementation does not check syscall failures > > The implementation should check status of file opens, mmap operations etc. Yes, maybe it should. I do not see any rational error handling however, but at least we should do a CRASH() to avoid indeterministic behavior. (In reply to comment #1) > > This needs the ConnectionQt.cpp to be modified to use unix socket api or FIFO api to pass all the data. The Qt abstraction of QLocalServer and QLocalSocket cannot express sending FDs. However, I don't see this as a bad thing -- it should just remove unnecessary stuff. > > Sending the descriptor is just sending an integer. This is obviously possible with the current connection infrastructure. I'm no expert, but I don't think it is. You need to send the message by using specific API. In other words: the file descriptor table is not shared after fork. It's only copied during fork. AFAIK. "File Descriptor Passing" in this url: http://www.wsinnovations.com/softeng/articles/uds.html > The memory chunk is used by both process. The role of the isFree member is that it is seen in both process at the same time. > Having a list in both processes is not enough here. About the race condition: could you give an example when smg wrong happen? 0) Web process has passed 1 update chunk to ui process. Ui process has processed that update chunk and marked the underlying MappedMemory free. Both processes have this MappedMemory in the m_pool. 1) Both processes run MappedMemoryPool::mapMemory(size_t size) 2) Both processes run MappedMemoryPool::isFree() at the same time for the same pool entry. Both process get result 'true'. 3) Both processes run MappedMemoryPool::markUsed() at the same time I don't see any "atomic compare and increment" operator used. I don't know if they even work cross-process. > As I see the invariant here is only one of the processes is allowed to write it at a time. The only race condition is when the UI process set it > to free and the same time the web process reading ifFree but that is a safe situation in all possible outcome. > > > > 4) Implementation does not check syscall failures > > > > The implementation should check status of file opens, mmap operations etc. > > Yes, maybe it should. I do not see any rational error handling however, but at least we should do a CRASH() to avoid indeterministic behavior. No. It should fail the memory allocation. This should fail the update chunk allocation. This should result in empty result in paint. Beats crashing, is easy and somewhat well-defined. > You need to send the message by using specific API. > In other words: the file descriptor table is not shared after fork. It's only copied during fork. AFAIK. > > "File Descriptor Passing" in this url: > http://www.wsinnovations.com/softeng/articles/uds.html > You are right. > 0) Web process has passed 1 update chunk to ui process. Ui process has processed that update chunk and marked the underlying MappedMemory free. Both processes have this MappedMemory in the m_pool. > 1) Both processes run MappedMemoryPool::mapMemory(size_t size) The UI process never calls that. > 2) Both processes run MappedMemoryPool::isFree() at the same time for the same pool entry. Both process get result 'true'. > > 3) Both processes run MappedMemoryPool::markUsed() at the same time Ditto. > No. It should fail the memory allocation. This should fail the update chunk allocation. This should result in empty result in paint. Beats crashing, is easy and somewhat well-defined. Yep, sounds better. Created attachment 79274 [details]
First try removing Cleanup Handler
Comment on attachment 79274 [details] First try removing Cleanup Handler View in context: https://bugs.webkit.org/attachment.cgi?id=79274&action=review Lets go over this at your computer :-) First some style comments. > Source/WebKit2/ChangeLog:9 > + To avoid the need change following: I dont understand the above sentence. "For this we need the following?" > Source/WebKit2/ChangeLog:14 > + passing them via filesystem. *the* file system > Source/WebKit2/Platform/CoreIPC/qt/ConnectionQt.cpp:47 > +static const size_t attachmentMaxNum = 255; I would use Count or Number. Maybe attachmentMaxAmount is even better. > Source/WebKit2/Platform/CoreIPC/qt/ConnectionQt.cpp:145 > + > + remove one of the newlines > Source/WebKit2/Platform/CoreIPC/qt/ConnectionQt.cpp:152 > + struct cmsghdr *controlMessage = CMSG_FIRSTHDR(&message); * alignment > Source/WebKit2/Platform/CoreIPC/qt/ConnectionQt.cpp:156 > + > + remove one newline > Source/WebKit2/Platform/CoreIPC/qt/ConnectionQt.cpp:295 > + > + double newline > Source/WebKit2/Platform/CoreIPC/qt/ConnectionQt.cpp:303 > + struct cmsghdr *cmsg = CMSG_FIRSTHDR(&message); * alignment > common.pri:11 > +!isEmpty($$(SBOX_DPKG_INST_ARCH)):exists(/usr/bin/ld.gold) { > message(Using gold linker) > + QMAKE_CFLAGS+=-fuse-ld=gold > + QMAKE_CXXFLAGS+=-fuse-ld=gold > QMAKE_LFLAGS+=-fuse-ld=gold This doesnt seem related. Comment on attachment 79274 [details] First try removing Cleanup Handler View in context: https://bugs.webkit.org/attachment.cgi?id=79274&action=review Generally this patch moves to the direction to implement our API on each platform with the platform's system level insfrastructure. This is differ from how QtWebKit has been developed so far. I am not saying I'm against this but such a philosophic change should be debated with the wider community before moving into that direction. It's seems obvious to me that the problems with the CleanupHandler is not enough as an argument to this change. If you have other arguments, like you think we can achieve better quality by using system level stuff, please make this discussion more public. >> Source/WebKit2/ChangeLog:9 >> + Deleting files in signal handlers is not a good idea, >> + because process memory might not be valid after a crash. >> + To avoid the need change following: > > I dont understand the above sentence. "For this we need the following?" The crash handling has been removed, now the CleanupHandler is just about termination. Actually the signal handler just stops the event loop. The actual removal of the files (and releasing of the shared mem segments) is happening later when the aboutToQuit signal is emitted. > Source/WebKit2/ChangeLog:25 > + - Remove MemoryMappedPool and rely on libc/kernel caching > + of mmapped areas. We can remove MamoryMappedPool if it seems like not a valuable optimization but we can do it without the rest of this patch. This should be factored out from the patch and maybe the rest should be sliced further. (In reply to comment #6) > (From update of attachment 79274 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=79274&action=review > > Generally this patch moves to the direction to implement our API > on each platform with the platform's system level insfrastructure. > This is differ from how QtWebKit has been developed so far. Nah. The ConnectionQt.cpp was just written in one way, there's no philosophic agenda afaik. > I am not saying I'm against this but such a philosophic change should be > debated with the wider community before moving into that direction. Why? This patch fixes legimite (big) problems, which is is the purpose of this patch. The philosophic agenda was not discussed with any other patch either, it is not written anywhere. Are you sure there's such a philosophic agenda? To my knowledge, the philosophic agenda is to have the best possible implementation, regardless of if it uses Qt or not. > It's seems obvious to me that the problems with the CleanupHandler is > not enough as an argument to this change. If you have other arguments, Yes, it really is. CleanupHandler is fundamentally wrong design and thus there's no way to really back it "philosophically". May I remind you: signal handlers in general, and actions in signal handlers are _very_ platform specific, even if you use a class that begins with Q letter. In practice: It creates huge problems just because it causes legitimate crash dumps of ui process to be invalid, because the CleanupHandler will cause the ui to crash second time and hide the original crash dump. In practice in other areas: the other parts of the implementation are not that good either. > like you think we can achieve better quality by using system level stuff, > please make this discussion more public. I've written many points of rationale already in this bug, and nobody has made a real argument against them. Also further: The implementation proposed here implements the API more like the way it's designed originally in WebKit2 on mac: the message passing is unbuffered, passing small messages fast, etc. So the implementation proposed here is more aligned with the mac implemetation. This should mean that it's better for qt webkit port, because it gets all the same improvements that get done architecturally to the other main ports. On the other hand, may also be better for the rest of the webkit community, because there's less of a chance that Qt-specific hacks are needed and maybe we can even contribute to the general architecture at some point. > We can remove MamoryMappedPool if it seems like not a valuable optimization but we > can do it without the rest of this patch. This should be factored out from the patch > and maybe the rest should be sliced further. The memorymappedpool doesn't add much, thus you cannot really require it. On the other hand, when you use it in this context, it creates the problems pointed above in this bug. It simply is not needed in its current form, as you can see from the patch: it can be just removed and everything works. Why are we debating philosophical problems here? > In practice: It creates huge problems just because it causes legitimate crash dumps of ui process to be invalid, because the CleanupHandler will cause the ui to crash second time and hide the original crash dump. No, it doesn't do that. Like I said, crash handling has been removed in http://trac.webkit.org/changeset/74967. Profiling this patch, I think it has problems. At least on encrypted /tmp, a continuous animation exercises kcryptd with this patch, where as trunk without this patch does not. This indicates that there's a lot of redundant disk activity with the patch. The main argument I see in favor of this approach is that IMHO it's the right way of establishing the shared memory. 1) Shared memory allocated via shmget is subject to system wide limitations (/proc/sys/kernel/shmall on Linux). Space in /tmp is in my experience less of an issue, and certainly a more portable approach to managing the memory needs (instead of a Linux specific one) 2) Memory allocated with shmget is prone to leaking this system limited pool of memory when the processes crash. The extension of allowing to shmat() to a segment that's already deleted is a Linux extension (I think it's also available on Solaris). It's not really portable though. The approach of creating the file in /tmp, mmap'ing it, unlinking from the filesystem and passing the fds is a portable solution that is robust. 3) The approach of passing file descriptors should work nicely with sandboxing. Shared file names require the same filesystem namespace for the web and ui process, which is certainly not desirable. So yes, this means using system APIs instead of Qt APIs, and it also means the code will have to be written a few times, but how bad is it really? We need a posixy implementation, one for Win32 (maybe use ConnectionWin.cpp?) and one for Symbian (*sigh*). But it will allow us to have the fastest, most efficient and robust implementation on each platform. (unless tmp is encrypted hehe) Thank you for pointing these out, now the picture is getting clearer to me. Created attachment 79606 [details] Second try. Fixes the regression and implements connectionDidClose The performance regression was due to leaking fd handles that were passed to the other process. This caused them to be synced to the disk and thus performance regression. This also implements Connection::connectionDidClose. This is needed when web process crashes. The detection is implemented by observing the web process termination. To test this, bug 52796 is needed. Attachment 79606 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Source/WebKit2/Platform/qt/SharedMemoryQt.cpp:55: Semicolon defining empty statement for this loop. Use { } instead. [whitespace/semicolon] [5]
Source/WebKit2/Platform/CoreIPC/qt/ConnectionQt.cpp:99: More than one command on the same line in while [whitespace/parens] [4]
Total errors found: 2 in 18 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 79608 [details]
And 3rd patch, remove unneeded qthread include from ConnectionQt.cpp and invalid assert in Attachment::dispose
sorry for the spam.
Attachment 79608 [details] did not build on win: Build output: http://queues.webkit.org/results/7539242 Adam, any idea what is going on on windows? Comment on attachment 79608 [details] And 3rd patch, remove unneeded qthread include from ConnectionQt.cpp and invalid assert in Attachment::dispose View in context: https://bugs.webkit.org/attachment.cgi?id=79608&action=review Pretty nice patch, r=me > Source/WebKit2/Platform/CoreIPC/ArgumentDecoder.cpp:53 > + for (Deque<Attachment>::iterator i = m_attachments.begin(); i != end; ++i) We normally call non-int iterators for 'it' > Source/WebKit2/Platform/CoreIPC/ArgumentDecoder.cpp:54 > + i->dispose(); Something went wrong with indent here > Source/WebKit2/Platform/CoreIPC/Attachment.cpp:33 > +#if PLATFORM(QT) > +#include <unistd.h> > +#include <errno.h> > +#endif I think this could be made into a Linux (or POSIX) implementation in the future, so we might want to change the ifdefs then > Source/WebKit2/Platform/CoreIPC/Attachment.cpp:92 > + while (close(m_fileDescriptor) == -1 && (errno == EINTR)) {} space needed between { and }, coding style rule > Source/WebKit2/Platform/CoreIPC/Connection.h:103 > #elif PLATFORM(QT) > - typedef const QString Identifier; > + typedef int Identifier; > #elif PLATFORM(GTK) > typedef int Identifier; > #endif Seems that these could be combined > Source/WebKit2/Platform/CoreIPC/qt/ConnectionQt.cpp:98 > + while (close(m_socketDescriptor) == -1 && errno == EINTR) {} Space needed between { and } > Source/WebKit2/Platform/CoreIPC/qt/ConnectionQt.cpp:313 > + > + Remove one of those newlines > Source/WebKit2/Platform/qt/SharedMemoryQt.cpp:55 > + while (close(m_fileDescriptor) == -1 && errno == EINTR) {} space between { and } > Source/WebKit2/Platform/qt/SharedMemoryQt.cpp:115 > + while (close(fileDescriptor) == -1 && errno == EINTR) {} space between { and }, I think this is used to much that we should make a macro for it. That would also make the code more clear > Source/WebKit2/UIProcess/Launcher/qt/ProcessLauncherQt.cpp:114 > + // Don't expose web socket to possible future web processes *the* web socket (In reply to comment #16) > Adam, any idea what is going on on windows? You put the declaration of dispose() inside PLATFORM(QT), but the definition is unguarded. Here's the error: 4>..\Platform\CoreIPC\Attachment.cpp(89) : error C2039: 'dispose' : is not a member of 'CoreIPC::Attachment' 4> c:\cygwin\home\buildbot\webkit\source\webkit2\platform\coreipc\Attachment.h(34) : see declaration of 'CoreIPC::Attachment' Maybe an AttachmentQt.cpp file would be more appropriate? Attachment 79608 [details] did not build on mac: Build output: http://queues.webkit.org/results/7599247 Created attachment 79899 [details]
4th version, fixing compiles and comments (uses AttachmentQt.cpp)
The commit-queue encountered the following flaky tests while processing attachment 79899 [details]: http/tests/xmlhttprequest/cross-origin-authorization.html bug 52398 (author: ap@webkit.org) The commit-queue is continuing to process your patch. Comment on attachment 79899 [details] 4th version, fixing compiles and comments (uses AttachmentQt.cpp) Rejecting attachment 79899 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'la..." exit_code: 1 Last 500 characters of output: er -> origin/master Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ... Currently at 76488 = ca4146104c9f9db9b04d4b446a63be89f4c0924f r76489 = 42a6ccb2175a21e731f8d40fb792de897ee02ac0 r76490 = 53884fbd3b595d4b1b7df35cbd91d0e3a6cc21ec Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc First, rewinding head to replay your work on top of it... Fast-forwarded master to refs/remotes/origin/master. Full output: http://queues.webkit.org/results/7580312 Comment on attachment 79899 [details]
4th version, fixing compiles and comments (uses AttachmentQt.cpp)
Let's try the EWS's.
D'oh, mid air collision. By the way, here are my suggestions (copied from the preview): Just some impressions. I do not have the knowledge to review the posix stuff but generally the patch looks like a pretty good and correct work to me. > Source/WebKit2/ChangeLog:9 > + Deleting files in signal handler of UI process is not a good idea, > + because the memory where filenames are stored might not be valid > + after a crash. The expected performance and stability improvements should be also mentioned as well as the change in porting policy - i.e. that a given platform should implement it's own IPC mechanisms). > Source/WebKit2/Platform/CoreIPC/qt/ConnectionQt.cpp:106 > + if (m_socketDescriptor != -1) > + while (close(m_socketDescriptor) == -1 && errno == EINTR) { } > + > + if (!m_isConnected) > + return; > + > + delete m_socketNotifier; > + m_socketNotifier = 0; > + m_socketDescriptor = -1; > + m_isConnected = false; This seems to be somewhat overcomplicated, should return early if m_socketDescriptor is -1 (and maybe do not touch m_isConnected at all). > Source/WebKit2/Platform/CoreIPC/qt/ConnectionQt.cpp:259 > + // Schedule a call to readyReadHandler. Data may have arrived before installation of the signal > + // handler. Nit: the comment should not be breaked > Source/WebKit2/Platform/qt/WorkQueueQt.cpp:88 > + notifier->setEnabled(true); Why call this again? > Source/WebKit2/UIProcess/Launcher/qt/ProcessLauncherQt.cpp:98 > + // Don't expose the ui socket to the web process Nit: missing dot > Source/WebKit2/WebProcess/qt/WebProcessMainQt.cpp:165 > + qDebug() << "Error: wrong number of arguments."; > + return 1; qFatal? (In reply to comment #24) > D'oh, mid air collision. By the way, here are my suggestions (copied from the preview): > > Just some impressions. I do not have the knowledge to review the posix stuff but generally > the patch looks like a pretty good and correct work to me. > > > Source/WebKit2/ChangeLog:9 > > + Deleting files in signal handler of UI process is not a good idea, > > + because the memory where filenames are stored might not be valid > > + after a crash. > > The expected performance and stability improvements should be also mentioned as well as Is that not enough rationale? Major stability and reliability fix (avoid deleting possibly arbitrary files, and actually guarantee cleanup). This patch fixes the cleanup handler. If you read from the points, you can walk back the implementation and see that it's also pretty minimal -- eg. it's almost smallest patch that implements the cleanup correctly. It would be harder to reproduce the slightly buggy behavior of current trunk. > the change in porting policy - i.e. that a given platform should implement it's own IPC mechanisms). Is it necessary? I don't think there's a change in porting policy. Neither I think it's very unclear to anyone how to implement IPC for Qt. Use whatever works best. If there's Qt-only way to do this better, I don't see why not using it should be mandated by some policy. > > Source/WebKit2/Platform/CoreIPC/qt/ConnectionQt.cpp:106 > > + if (m_socketDescriptor != -1) > > + while (close(m_socketDescriptor) == -1 && errno == EINTR) { } > > + > > + if (!m_isConnected) > > + return; > > + > > + delete m_socketNotifier; > > + m_socketNotifier = 0; > > + m_socketDescriptor = -1; > > + m_isConnected = false; > > This seems to be somewhat overcomplicated, should return early if m_socketDescriptor is -1 (and maybe do not touch m_isConnected at all). This structure is to match other ports. The desire to match other ports is due be able to conform ASSERTS in the general Connection.cpp and other parts of the code. > > Source/WebKit2/Platform/CoreIPC/qt/ConnectionQt.cpp:259 > > + // Schedule a call to readyReadHandler. Data may have arrived before installation of the signal > > + // handler. > > Nit: the comment should not be breaked Comments need to switch lines at some point. That's set to 100 columns. AFAIK there isn't webkit coding style policy about this. Is this a big deal? > > Source/WebKit2/Platform/qt/WorkQueueQt.cpp:88 > > + notifier->setEnabled(true); > > Why call this again? Can you elaborate on the "again"? The first setEnabled(false) call disables the notifier, so that no call is made from other thread with half-constructed state. This call then enables the notifier. > > Source/WebKit2/UIProcess/Launcher/qt/ProcessLauncherQt.cpp:98 > > + // Don't expose the ui socket to the web process > > Nit: missing dot Good point. (pun intended :) ) > > Source/WebKit2/WebProcess/qt/WebProcessMainQt.cpp:165 > > + qDebug() << "Error: wrong number of arguments."; > > + return 1; > > qFatal? Why? What does it improve? Here we can control the exit code, and thus it can be checked if somebody wants to check it in the future. So, do you need to discuss these nits before this patch lands? I think it is fine for landing, please go ahead > > > Source/WebKit2/Platform/qt/WorkQueueQt.cpp:88 > > > + notifier->setEnabled(true); > > > > Why call this again? > > Can you elaborate on the "again"? The first setEnabled(false) call disables the notifier, so that no call is made from other thread with half-constructed state. This call then enables the notifier. My bad. > > So, do you need to discuss these nits before this patch lands? No, my suggestions are generally subjective or affecting only style so the patch can land as it is. Comment on attachment 79899 [details] 4th version, fixing compiles and comments (uses AttachmentQt.cpp) Clearing flags on attachment: 79899 Committed r76507: <http://trac.webkit.org/changeset/76507> All reviewed patches have been landed. Closing bug. *** Bug 46252 has been marked as a duplicate of this bug. *** |