Re: Recent version of Iceweasel along with fixes
On Tue, Sep 16, 2014 at 11:18:04PM +0800, Zhang Cong wrote: @Mike, what things need to do to make this work upstream, as we talked in, Zhang, please don't take it wrong, but it's not your business. You didn't do the work, you didn't actually contribute to anything yet, don't try and boss other people. Mike will handle the bug when he decides to, and I'll keep providing packages in the mean time. -- Richard Braun
Re: Recent version of Iceweasel along with fixes
On Thu, Feb 27, 2014 at 10:17:07PM +0100, Richard Braun wrote: I intend to regularly update these packages to track the experimental branch until the changes are merged in the official repository. Iceweasel 31.1.0esr-1 (from unstable) is available. -- Richard Braun
Re: GDB testsuite: »Memory at address 0 is possibly executable«
On Sat, Sep 13, 2014 at 01:39:05AM +0200, Samuel Thibault wrote: So it seems we need what is not actually documented, i.e. a vm_map with anywhere=1, but which takes into account the suggested address. I'm fine with officially supporting that, we just need to fix the documentation, and fix all places which weren't aware of this behavior (there are very few). Do we agree on this? Personally, I do prefer this approach, since that's how mmap is expected to behave (with the interesting addition that, if MAP_FIXED isn't set, but the hint is non-zero, the returned mapping must not start at address 0). -- Richard Braun
Re: Small test example for: cannot create /dev/null: Interrupted system call
On Mon, Jul 07, 2014 at 01:29:20PM +0200, Svante Signell wrote: ./test.sh: 6: ./test.sh: cannot create /dev/null: Interrupted system All right, it looks like open() gets interrupted by SIGCHLD here. It's my understanding that signal handling is highly system-specific in such cases, but we probably want to align on what others do, as usual. -- Richard Braun
Re: Small test example for: cannot create /dev/null: Interrupted system call
On Thu, Jul 10, 2014 at 11:18:17PM +0200, Richard Braun wrote: All right, it looks like open() gets interrupted by SIGCHLD here. It's my understanding that signal handling is highly system-specific in such cases, but we probably want to align on what others do, as usual. By the way, it looks like it happens with dash only, not bash. I suppose dash relies on the Linux-specific behaviour of interrupting a blocked open(). See if switching to bash helps. -- Richard Braun
Re: Small test example for: cannot create /dev/null: Interrupted system call
On Thu, Jul 10, 2014 at 02:21:06PM -0700, Roland McGrath wrote: Is the SIGCHLD set with SA_RESTART? From what I see, no. -- Richard Braun
Re: Bug#752237: libc0.3: send() asked to transmit 0 chars still triggers recv() on Hurd
On Sat, Jun 21, 2014 at 03:56:46PM +0200, Andreas Cadhalpun wrote: This is because the client is calling: send(sockfd, , 0, 0) Normally this doesn't trigger recv() in the server and thus can be used to test, whether the socket is working. But on Hurd it actually sends an empty message, so that the real message, which is sent later, is not received. Hello, Thanks for the report. There are actually two sides of the problem. First, I agree that there seems to be a bug, but let's take a closer look at the spec. The return value for recv() is defined as : Upon successful completion, recv() shall return the length of the message in bytes. If no messages are available to be received and the peer has performed an orderly shutdown, recv() shall return 0. This doesn't creates a strict equivalence between orderly shutdown and shall return 0. But in practice, this seems to be the actual assumption, so let's say that there is indeed a Hurd bug here. By the way, although the send() and recv() functions themselves (in glibc) may benefit from additions to filter out empty messages (at least send()), the server functions are those found in pflocal concerning AF_UNIX sockets. But there is also a bug in the client code, IMO. Here is how send() is specified : The length of the message to be sent is specified by the length argument. [...] If the socket argument refers to a socket and the flags argument is 0, the send() function is equivalent to write(). And here is write() : If nbyte is zero and the file is not a regular file, the results are unspecified. We might also want to change this though, since the behaviour observed on other systems seems more appropriate. -- Richard Braun
Re: Bug#752237: libc0.3: send() asked to transmit 0 chars still triggers recv() on Hurd
On Sat, Jun 28, 2014 at 10:48:56AM +0200, Richard Braun wrote: On Sat, Jun 21, 2014 at 03:56:46PM +0200, Andreas Cadhalpun wrote: This is because the client is calling: send(sockfd, , 0, 0) Normally this doesn't trigger recv() in the server and thus can be used to test, whether the socket is working. But on Hurd it actually sends an empty message, so that the real message, which is sent later, is not received. Hello, Thanks for the report. There are actually two sides of the problem. First, I agree that there seems to be a bug, but let's take a closer I've committed a fix upstream [1], please see if it solves your issues as well. -- Richard Braun [1] http://git.savannah.gnu.org/cgit/hurd/hurd.git/commit/?id=6f856c62613ffc82bf3572a372d2851638c2fb90
Re: Bug#752237: libc0.3: send() asked to transmit 0 chars still triggers recv() on Hurd
On Sat, Jun 28, 2014 at 12:09:15PM +0200, Samuel Thibault wrote: Richard Braun, le Sat 28 Jun 2014 11:51:42 +0200, a écrit : On Sat, Jun 28, 2014 at 10:48:56AM +0200, Richard Braun wrote: On Sat, Jun 21, 2014 at 03:56:46PM +0200, Andreas Cadhalpun wrote: This is because the client is calling: send(sockfd, , 0, 0) Normally this doesn't trigger recv() in the server and thus can be used to test, whether the socket is working. But on Hurd it actually sends an empty message, so that the real message, which is sent later, is not received. I've committed a fix upstream [1], please see if it solves your issues as well. [1] http://git.savannah.gnu.org/cgit/hurd/hurd.git/commit/?id=6f856c62613ffc82bf3572a372d2851638c2fb90 Mmm, I'm not sure we want to fix it in libpipe: other users of libpipe may want to pass empty messages (e.g. as mere tokens). Strictly speaking, it could be fixed in glibc's send() function. It however makes sense to me to fix it at the socket RPC protocol, i.e. in pflocal's S_socket_send. In addition, I just noticed that passing control data relies on always passing regular data, so this needs to be fixed too. However, I'm not sure I understand why other users would rely on a stream protocol for tokens. This would merely result in spurious wakeups, and AIUI, control data is meant exactly for that purpose. I'll see if simply catching completely empty messages at socket_send is a good enough solution. -- Richard Braun
Re: Bug#752237: libc0.3: send() asked to transmit 0 chars still triggers recv() on Hurd
On Sat, Jun 28, 2014 at 12:42:40PM +0200, Richard Braun wrote: I'll see if simply catching completely empty messages at socket_send is a good enough solution. The solution seems to work, and I couldn't see anything against it, unlike the previous attempt. However I'd really like to put it into libpipe as I don't see any need, but only potential problems, for empty data packets. I'll wait for your reply before taking action. -- Richard Braun
Re: Recent version of Iceweasel along with fixes
On Thu, Feb 27, 2014 at 10:17:07PM +0100, Richard Braun wrote: I intend to regularly update these packages to track the experimental branch until the changes are merged in the official repository. Iceweasel 30.0-2 (from unstable) is available. -- Richard Braun
Re: Please merge the random translator into the Hurd repository
On Thu, Jun 12, 2014 at 09:38:48AM +0200, Justus Winter wrote: I think I’ve already asked this one, but still: why not a Git submodule? Complexity. We are trying to reduce it. One problem with the “merge ’em all” approach is that one can /add/ a thing to the repository, but one cannot really /delete/ it later. (In the sense that from that point on, the code added would remain in the Git history, – and be $ git clone’d forever.) In my head, that is exactly the point of a vcs, but ymmv. On the contrary, submodules may be added and removed without any hassle. And from this point, I’d rather see Hurd – a collection of servers – living as a collection of Git repositories. I'd rather see the Hurd not being a major pain in the ass to develop, but again, ymmv, as I guess it's a matter of perspective. The only downside I could think of is that a single commit (unless made to the “upper level” repository) cannot really span multiple Git repositories. Which may be a problem as long as there’re strong interdependencies in the servers’ code. There are strong interdependencies, some are explicit, some are implicit. And being able to change some aspects of the Hurd is part of why I advocated this merge in the first place (as clearly stated in my original mail). I strongly agree with Justus on this. -- Richard Braun
Re: let's make libpager single-threaded
On Fri, May 30, 2014 at 09:35:37AM +0200, Justus Winter wrote: Quoting Richard Braun (2014-05-29 19:12:13) On Thu, May 29, 2014 at 07:04:48PM +0200, Samuel Thibault wrote: But precisely: once the only thread gets a data_request, it'll call pager_read_page, which will typically call store_read, which will wait on the eventual I/O, and no other request will be processed during that time, and thus not pipelining the I/O requests. Right, I assumed the store interface wasn't synchronous... So we want a fixed number of pager threads? Let's see if a single thread is actually that bad: what other request could actually be processed while a paging thread is waiting for I/O completion ? -- Richard Braun
Re: let's make libpager single-threaded
On Fri, May 30, 2014 at 08:15:43PM +0200, Samuel Thibault wrote: Justus Winter, le Fri 30 May 2014 09:35:37 +0200, a écrit : Quoting Richard Braun (2014-05-29 19:12:13) On Thu, May 29, 2014 at 07:04:48PM +0200, Samuel Thibault wrote: But precisely: once the only thread gets a data_request, it'll call pager_read_page, which will typically call store_read, which will wait on the eventual I/O, and no other request will be processed during that time, and thus not pipelining the I/O requests. Right, I assumed the store interface wasn't synchronous... So we want a fixed number of pager threads? We can use a fixed number of pager threads, yes. Perhaps just 1 indeed currently gives the best performance because the device backends that we currently use don't get benefit from piling up requests. But at least we keep the infrastructure to pile up requests for when we fix the backends. Agreed. -- Richard Braun
Re: let's make libpager single-threaded
On Thu, May 29, 2014 at 07:04:48PM +0200, Samuel Thibault wrote: But precisely: once the only thread gets a data_request, it'll call pager_read_page, which will typically call store_read, which will wait on the eventual I/O, and no other request will be processed during that time, and thus not pipelining the I/O requests. Right, I assumed the store interface wasn't synchronous... -- Richard Braun
Re: let's make libpager single-threaded
On Fri, May 23, 2014 at 01:44:09AM +0200, Samuel Thibault wrote: Richard Braun, le Mon 05 May 2014 18:32:26 +0200, a écrit : On Mon, May 05, 2014 at 06:01:17PM +0200, Samuel Thibault wrote: ? The patch makes both ext2fs's service_paging_requests and libdiskfs' service_paging_requests become singlethreaded. That's what I call the paging part. The front side, where client calls are processed, is still multithreaded, which is what really matters. But the paging part also comes from client events, doesn't it? E.g. on-demand loads of parts of mapped libraries, etc. If on-demand here means on page fault, then no. In Mach, client threads don't process their page faults, the kernel merely sends async requests to external pagers. There is normally no I/O when the underlying physical pages are already resident. External pagers are used only when I/O must be done, in which case a single thread per backing device is fine since processing incoming requests is normally orders of magnitude faster than the underlying device latency. Note that one of the future directions of the thread migration paper [1] was about making client threads service their own page faults. The two advantages of this strategy that I can think of are 1/ concurrency in diskless file systems where I/O latency can actually be very low and 2/ maintaining scheduling properties. We could use a multithreaded pool to help case 1/, and 2/ actually doesn't matter that much since scheduling properties are most important to real time applications, which are supposed to lock memory in anyway, but it's better for processor time accounting nonetheless. -- Richard Braun [1] http://www.bford.info/pub/os/thread-migrate.pdf
Some test results about libihash
On Tue, May 13, 2014 at 10:37:05AM +0200, Richard Braun wrote: It disappeared a few months ago, and since it's not widespread, I suggest, despite providing to Justus in the first place, to use the Murmur finalizer instead. But we need to test that part a bit more. I'm currently doing just that. Here are some results I could obtain : 1/ Using an extremely simple microbenchmark [1] that merely inserts keys, either random integers or sequential ones to match the way port names are managed, it seems that the previous code, despite its apparent flaws, did quite well. Using an integer hashing function actually reduces performance on the sequential integer test case. It makes sense because, considering a set of consecutive integers starting from 0, and a hash table that always has more slots than items, a modulo is a perfect hash function. Even when taking into account that only names for receive rights are normally managed with libihash, i.e. that keys aren't actually sequential, they are almost all equally distributed, leading to very few collisions. Therefore, as a third option, I've removed the hashing function, leaving only a fast modulo (an AND) and this variant provided the best raw results. 2/ I've also built hurd packages multiple times and got average build times with each variant (previous, new, new without hash function) and here are the results I obtained respectively : 52m59s, 52m31s, 52m22s. In the end, no surprise, but still a decent improvement. Good job :). -- Richard Braun [1] http://darnassus.sceen.net/gitweb/rbraun/ihtest.git
Re: GSOC - Valgrind for Hurd
On Tue, May 20, 2014 at 10:50:07PM +0200, Samuel Thibault wrote: I would advise starting with trivial system calls, such as mach_thread_self, mach_task_self, and mach_host_self, which don't take any parameter, and just returns a port. That'll make you implement the basic infrastructure for the trap call. Keep in mind that, while most system calls are actually RPCs, a few of them actually are real system calls (i.e. traps) in addition to mach_msg. mach_thread_self and mach_task_self are such calls. -- Richard Braun
Re: GSOC - Valgrind for Hurd
On Tue, May 20, 2014 at 11:50:28PM +0200, Samuel Thibault wrote: Richard Braun, le Tue 20 May 2014 23:42:24 +0200, a écrit : On Tue, May 20, 2014 at 10:50:07PM +0200, Samuel Thibault wrote: I would advise starting with trivial system calls, such as mach_thread_self, mach_task_self, and mach_host_self, which don't take any parameter, and just returns a port. That'll make you implement the basic infrastructure for the trap call. Keep in mind that, while most system calls are actually RPCs, a few of them actually are real system calls (i.e. traps) in addition to mach_msg. mach_thread_self and mach_task_self are such calls. Well, valgrind will only see system calls, not RPCs. Yes, and that's important to stress since calls like vm_map are RPCs. Subhashish must be well aware of what to expect (if it's already been done, good, otherwise consider this a reminder). -- Richard Braun
Re: Recent version of Iceweasel along with fixes
On Thu, Feb 27, 2014 at 10:17:07PM +0100, Richard Braun wrote: I intend to regularly update these packages to track the experimental branch until the changes are merged in the official repository. Iceweasel 29.0.1 (from unstable) is available. -- Richard Braun
Re: [PATCH 07/11] libihash: use an integer hash function on the keys
On Tue, May 13, 2014 at 09:47:31AM +0200, Neal H. Walfield wrote: This URL is not valid (ttwang.cnc.net Not Available. The domain ttwang.cnc.net which you are trying to access is currently unavailable...). Do you have a more stable URL? At least mention the name of the paper. It disappeared a few months ago, and since it's not widespread, I suggest, despite providing to Justus in the first place, to use the Murmur finalizer instead. But we need to test that part a bit more. I'm currently doing just that. -- Richard Braun
Re: [PATCH 03/11] include: add lock-less reference counting primitives
On Tue, May 13, 2014 at 12:56:03PM +0200, Samuel Thibault wrote: AIUI this cast is a case of type-puning. Why not making refcounts_t the union itself? That way would be clearly safe with gcc's extension. As stated in the comment for refcounts_t, I like the idea of using the type system to enforce the use of the accessor functions. I understand, but type-puning will break with smart compilers. We do have had bugs like that in the past. I agree, you should directly use the proper type here, or maybe void * but I don't see the point. -- Richard Braun
Re: [PATCH 03/11] include: add lock-less reference counting primitives
On Tue, May 13, 2014 at 12:23:48AM +0200, Samuel Thibault wrote:
Justus Winter, le Mon 12 May 2014 12:05:41 +0200, a écrit :
+ const union _references op = { .refs = { .hard = 1 } };
+ refcounts_t r = __atomic_add_fetch (ref, op.rc, __ATOMIC_RELAXED);
Mmm, I don't think it is allowed by C to write into a field and read
from another field. The legacy Hurd code used to tend to do it in some
places, but it breaks with smart compilers.
It's not defined by C, but all major compilers support it as it's the
cleanest way, if done properly, not to violate strict aliasing currently.
--
Richard Braun
Re: let's make libpager single-threaded
On Mon, May 05, 2014 at 04:55:19PM +0200, Samuel Thibault wrote: It's an interesting alternative indeed. This however means our ext2fs is not multithreaded any longer, which is a bit sad considered that we'll want to go parallel in the end. Well, no, only the paging part becomes single threaded, which is fine since it's the I/O bound part. -- Richard Braun
Re: let's make libpager single-threaded
On Mon, May 05, 2014 at 06:01:17PM +0200, Samuel Thibault wrote: ? The patch makes both ext2fs's service_paging_requests and libdiskfs' service_paging_requests become singlethreaded. That's what I call the paging part. The front side, where client calls are processed, is still multithreaded, which is what really matters. -- Richard Braun
Re: let's make libpager single-threaded
On Mon, Apr 28, 2014 at 12:19:55PM +0200, Justus Winter wrote: We will try this change on darnassus and follow-up with the results. Unfortunately, this looks too unstable to merge as it is. There seems to be a port leak somewhere, and it looks like support for notifications is needed, as I could freeze a file system by merely writing to it (e.g. with dd) and interrupting with Ctrl-C. But it's certainly on the right path and shouldn't be far from being reliable (or at least, a lot more reliable than the current code). Thanks a lot for working on this. -- Richard Braun
Re: GCC's -fsplit-stack disturbing Mach's vm_allocate
On Fri, Apr 11, 2014 at 11:51:44PM +0200, Samuel Thibault wrote:
It's indeed:
/* This function is called at program startup time to make sure that
mmap, munmap, and getpagesize are resolved if linking dynamically.
We want to resolve them while we have enough stack for them, rather
than calling into the dynamic linker while low on stack space. */
void
__morestack_load_mmap (void)
{
/* Call with bogus values to run faster. We don't care if the call
fails. Pass __MORESTACK_CURRENT_SEGMENT to make sure that any
TLS accessor function is resolved. */
mmap (__morestack_current_segment, 0, PROT_READ, MAP_ANONYMOUS, -1, 0);
mprotect (NULL, 0, 0);
munmap (0, getpagesize ());
}
Yes...
So, do we really want to let munmap poke a hole at address 0 and thus
let further vm_map() return address 0?
We probably don't. AIUI, the first page is always mapped, and always
with PROT_NONE to make sure null pointers are catched. Considering other
systems have predefined ranges depending on the mapping type, instead
of blindly starting at the beginning of the map like vm_map() does, it's
perfectly valid to unmap the first page, which is normally the right
way to catch null pointers. So, since we do want to catch null pointers,
we do want to keep that first page, but only the first page. Or rather,
a range large enough to catch accesses through null pointers, e.g. it
could even be 64 or 128 KiB. We could alter glibc so that the first
mapping has this special size, and have munmap override its given range
to skip that area.
--
Richard Braun
Re: Problem with glibc and libihash
On Sun, Apr 13, 2014 at 11:56:44PM +0200, Samuel Thibault wrote: Manolis Ragkousis, le Sun 13 Apr 2014 21:50:17 +, a écrit : First of all libihash and any of the hurd libraries cannot be built without a working glibc. Hurd libs depend heavily on header files found in glibc like lowlevellock.h, Mmm, if it's only headers, wouldn't it be possible to install just them? Keep in mind that, when boostrapping toolchains, it is common practice to install headers only, and build partially in multiple stages until the final versoin is obtained. -- Richard Braun
Re: Hurd support in libpcap
On Thu, Apr 10, 2014 at 02:01:17AM +0200, Richard Braun wrote: The first issue that can be noticed is that, despite the filter being filled with both NETF_IN and NETF_OUT, only incoming packets seem to be captured. This is probably a minor bug in libbpf but I didn't investigate yet. The issue seems to be in libmachdev, which simply doesn't forward sent packets to any packet filter. Is there a reason why libmachdev is only built statically ? -- Richard Braun
Re: Cleaning up dde for the Hurd (was: Hurd support in libpcap)
On Fri, Apr 11, 2014 at 03:13:02PM +0200, Justus Winter wrote: This is most likely just an oversight. I started going over the dde code. Zheng Da got lot's of little details wrong. This is not meant to sound harsh, I'm full of respect that he got dde-based drivers up and running during a gsoc. Agreed. Now let's team up and clean up his work so that we can merge it upstream and have a good template for more dde-based drivers :) It won't be easy and I'll probably won't have time to do much about it, for a change... But at least, I'm currently working on fixing locking on packet filters and actually relaying sent packets so that packet capture works. Note that this may slightly reduce performance because of the way filters work, i.e. a net_msg must be completely forged, with memory copies, before being sent to packet filters, even if there is no filter installed. If someone wants to work on optimizing this, feel free to do it, but please mind the details because that's where the tricky bugs will come from. -- Richard Braun
Hurd support in libpcap
Hello, A while ago, I added support for Hurd (actually Mach) network devices in libpcap. I've refreshed this work so that it now tries to fetch data from netdde before trying a Mach device, just like pfinet does. Debian packages built with the patch [1] are available from my repository for tests : deb http://ftp.sceen.net/debian-hurd-i386 experimental/ deb-src http://ftp.sceen.net/debian-hurd-i386 experimental/ The first issue that can be noticed is that, despite the filter being filled with both NETF_IN and NETF_OUT, only incoming packets seem to be captured. This is probably a minor bug in libbpf but I didn't investigate yet. -- Richard Braun [1] http://darnassus.sceen.net/~rbraun/0001-Debian-GNU-Hurd-with-NETDDE-support.patch
Re: Recent version of Iceweasel along with fixes
On Thu, Feb 27, 2014 at 10:17:07PM +0100, Richard Braun wrote: I intend to regularly update these packages to track the experimental branch until the changes are merged in the official repository. Iceweasel 28.0-1 and NSPR 4.10.4 are now available. -- Richard Braun
Re: Please merge the random translator into the Hurd repository
On Mon, Apr 07, 2014 at 01:49:52PM +0200, Thomas Schwinge wrote: If there is agreement, despite my comment b), that it'd be beneficial to merge the current external translators into the Hurd core repository's master branch, if this substantially makes any workflows easier, then I'm fine with accepting this. There is, I guess, no need to make our lives harder, for some abstract (?) benefits, just for the elegance of explicitly modularized system architecture? Grouping translators together along with the Hurd libraries and build system makes the life of developers a lot easier. It reduces the overhead related to build system settings, and it also reduces the likeliness of mistakes. It really is the pragmatic approach, and IMO, it strongly beats the apparent advantages of separate repositories. From experience, on this project or others, I've never actually seen or felt in any way any of these advantages in practice, but I've hit the issues many times... -- Richard Braun
Re: portseal - tools to locate port management bugs
On Sun, Mar 30, 2014 at 07:40:50PM +0200, Justus Winter wrote: here is another prototype of mine, also employing a source-transformation, that can detect port leaks: Well, that's impressive, once more. Excellent job. -- Richard Braun
GSoC deadline for proposals
Hello, Apparently, the deadline for proposals is tomorrow (March 21). Students indenting to participate should be careful not to miss it. -- Richard Braun
Re: New installation CDs and qemu image
On Tue, Mar 18, 2014 at 10:46:26AM +0100, Riccardo Mottola wrote: without success and further help, I just attempted a clean reinstall .As a note, I add that the machine I am using worked with hurd for years, until its hard-disk trashed. The installer went fine (it is quite slow, CD-ROM access seems quite painful) through all the steps, up to the last one where it says unmounting volumes. After 10 minutes it was still there without any disk activity, I shut down the machine. At boot, I do run fsck, as expected. At the next boot, I'm in the same situation as before. fsck says it can't check a mounted partition, my /proc is empty as is /var/run I will, as I did before, reconfigure the hurd package. But what can I do vor /var/run? and generally the problem? something else that could benefit of being reconfigured? Honestly, I'm not sure you'll get help for such a specific problem. The failed reinstall, though, is more troubling (and then interesting). I suggest you reattempt to reinstall, and use the other virtual terminals to check the status of the system if reinstallation fails again. -- Richard Braun
Re: New installation CDs and qemu image
On Tue, Mar 18, 2014 at 11:59:44AM +0100, Gabriele Giacone wrote: Did you install it again on / and /home distinct filesystems? If so, try to reinstall it on / only. Plus, don't set any mirror so that install takes all packages from CD. Just for the record, it's strongly advised to use separate / and /home partitions. -- Richard Braun
Re: New installation CDs and qemu image
On Tue, Mar 18, 2014 at 02:27:02PM +0100, Riccardo Mottola wrote: Also, if I can fix this installation, I'd prefer.. it took me two days to reinstall hurd: not continuously attented, but the operation was quite slow.. CD-ROM and HDD seem to be quite slow in access, especially the former. It's not disk access that is slow. It's the page cache that is in bad shape. It could also be due to a writeback caused thread storm (a kind of thundering herd problem with spin locks in userspace). If any potential GSoC candidate is reading this, fixing that particular issue is one project we'd very much appreciate mentoring. -- Richard Braun
Re: I got visudo working
On Fri, Mar 14, 2014 at 05:21:23PM -0500, Peter Baumgarten wrote: On Fri, 2014-03-14 at 09:29 +0100, Samuel Thibault wrote: Use patch -ur between an unmodified version of sudo and your modified version. Send that to debian-h...@lists.debian.org for review, and then I sent the patch to debian-h...@lists.debian.org does that qualify me now for google summer of code to work on the hurd? What qualifies a student is the quality, seriousness and relevance of his proposal. Prior positive experience sure helps. Would you say replacing configure.in with configure.ac and adding it in a series file is enough ? -- Richard Braun
Re: Trying to solve file lock problem with /etc/sudoers
On Tue, Mar 11, 2014 at 02:44:36AM -0500, Peter Baumgarten wrote: I'm trying to be a hurd developer by jumping straight in and trying to tackle a bug. This may not be a hurd bug, but I only see it with the hurd version of sudo. I am trying to figure out why I get this message visudo: /etc/sudoers busy, try again later when I run visudo as root. I suspect something has a file lock on /etc/sudoers. I would normally use lsof to see what processes have a lock on a file, but apparently lsof is not available for hurd. When I say not available I mean not installed by default on the debian hurd qemu image and not available in the debian hurd repos. Does anyone know what tools are available to me to see what has a lock on file or what processes are using a file on? To everyone who's trying to be a free software contributor, this is the right (or at least recommended) approach. The Hurd is a distributed system, and it's not straightforward to come up with tools like lsof. The closest one is probably portinfo, but it won't give you as much information. The issue is known, although I'm not familiar with the details. You need to understand the general architecture of the Hurd, and then dig in libdiskfs/libfshelp and the ext2fs translator, I suppose. It is definitely a Hurd bug, which may require development (i.e. it may be a feature missing, not a mere bug). Unless I'm mistaken, the issue is described on the wiki [1]. -- Richard Braun [1] http://darnassus.sceen.net/~hurd-web/open_issues/file_locking/
Re: New installation CDs and qemu image
On Wed, Mar 05, 2014 at 02:43:01PM +0100, Riccardo Mottola wrote: /dev/hd0s1:clean /dev/hd0s2 is mounted e2fsck cannot continue aborting. Automatic boot failed... help! # at the prompt I could type cat /etc/fstab /dev/hd0s1/ext2defaults0 1 /dev/hd0s2/bootext3defaults0 3 /dev/hd0s3noneswapsw0 0 /dev/hd2/media/cdrom0iso9660noauto0 0 what could be the problem? what is failing? it looks quite correct to me. Swap and CDROM shouldn't be fsck'd and they appear correct. You set hd0s2 to type ext3. -- Richard Braun
Re: Recent version of Iceweasel along with fixes
On Sat, Mar 01, 2014 at 12:13:18PM +0800, Zhang Cong wrote: 1. Crash once for clipboard assert=NULL?, I lost the detail. Please try to reproduce, or at least give more details about what you did before the crash occurred. -- Richard Braun
Recent version of Iceweasel along with fixes
Hello, I've started working on porting more recent versions of Iceweasel (our binary packages are from version 17), and fixing some important issues at the same time. The current status is that packages are available for version 27.0-2, and all major bugs have been fixed. I have reliably used it for several days straight, without terminating the test processes, with many tabs, with custom GTK themes, IPv4/IPv6 sites, SSL/TLS, and even SWF applets through Gnash. I consider that reliable enough for practical use, although the lack of acceleration and sound will make watching videos a bit frustrating at times ;-). The thread-safety issues that would lead to random crashes have been fixed in libc0.3 2.18-1. The SSL/TLS issue is addressed in a patch attached to bug #739658 [1]. The Iceweasel FTBFS issues themselves are still being worked on until I'm satisfied with the result. The patch fixing them can be found in the source packages on my personal repository, along with the binary packages for Iceweasel and nspr : deb http://ftp.sceen.net/debian-hurd-i386 experimental/ deb-src http://ftp.sceen.net/debian-hurd-i386 experimental/ I intend to regularly update these packages to track the experimental branch until the changes are merged in the official repository. Enjoy. -- Richard Braun [1] https://bugs.debian.org/cgi-bin/bugreport.cgi?msg=5;filename=0001-Define-_PR_HAVE_SOCKADDR_LEN-for-the-Hurd.patch;att=1;bug=739658
Re: [PATCH 1/5] ipc: add protected payload
On Fri, Feb 21, 2014 at 12:48:49PM +0100, Justus Winter wrote:
Add a field ip_protected_payload and a flag ip_has_protected_payload
to struct ipc_port.
diff --git a/ipc/ipc_port.h b/ipc/ipc_port.h
index 27d2e49..4c7c742 100644
--- a/ipc/ipc_port.h
+++ b/ipc/ipc_port.h
@@ -71,6 +71,10 @@ typedef unsigned int ipc_port_timestamp_t;
struct ipc_port {
struct ipc_target ip_target;
+ /* Flags. */
+ unsigned intip_has_protected_payload:1; /* A pp has
+been set. */
+
/* This points to the ip_target above if this port isn't on a port set;
otherwise it points to the port set's ips_target. */
struct ipc_target *ip_cur_target;
@@ -96,6 +100,7 @@ struct ipc_port {
mach_port_msgcount_t ip_msgcount;
mach_port_msgcount_t ip_qlimit;
struct ipc_thread_queue ip_blocked;
+ unsigned long ip_protected_payload;
};
Increasing the port structure size by 4 bytes for a single bit is a
little frustrating. Instead, how about storing that bit in
ip_target.ipt_object.io_bits, right next to IO_BITS_ACTIVE ?
--
Richard Braun
Re: [PATCH 1/5] ipc: add protected payload
On Fri, Feb 21, 2014 at 05:49:24PM +0100, Justus Winter wrote: -#define IO_BITS_OTYPE0x7fff /* determines a cache */ +#define IO_BITS_OTYPE0x3fff /* determines a cache */ +/* The following masks are used to store attributes of ipc ports. */ +#define IO_BITS_PROTECTED_PAYLOAD 0x4000 /* pp set? */ Thanks :-). -- Richard Braun
Re: protected payloads for GNU Mach
On Tue, Feb 18, 2014 at 09:49:23PM -0800, Samuel Thibault wrote: Apart from that and perhaps a function to clear the protected payload value (which may however not be really useful), the whole thing seems interesting and good to me, do other people agree? Well, since I've pushed that as a project idea [1], I'm quite happy to see it happening. Very good job again, Justus. -- Richard Braun [1] http://www.gnu.org/software/hurd/community/gsoc/project_ideas/object_lookups.html
Re: [PATCH 04/11] doc: document mach_port_set_protected_payload
On Mon, Feb 17, 2014 at 06:20:54PM +0100, Justus Winter wrote:
+@deftypefun kern_return_t mach_port_set_protected_payload (@w{ipc_space_t
@var{task}}, @w{mach_port_t @var{name}}, @w{unsigned long @var{payload}})
+The function @code{mach_port_set_protected_payload} sets the protected
+payload to @var{payload}. If @var{payload} is non-zero, the
+@code{msgh_protected_payload} field will be set to @var{payload} if a
+message is delivered to @var{name}.
If I'm right, this also means switching back from the protected payload
is done by calling this RPC with a payload of 0. It could be worth
emphasizing that 0 is an invalid value for a protected payload.
--
Richard Braun
Re: [PATCH 1/8] kern: fix printing of kmem_cache names
On Sun, Feb 02, 2014 at 12:11:59AM +0100, Samuel Thibault wrote: Justus Winter, le Sat 01 Feb 2014 16:34:20 +0100, a écrit : I thought about doing that. But that would waste one character just for the termination. That's little compared to security :) GNU Machs printf implementation supports strings with the precision given as argument: printf (%.*s, sizeof foo-name, foo-name); I think thats clean, concise and easy to use. But people would tend to forget doing it. We can't really fight that. Null-terminated strings is what people will naturally expect and have in mind while reading/editing the code. Doing things differently for something like mere names isn't worth the trouble. -- Richard Braun
Re: [PATCH v16] kern: simple futex for gnumach
On Fri, Jan 17, 2014 at 07:45:15PM -0800, Roland McGrath wrote:
This is why I was insisting on passing *memory* through IPC.
It's not at all clear that makes any kind of sense, unless you mean
something I haven't imagined. Can you be specific about exactly what the
interface (say, a well-commented MiG .defs fragment) you have in mind would
look like?
If it's an RPC that passes out of line memory, that (IIRC) always has
virtual-copy semantics, never page-sharing semantics. So it would be
fundamentally the wrong model for matching up with other futex calls (from
the same task or others) to synchronize on a shared int, which is what the
futex semantics is all about.
That's right, IPC can only copy private memory, not make it shared. So
technically, not through IPC, but through the VM system.
What I always anticipated for a Machish futex interface was vm_futex_*
calls, which is to say, technically RPCs to the task port (which need not
be the task port of the caller), passing an address as an integer literal
just as calls like vm_write do (and each compareexchange value as a datum,
i.e. an integer literal, just as vm_write takes a datum of byte-array type,
with semantics unchanged by whether that's inline or out of line memory).
That's more what I had in mind too.
The task port and address serve as a proxy by which the kernel finds the
memory object and offset, and the actual synchronization semantics are
about that offset in that memory object and the contents of the word at
that location. (Like all such calls, they would likely be optimized
especially for the case of calls to task-self and probably even to the
extent of having a bespoke syscall for the most-optimized case, as with
vm_allocate. But that's later optimization.)
Given the specified usage patterns for the futex operations, it might be
reasonable enough to implement those semantics solely by translating to a
physical page, including blocking to fault one in, and then associating the
wait queues with offsets into the physical page rather than the memory
object abstraction. (Both a waiter and a waker will have just faulted in
the page before making the futex call anyway.) But note that the semantics
require that if a waiter was blocked when the virtual page got paged out,
then when you page it back in inside vm_futex_wake, that old waiter must
get woken. I don't know the kernel's VM internals much at all, but I
suspect that all tasks mapping a shared page do not get eagerly updated
when the memory object page is paged in to service a page fault in some
other task, but rather service minor faults on demand (i.e. later) to
rediscover the new assocation between the virtual page and the new physical
page incidentally brought in by someone else's page fault a little earlier.
Since you need to track waiters at the memory object level while their page
is nonresident anyway, it probably makes sense just to hang the {offset =
wait queue} table off the memory object and always use that. At least,
that seems like the approach for the first version that ensures correctness
in all the corners of the semantics. It can get fancier as needed in later
optimizations. When it comes to optimizing it, a fairly deep understanding
of the Linux futex implementation (which I don't have off hand, though I
have read it in the past) is probably instructive.
Locking physical pages could be used for denial of service, i.e. a user
may implicitely starve the system of wired memory, unless they're
accounted as such, but then users might be randomly unable to use
mutexes. Trying to cope with object/offset to pages associations would
imply some container very similar to what has been considered until now
for the regular case (that is, a hash table or tree for all shared
futexes) so that futexes can immediately be reassociated to pages after
faulting them in. So I expect we have to use VM objects.
What I had in mind is already partially explained in previous mails
but I still didn't take the time to get a clear view of every use case
so it's probably incomplete. But it would start with a union of either
(task translated to map, address) or (object, offset), depending on the
futex type (private, shared, respectively). Problems I can see with that
approach are :
- do we have to check that shared futexes refer to shareable memory ?
- if so, how to make that check reliably ?
- what happens when unmapping a futex ?
- does copy-on-right have any effect on a private futex - if implemented
as a (map, address) pair, I imagine it wouldn't, but is it true ?
These are the kind of things I was hoping to discuss with this patch.
--
Richard Braun
Re: [PATCH v16] kern: simple futex for gnumach
On Sat, Jan 18, 2014 at 02:06:05AM +0100, Samuel Thibault wrote:
Diego Nieto Cid, le Fri 17 Jan 2014 22:49:25 -0200, a écrit :
El ene 17, 2014 11:36 a.m., Marin Ramesa m...@hi.t-com.hr escribió:
+
+kern_return_t
+futex_wait(task_t task, vm_offset_t futex_address, int value,
+ mach_msg_timeout_t msec, boolean_t private_futex)
+{
+ if (private_futex) {
+ struct private_futex *futex;
+
+ futex = futex_private_lookup_address(futex_address);
+ if (futex == NULL) {
+ futex = futex_private_init(futex_address);
+ if (futex == NULL)
+ return KERN_RESOURCE_SHORTAGE;
+ }
+
+ if (__atomic_load_n(
+ (int *) futex_address, __ATOMIC_RELAXED) ==
value) {
Are you sure you can dereference futex_address?
It's a user supplied virtual address which is probably not valid in the
kernel's virtual address space (unless things like copyin are used)
I have the vague idea that the kernel has it's own address space. Can
somebody
confirm that? :-)
Yes. Care has to be taken when dereferencing user pointers.
This is why I was insisting on passing *memory* through IPC. But this
does work in most cases on i386 since the kernel page tables are shared
by all user page tables to avoid a complete TLB flush on kernel entry,
as it's done for practically every system. So in practice, setting some
user data and accessing it from the kernel soon after will succeed most
of the time because writing the content will cause page faults if
needed, after which the kernel can simply read it.
That is how e.g. mach_print is able to do its job, although, for this
special call, it was done on purpose to avoid the VM system (since it's
a debugging call).
--
Richard Braun
Re: [PATCH v14] kern: simple futex for gnumach
On Mon, Jan 13, 2014 at 03:38:19PM +0100, Marin Ramesa wrote: Fixed a bug in private wakeups by calling ipc_entry_alloc_name(). This is the last version as everything works now. *Sigh* You didn't understand my comment mentioning IPC spaces at all... You keep wrongly using the red-black tree interface. You keep making basic locking mistakes. You still pass raw addresses/values instead of passing *memory*. You obviously didn't take the time to look at how assert_wait and thread_will_wait_with_timeout were used elsewhere and implemented. You didn't change the timeout parameter type, despite your claim for a final version. And worst of all, you dare claim everything works. You clearly have little clue about what you're actually doing, and don't make the effort to check your assumptions and learn. I won't review any more of your work. -- Richard Braun
Re: [RFC] kern: simple futex for gnumach (version 11)
On Fri, Jan 10, 2014 at 08:28:12AM +0100, Marin Ramesa wrote: On 01/10/2014 01:59:55 AM, Richard Braun wrote: On Thu, Jan 09, 2014 at 09:51:51PM +0100, Marin Ramesa wrote: On 01/09/2014 05:52:21 PM, Richard Braun wrote: On Thu, Jan 09, 2014 at 05:06:09PM +0100, Marin Ramesa wrote: Shouldn't the compare be atomic. Maybe I don't understand what atomic really means but the GCC manual says this function is. Or is it enough to hold the lock. From the GCC manual : These built-in functions perform an atomic compare and swap. That is, if the current value of *ptr is oldval, then write newval into *ptr. So tell us what the point of replacing oldval with itself is. I do a swap so it returns true if they are equal. I'll say it again because it doesn't look like you got it : when I point something out to you, consider you're wrong, and make the effort to question your assumptions. You may end up being right at times, but most often not. Now please, read your code again, then answer the question I asked : So tell us what the point of replacing oldval with itself is. OK, sorry. There's no point. I couldn't find just an atomic compare, so I'm using the compare-and-swap this way. I'll read more in the GCC manual, maybe there is another way. Reading correctly aligned integers that are not wider than the processor word size is already atomic. But there still is a problem I mentioned in another review and forgot this time : On Thu, Jan 09, 2014 at 03:45:35PM +0100, Richard Braun wrote: On Wed, Jan 08, 2014 at 08:43:28PM +0100, Marin Ramesa wrote: +routine futex_wait( + task: task_t; + address : vm_offset_t; + value : int; + msec: int; + private_futex : boolean_t); + +routine futex_wake( + task: task_t; + address : vm_offset_t; + wake_all: boolean_t); Looks good. You'll probably want the requeue operation too some day, but that can wait. Actually, this isn't good. Rework the futex_wait routine so that it passes *memory* to the kernel, not raw values. Once the kernel can directly access the futex value, a plain memory access should do the job, but since I haven't read the futex paper in depth, I'm not even sure why this is done before queueing a thread on a futex, and what are the exact constraints on the system call. -- Richard Braun
Re: About Regular IRC Meetings.
On Sat, Jan 11, 2014 at 03:26:50AM +0530, Subhashish Pradhan wrote: Hello, Currently, the meetings take place in the *#hurd channel every Thursday at 19:00 UTC* and are open to any interested party. Are these held regularly? How long do they last? Can anyone drop in and discuss their queries related to the project? That was done for the summer of code, and it will probably be done again next time. And yes, in the mean time, just come on the channel and ask your questions. But keep in mind that, in accordance with Hurd and IRC traditions, we may be long to answer, in particular during work/week ends/holidays, so let your client lurk there a few hours/days at least. -- Richard Braun
Re: [RFC] kern: simple futex for gnumach (version 11)
)) != SMUTEX_UNLOCKED) {
+ if (c != SMUTEX_WAITERS)
+ c = __sync_lock_test_and_set(smutex-value,
SMUTEX_WAITERS);
+ while (c != SMUTEX_UNLOCKED) {
+ futex_wait(current_thread()-task,
(vm_offset_t)smutex-value, SMUTEX_WAITERS, 0, 1);
+ c = __sync_lock_test_and_set(smutex-value,
SMUTEX_WAITERS);
+ }
+ }
+}
+
+void simple_mutex_unlock(struct simple_mutex *smutex)
+{
+ if (__sync_lock_test_and_set(smutex-value, smutex-value - 1) !=
SMUTEX_NO_WAITERS) {
+ smutex-value = SMUTEX_UNLOCKED;
+ futex_wake(current_thread()-task, (vm_offset_t)smutex-value,
0);
+ }
+}
Userspace tests please.
+struct futex;
+
+void futex_setup(void);
+
+/*
+ * This is a method for a thread to wait for a change of value at a given
address.
+ * If msec is non-zero, thread blocks for msec amount of time. If it's zero,
no
+ * timeout is used. If private_futex is one, futex is not shared between
tasks.
+ */
+void futex_wait(task_t task, vm_offset_t address, int value, int msec,
boolean_t private_futex);
s/one/true/
+/*
+ * This is a method for waking up one or all threads waiting for a change of
+ * value at a given address.
+ *
+ * If wake_all is one, all threads are awakened. If it's zero, only one
thread is
+ * awakened.
+ */
+void futex_wake(task_t task, vm_offset_t address, boolean_t wake_all);
This requires more description. For example, state the address
requirements (pointing and aligned to an int).
--
Richard Braun
Re: [RFC] kern: simple futex for gnumach (version 11)
On Thu, Jan 09, 2014 at 02:36:18PM -0200, Diego Nieto Cid wrote:
2014/1/8 Marin Ramesa m...@hi.t-com.hr
+
+static unsigned long futex_init(task_t task, vm_offset_t address,
boolean_t private_futex, struct futex *futex)
+{
+ unsigned long node_slot = 0;
+
+ futex = (struct futex *)kalloc(sizeof(*futex));
This only changes the value of the parameter. futex could very well
be a local variable instead.
If what you want is to update the value seen by the caller you should
declare futex as a struct futex ** and then do something like this:
*futex = (struct futex *)kalloc(sizeof(**futex));
Right, this was already mentioned in an earlier review, and is still
there. Fix it.
+void futex_wait(task_t task, vm_offset_t address, int value, int /* TODO
Use time_value_t */ msec, boolean_t private_futex)
+{
+ unsigned long node_slot = 0;
+
+ node_slot = futex_lookup_address(address);
+ if (node_slot == 0) {
+ if (private_futex) {
+ pfutexes = (struct futex *)kalloc(sizeof(pfutexes));
+ if (pfutexes == NULL)
+ return;
+ node_slot = futex_init(task, address, TRUE,
pfutexes[ARRAY_SIZE(pfutexes) - 1]);
I don't really get this stuff.
First of all, you are allocating sizeof(pfutexes) and pfutexes is
defined as struct futex *. Thus, you are just allocating memory for
a pointer (4 bytes in a 32-bit machine) and then casting it to a
pointer to struct futex. So, if you dereference pfutexes to access
some member of the futex structure you will probably go past the end
of the allocated space (for the futex structure is bigger than a
pointer).
I agree. this array isn't needed, get rid of it.
Secondly, you are calling the macro ARRAY_SIZE with pfutexes as
parameter. This will be evaluated to 0. Here's why:
Simply put, ARRAY_SIZE() is intended for statically allocated arrays
where the size is known at compile time. Don't use it on pointers.
But again, since there shouldn't be an array in the first place, this
function should be entirely reworked.
--
Richard Braun
Re: [RFC] kern: simple futex for gnumach (version 11)
On Thu, Jan 09, 2014 at 05:06:09PM +0100, Marin Ramesa wrote:
On 01/09/2014 03:45:35 PM, Richard Braun wrote:
+/* TODO Should be per-task. */
+static struct futex *pfutexes;
Most locks are private, so yes, do work on that too.
I actually did not implement this because I don't know how.
How to make a variable visible only and private to a task?
I tried to find code examples of this but I don't know what
should I look for.
Just as tasks have IPC spaces (struct ipc_space *itk_space in struct
task), you could create a futex space and add it to the task
structure. It's as simple as that. I said per task, not only visible
to one task.
+static vm_offset_t futex_gen_unique_event_num(vm_offset_t sum,
vm_offset_t add1, vm_offset_t add2)
+{
+ if (sum == 0) return 0;
+
+ struct rbtree_node *node;
+
+ for(node = futex_tree.root; node != NULL; node =
node-children[RBTREE_RIGHT]) {
+
+ struct futex *futex;
+ futex = rbtree_entry(node, struct futex, node);
+
+ vm_offset_t futex_event_num = (vm_offset_t)futex-event;
+
+ if (futex_event_num == sum) {
+
+ if (futex_event_num - add2 == add1)
+ continue;
+ else
+ return futex_gen_unique_event_num(sum - 1,
add1, add2);
+
+ }
+ }
+
+ return sum;
+}
??
Consider a case where you have an address/map or an object
address/offset pair
of (x, y) and another one (z, w) where x != z and y != w, but x + y
= z + w. So,
in order for the events to work correctly I need to make the sum
unique. But if
I implement the trees differently this function might become reduntant.
That's the idea, use one global tree for shared futexes, per task trees
for private ones.
+ if (private_futex) {
+ pfutexes = (struct futex *)kalloc(sizeof(pfutexes));
+ if (pfutexes == NULL)
+ return;
+ node_slot = futex_init(task, address, TRUE,
pfutexes[ARRAY_SIZE(pfutexes) - 1]);
+ } else {
+ shared_futexes = (struct futex
*)kalloc(sizeof(shared_futexes));
+ if (shared_futexes == NULL)
+ return;
+ node_slot = futex_init(task, address, FALSE,
shared_futexes[ARRAY_SIZE(shared_futexes) - 1]);
+ }
+ if (node_slot == 0) return;
+ }
+
+ if (__sync_bool_compare_and_swap((int *)address, value,
value)) {
What are you trying to do here ?? This call has no particular effect,
except being a memory barrier ...
Shouldn't the compare be atomic. Maybe I don't understand what
atomic really
means but the GCC manual says this function is. Or is it enough to
hold the lock.
From the GCC manual :
These built-in functions perform an atomic compare and swap. That is,
if the current value of *ptr is oldval, then write newval into *ptr.
So tell us what the point of replacing oldval with itself is.
In addition, use __sync_val_compare_and_swap please.
+ if (msec != 0) {
+
+ simple_lock(futex_shared_lock);
+
+ thread_timeout_setup(current_thread());
+
+ assert_wait(NULL, TRUE);
+ thread_will_wait_with_timeout(current_thread(),
(unsigned
int)msec);
+
+ simple_unlock(futex_shared_lock);
+
+ thread_block(NULL);
+
+ return;
+
+ }
This can't work... You're completely isolating the bounded wait case
from the unbounded one. This is merely a sleep, not waiting for
anything.
But it works. I tested it. How do I rewrite this?
Oh I do believe the sleep works... But think again, this code isn't
waiting on any event at all, so how do you wake up the thread ??
Look at the implementation of thread_sleep to understand how to merge
this into the unbounded case.
+ simple_lock(pfutex-futex_private_lock);
+
+ pfutex-num_waiters++;
+
+ pfutex-event = (event_t)futex_gen_unique_event_num
+ ((vm_offset_t)pfutex-map + address,
(vm_offset_t)pfutex-map, address);
Use the address of the futex object in kernel memory for events, this
is the traditional way of using them.
I can't because currently I can have two futexes with the same event
value. But
if I implement the tree differently maybe this will be possible. I
can get rid of
futex_gen_unique_event_num().
Yes, do that.
--
Richard Braun
Re: [RFC] kern: simple futex for gnumach (version 11)
On Thu, Jan 09, 2014 at 05:52:21PM +0100, Richard Braun wrote: On Thu, Jan 09, 2014 at 05:06:09PM +0100, Marin Ramesa wrote: +simple_lock(futex_shared_lock); + +thread_timeout_setup(current_thread()); + +assert_wait(NULL, TRUE); +thread_will_wait_with_timeout(current_thread(), (unsigned int)msec); + +simple_unlock(futex_shared_lock); + +thread_block(NULL); By the way, why is thread_timeout_setup called here ? It really looks wrong here. -- Richard Braun
Re: [RFC] kern: simple futex for gnumach (version 11)
On Thu, Jan 09, 2014 at 09:51:51PM +0100, Marin Ramesa wrote: On 01/09/2014 05:52:21 PM, Richard Braun wrote: On Thu, Jan 09, 2014 at 05:06:09PM +0100, Marin Ramesa wrote: Shouldn't the compare be atomic. Maybe I don't understand what atomic really means but the GCC manual says this function is. Or is it enough to hold the lock. From the GCC manual : These built-in functions perform an atomic compare and swap. That is, if the current value of *ptr is oldval, then write newval into *ptr. So tell us what the point of replacing oldval with itself is. I do a swap so it returns true if they are equal. I'll say it again because it doesn't look like you got it : when I point something out to you, consider you're wrong, and make the effort to question your assumptions. You may end up being right at times, but most often not. Now please, read your code again, then answer the question I asked : So tell us what the point of replacing oldval with itself is. In addition, use __sync_val_compare_and_swap please. Don't you think it's better this way. The function already returns a boolean. What would I do with the contents of *ptr before the operation. It doesn't tell me if the swap was successful. I would need to introduce a new variable. Actually it does, but since you probably didn't understand CAS, it's not so surprising you're thinking that. Now, after reading the code again, I'm not yet sure how to correctly deal with the previous value, and will have to read about futexes a bit more. The boolean version should do for now. -- Richard Braun
Re: [RFC] kern: simple futex for gnumach (version 11)
On Wed, Jan 08, 2014 at 08:43:28PM +0100, Marin Ramesa wrote: First threads were blocked and awakened when called from userspace. :) Timed waits work as expected. Private and shared futexes always block threads when called from futex_wait(). This is now ready for test. What needs to be tested is: This does look much better, but there is still quite some work to be done before it gets right. I'll send a detailed review later. -- Richard Braun
Re: [PATCH 2/2] Align kmem_cache objects using __cacheline_aligned
On Thu, Jan 09, 2014 at 12:40:49AM +0100, Samuel Thibault wrote: Samuel Thibault, le Thu 09 Jan 2014 00:39:30 +0100, a écrit : Mmm, perhaps it would be more useful to define a typedef with the attribute, and use it instead of struct kmem_cache? Mmm, from slab.h it would seem that you can apply the attribute directly to the structure, thus even avoiding the typedef altogether? Yes, that's the preferred way. -- Richard Braun
Re: Valgrind porting
On Mon, Jan 06, 2014 at 01:27:08AM +0530, Subhashish Pradhan wrote: Has anyone started working on porting valgrind to hurd? If not, I'd like to take up the task (and for GSOC). What would be the essential skills that would be required for this task? Shell scripting, understanding of C, and anything else? Operating system knowledge from the userspace point of view, essentially system calls, address space layout, and a decent grasp on the Mach and Hurd concepts. -- Richard Braun
Re: [PATCH 4/4] kern: optimize the layout of struct kmem_cache
On Mon, Jan 06, 2014 at 11:47:10AM +0100, Justus Winter wrote: No, but I profiled this. This is the output of pahole (on the left) and the results of my profiling on the right. The rightmost number is the access count. This is the situation before my patch: This looks fine to me, although I'm usually not keen on micro optimizations before fixing system encompassing design issues :). -- Richard Braun
Re: [RFC] kern: simple futex for gnumach (version 10)
);
External users don't care whether there is a tree and a lock.
+/*
+ * This is a method for a thread to wait for a change of value at a given
address.
+ *
+ * Function performs a lookup of the address in the futex tree. If address is
+ * found, value at the address is atomically compared with the argument
value.
+ * If the values are same, offset from the task's map and futex address is
+ * calculated and thread blocks.
+ */
+int futex_wait(int *address, int value);
Again, external users don't care about the implementation, only the
behaviour they can expect.
+/*
+ * This is a method for waking up one or all threads waiting for a change of
+ * value at a given address.
+ *
+ * Function performs a lookup of the address in the futex tree. If address is
+ * found and wake_all argument is TRUE, all threads with the same offsets are
+ * awakened. If wake_all is FALSE, only one thread from the futex is
awakened.
+ */
+int futex_wake(int *address, _Bool wake_all);
Likewise.
+/* RPCs to call from userspace. */
+int r_futex_wait(task_t task, pointer_t address, int value);
+int r_futex_wake(task_t task, pointer_t address, boolean_t wake_all);
?
+/*
+ * The idea is that each thread calls sync_circle_wait() with the same
object as
+ * the argument. Then a thread outside of the sync circle calls
sync_circle_signal()
+ * to send a wakeup signal. Function sync_circle_signal() first modifies the
sync_circle's
+ * value, which closes the sync circle and then calls futex_wake().
+ */
+struct sync_circle {
+
+ int value;
+
+};
+
+#define decl_sync_circle(class, name) \
+class struct sync_circle name;
+
+void sync_circle_init(struct sync_circle *scircle);
+int sync_circle_wait(struct sync_circle *scircle);
+int sync_circle_signal(struct sync_circle *scircle);
+
+/* Simple mutex implementation based on futex calls and GCC builtins. */
+struct simple_mutex {
+
+ #define SMUTEX_UNLOCKED 0
+ #define SMUTEX_NO_WAITERS 1
+ #define SMUTEX_WAITERS 2
+
+ int value;
+
+};
+
+#define decl_simple_mutex(class, name) \
+class struct simple_mutex name;
+
+void simple_mutex_init(struct simple_mutex *smutex);
+void simple_mutex_lock(struct simple_mutex *smutex);
+void simple_mutex_unlock(struct simple_mutex *smutex);
What does all this have antyhing to do in the public header ?
@@ -158,6 +159,8 @@ void setup_main(void)
recompute_priorities(NULL);
compute_mach_factor();
+ futex_setup();
+
Well, you got that one right, but I suppose it won't be needed once all
the other changes are made, since there should be no globally shared
structure after that point.
--
Richard Braun
Re: [RFC] kern: simple futex for gnumach (version 10)
On Mon, Jan 06, 2014 at 12:34:32PM +0100, Richard Braun wrote: +static struct rbtree futex_tree; http://lists.gnu.org/archive/html/bug-hurd/2013-12/msg00545.html : Personally, I'd use a per-task red-black tree. http://lists.gnu.org/archive/html/bug-hurd/2013-12/msg00546.html : To finish with, the more I think about it, the less I understand why there is a need for futex objects in the first place. Instead of dynamically allocating such structures, it probably makes more sense to use the addresses of the physical page descriptors instead. ^^ After some thinking, this would actually not work, as physical pages can be paged out. You can use a (VM object, offset) pair instead, but you have to deal with private futexes, which should probably use a (VM map, address) pair. So, you actually need a global, shared container for process shared futexes, and per-task containers for private ones. We also need to make sure nothing bad happens to an (object, offset) pair when, say, copy-on-write is performed by the VM system... All this looks quite tricky to me, and certainly not a small hack entry :-/. -- Richard Braun
Re: [RFC] kern: simple futex for gnumach (version 8)
On Tue, Dec 31, 2013 at 04:26:01PM +0100, Richard Braun wrote:
On Sun, Dec 29, 2013 at 09:44:51PM +0100, Marin Ramesa wrote:
+ simple_lock(futex-futex_wait_lock);
+
+ /* If the value is still the same. */
+ if (*address == value) {
In addition, such an access should go through some userspace memory
accessor (something like copyin at least, although passing it through
a message would be far cleaner), and it must be atomic. To finish with,
the more I think about it, the less I understand why there is a need
for futex objects in the first place. Instead of dynamically allocating
such structures, it probably makes more sense to use the addresses of
the physical page descriptors instead.
--
Richard Braun
Re: [RFC] kern: simple futex for gnumach (version 6)
On Sat, Dec 28, 2013 at 02:30:01AM +0100, Marin Ramesa wrote:
On 12/27/2013 07:14:40 PM, Richard Braun wrote:
+void futex_cross_address_space_wake(futex_t futex, boolean_t
wake_all)
+{
+ #define min(x, y) (x = y ? x : y)
+
+ queue_iterate(futex-chain, futex, futex_t, chain) {
+
+ simple_lock(futex-futex_wait_lock);
+
+ int i;
+
+ for (i = 0; i min(futex-num_futexed_threads,
+ ((futex_t)futex-chain.next)-num_futexed_threads);
i++) {
If you have a list, you just walk it, there is no need to count the
number of items.
Threads are not in a list and I need to count the number of threads
to use indexes for comparison of offsets.
+void futex_wake_threads(futex_t futex, boolean_t wake_all)
+{
+ if (wake_all) {
+ int i;
+ for (i = 0; i futex-num_futexed_threads; i++)
+ futex_wake_one_thread(futex, i);
+ } else
+ futex_wake_one_thread(futex, futex-num_futexed_threads-1);
+}
What's the difference between this and
futex_cross_address_space_wake ??
futex_cross_address_space_wake() wakes all threads with the same
offset. It
doesn't matter in which futex they are in. This function is just for one
futex, it doesn't matter which offsets the threads have.
Here is how it probably should be :
There should be one futex_waiter struct per thread, allocated on the
stack when a thread is about to wait. That structure is then queued on
the futex it's waiting for, using the address of the futex as the
wakeup event. When another thread wakes the futex, it walks the list
of futex_waiters, and for each of them, wakes up the associated thread.
No need to count anything here, just a simple list to walk, and no
need to have distinct cases between cross address space wakeups or not.
What makes a futex cross address space is simply that its a non first
class kernel object, just like ports, referenced with a (map, address)
pair. Once in the kernel, it's merely a wait queue.
--
Richard Braun
Re: [RFC] kern: simple futex for gnumach (version 6)
allocated in futex_wait ? In that case, the target thread might end up
trying to use memory that has already been freed by the waker. And this
is the kind of mistakes that someone with proper experience with
concurrency would not do.
I may already have said that, but better safe than sorry so: I don't
see the point of maintaining the number of threads.
+void futex_cross_address_space_wake(futex_t futex, boolean_t wake_all)
+{
+ #define min(x, y) (x = y ? x : y)
+
+ queue_iterate(futex-chain, futex, futex_t, chain) {
+
+ simple_lock(futex-futex_wait_lock);
+
+ int i;
+
+ for (i = 0; i min(futex-num_futexed_threads,
+ ((futex_t)futex-chain.next)-num_futexed_threads);
i++) {
If you have a list, you just walk it, there is no need to count the
number of items.
+
futex_wake_one_thread((futex_t)futex-chain.next, i);
+
+ if
(((futex_t)futex-chain.next)-num_futexed_threads == 0) {
+ simple_unlock(futex-futex_wait_lock);
+
simple_unlock(((futex_t)futex-chain.next)-futex_wait_lock);
+
futex_hash_table_delete_futex((futex_t)futex-chain.next);
+ #undef min
+ return;
+ }
+ }
+ }
+
+ simple_unlock(((futex_t)futex-chain.next)-futex_wait_lock);
+ simple_unlock(futex-futex_wait_lock);
+
+ }
+
+#undef min
This isn't C, it's a preprocessor directive, and you've already
undefined min earlier.
+void futex_wake_threads(futex_t futex, boolean_t wake_all)
+{
+ if (wake_all) {
+ int i;
+ for (i = 0; i futex-num_futexed_threads; i++)
+ futex_wake_one_thread(futex, i);
+ } else
+ futex_wake_one_thread(futex, futex-num_futexed_threads-1);
+}
What's the difference between this and futex_cross_address_space_wake ??
+unsigned int futex_hash(int *address)
+{
+ unsigned int hash_val = 0;
+
+ hash_val = (unsigned int)address + (hash_val 5) - hash_val;
+
+ if (table != NULL) {
+ unsigned int ret = hash_val % table-size;
A modulo is expensive. Make the hash table size a power-of-two and use
an AND.
+int futex_hash_table_add_address(int *address)
+{
+ futex_t new_futex;
+
+ unsigned int hash_val = futex_hash(address);
+
+ if (table != NULL) {
+ simple_lock(table-futex_hash_table_lock);
+ }
+
+ if ((new_futex = (futex_t)kalloc(sizeof(struct futex))) == NULL) {
+ if (table != NULL)
+ simple_unlock(table-futex_hash_table_lock);
Uh ? Do you have a hash table or not ? How can this work if it's not
there ??
+ table-futex_elements[hash_val] = *new_futex;
+ table-size++;
Looks like you have one after all...
+int futex_thread_init(futex_t futex, futex_thread_t thread)
+{
+ if ((thread = (futex_thread_t)kalloc(sizeof(futex_thread_t))) == NULL)
Why do you pass thread as an argument ?? This means futex_wait is bound
to crash on its first access to the thread variable...
+ return FUTEX_RESOURCE_SHORTAGE;
+
+ thread-thread = current_thread();
+ thread-futex = futex;
+
+ *thread-map = current_map();
You don't have to store the map since you can find it through the task
of the thread.
+ if ((vm_map_lookup(thread-map, (vm_offset_t)futex-address,
VM_PROT_READ, thread-version, thread-object,
+ thread-offset, thread-out_prot,
thread-wired) != KERN_SUCCESS)) {
+ kfree((vm_offset_t)thread, sizeof(struct futex_thread));
+ return FUTEX_MEMORY_ERROR;
+ }
Futexes are only concerned with (map, address) pairs, why are
VM objects involved ?
+++ b/kern/futex.h
The public header should at least define struct futex as an opaque type.
+++ b/kern/futex_i.h
You didn't use the private header correctly. What you put into it
should instead be declared as static functions inside the .c file.
There are undoubtedly other problems but I don't have the time nor
the motivation to look through them now. If you really insist on this
task, you should really, really, REALLY get better with the basics
first. There are just too many mistakes here to do anything with that
patch.
--
Richard Braun
Re: [RFC] kern: simple futex for gnumach (version 6)
On Fri, Dec 27, 2013 at 07:55:02PM +0100, Marin Ramesa wrote: On 12/27/2013 07:14:40 PM, Richard Braun wrote: What do you mean when you say you test on the Hurd and Linux ? How do you use GDB with Mach to determine it's actually kalloc thta segfaults ? I link it with gnumach .o files and then run it trough GDB to see if I don't make big mistakes in the code. Do you mean you're expecting kalloc() to actually work in a POSIX environment ? -- Richard Braun
Re: [RFC] kern: simple futex for gnumach (version 6)
On Fri, Dec 27, 2013 at 10:23:01PM +0100, Marin Ramesa wrote: On 12/27/2013 09:58:07 PM, Richard Braun wrote: Do you mean you're expecting kalloc() to actually work in a POSIX environment ? I expected it to work on Hurd running gnumach, but I don't understand what it means to function to work in a POSIX environment. If running GNU Mach as the kernel, it should. If running it from GDB in userspace (and that's what I mean when I say a POSIX environment), there is no way it can. But it's OK, I can test the code without kalloc(). Not completely, because you won't have address spaces (not due to kalloc missing but rather because kalloc also relies on the virtual memory system, and at this point, I guess kalloc doesn't work because the environment in which you test is neither real hardware nor a virtual machine emulating the appropriate architecture, preventing the VM system from working). -- Richard Braun
Re: [RFC] kern: simple futex for gnumach (version 5)
On Thu, Dec 26, 2013 at 02:30:10PM +0100, Marin Ramesa wrote:
+ futex-address = address;
+ futex-num_futexed_threads = 0;
+ futex-next_futex = NULL;
+ futex-prev_futex = (table-futex_elements[prev_hash_val]);
+ futex-prev_futex-next_futex = futex;
+ *futex-map = current_thread()-task-map;
A futex shouldn't be a task-local object since it's used for inter
process synchronization. There should be per-thread objects (probably
allocated on the stack since a thread can only be doing one wait at
a time), and these per-thread objects should be listed in the global
futex object. Waking up the futex then simply walks the list and wakes
up threads.
+ if ((vm_map_lookup(futex-map, (vm_offset_t)address, VM_PROT_READ,
futex-version, futex-object,
+ futex-offset, futex-out_prot, futex-wired)
!= KERN_SUCCESS)) {
+ unsigned int temp_hash_val = futex_hash(address);
+ __builtin_free((table-futex_elements[temp_hash_val]));
Why __builtin_malloc and __builtin_free ??
+ if (futex-num_futexed_threads == 128)
+ return FUTEX_RESOURCE_SHORTAGE;
I assume this limit is temporary. If not, remove it, there is no reason
to add a constraint like this one.
+ suspend:
+ assert_wait(current_thread()-state , FALSE);
+ simple_unlock(futex-futex_wait_lock);
+ kern_return_t ret = thread_suspend(current_thread());
I really doubt assert_wait can be used with thread_suspend and
thread_resume... Use thread_block and thread_wakeup instead. Again, be
more careful.
+struct futex {
As previously said, rework that structure.
+ /* Next futex in queue. */
+ futex_t next_futex;
+
+ /* Previous futex in queue. */
+ futex_t prev_futex;
Do NOT reimplement generic doubly-linked lists...
+ /* Number of futexed threads at the same address. */
+ unsigned int num_futexed_threads;
+
+ /* Array of futexed threads at the same address. */
+ //thread_t futexed_threads;
+
+ thread_t futexed_threads[128];
Ugh. No.
+int futex_init(futex_t futex, int *address);
+extern int futex_wait(int *address, int value);
+extern int futex_wake(int *address, boolean_t wake_all);
+void futex_cross_address_space_wake(futex_t futex, boolean_t wake_all);
+void futex_wake_threads(futex_t futex, boolean_t wake_all);
+int futex_hash_table_init(void);
+unsigned int futex_hash(int *address);
+futex_t futex_hash_table_lookup_address(int *address);
+int futex_hash_table_add_address(int *address);
+void futex_hash_table_delete_futex(futex_t futex);
I know this isn't the traditional way to do it in Mach, but please,
extensively document the API in the header, e.g. as it's done for the
slab allocator. I also suggest moving everything not public (such as
the hash table) in the .c file, and if there is anything private that
still needs to be in a header, move that to a _i.h file (i could mean
internal/implementation/inline/whatever), so that the main header only
documents the public API.
--
Richard Braun
Re: [RFC] kern: simple futex for gnumach (version 5)
On Thu, Dec 26, 2013 at 02:58:01PM +0100, Richard Braun wrote: I know this isn't the traditional way to do it in Mach, but please, extensively document the API in the header, e.g. as it's done for the slab allocator. I also suggest moving everything not public (such as Actually, kern/rbtree.h is a better example concerning the separation of private/public stuff. -- Richard Braun
Re: [RFC] kern: simple futex for gnumach (version 5)
On Thu, Dec 26, 2013 at 03:15:24PM +0100, Marin Ramesa wrote: I get segfaults in kalloc(). I don't know what I'm doing wrong. Show us how you use it. -- Richard Braun
Re: [RFC] kern: simple futex for gnumach (version 5)
On Thu, Dec 26, 2013 at 03:51:30PM +0100, Marin Ramesa wrote: futex-futexed_threads = (thread_t)kalloc((futex-num_futexed_threads+1)*sizeof(struct thread)); When futexed_threads is a pointer to struct thread. Why are you allocating a thread structure ?? I strongly suspect kalloc does what you want it to do, but something in your code makes an illegal access. In the worst case, use prints to find the faulty instruction. -- Richard Braun
[PATCH] Implement thread destruction
This change makes libpthread release almost every resource allocated for
a thread, including the kernel thread, its send right, its reply port
and its stack. This improves resource usage after peaks of activity
during which servers can create hundreds or even thousands of threads.
To achieve it, the library relies on the recently added
thread_terminate_release one-way GNU Mach RPC, which allows threads to
release their last resources along with terminating in a single
operation. The pthread_exit function unconditionally releases all the
resources it can, including other kernel objects (namely the port used
for blocking and waking up) and signal states. When releasing the
pthread structure, a reference counter is used so that joinable threads
remain available. Once the reference counter drops to 0, the pthread
structure can be recycled. Thread local storage (TLS) is also recycled
since it needs to remain allocated while terminating the thread, as it
is there that the reply port is stored. TLS could be released too, after
grabbing the reply port name, but it is difficult to make sure no RPC
involving a reply port is used afterwards, so the simpler solution of
recycling TLS was chosen.
This patch can be applied on top of the master-threadvars branch, as
a few commits are still not present in master.
---
Makefile| 2 +-
pthread/pt-alloc.c | 21 ++
pthread/pt-create.c | 69 +++
pthread/pt-dealloc.c| 5 +++
pthread/pt-detach.c | 24 +++
pthread/pt-exit.c | 35
pthread/pt-internal.h | 36 ++---
pthread/pt-join.c | 18 +
sysdeps/mach/hurd/pt-sigstate-destroy.c | 2 +-
sysdeps/mach/hurd/pt-sigstate-init.c| 18 -
sysdeps/mach/hurd/pt-sysdep.c | 5 +++
sysdeps/mach/hurd/pt-sysdep.h | 3 +-
sysdeps/mach/pt-thread-alloc.c | 31 --
sysdeps/mach/pt-thread-dealloc.c| 3 +-
sysdeps/mach/pt-thread-halt.c | 37 -
sysdeps/mach/pt-thread-start.c | 4 +-
sysdeps/mach/pt-thread-terminate.c | 72 +
17 files changed, 194 insertions(+), 191 deletions(-)
delete mode 100644 sysdeps/mach/pt-thread-halt.c
create mode 100644 sysdeps/mach/pt-thread-terminate.c
diff --git a/Makefile b/Makefile
index c57f1a0..61e59fc 100644
--- a/Makefile
+++ b/Makefile
@@ -121,7 +121,7 @@ libpthread-routines := pt-attr pt-attr-destroy
pt-attr-getdetachstate \
pt-thread-alloc \
pt-thread-dealloc \
pt-thread-start \
- pt-thread-halt \
+ pt-thread-terminate \
pt-startup \
\
pt-getconcurrency pt-setconcurrency \
diff --git a/pthread/pt-alloc.c b/pthread/pt-alloc.c
index 604d376..aa0c9e2 100644
--- a/pthread/pt-alloc.c
+++ b/pthread/pt-alloc.c
@@ -47,7 +47,7 @@ struct __pthread *__pthread_free_threads;
pthread_mutex_t __pthread_free_threads_lock;
static inline error_t
-initialize_pthread (struct __pthread *new, int recycling)
+initialize_pthread (struct __pthread *new)
{
error_t err;
@@ -55,6 +55,7 @@ initialize_pthread (struct __pthread *new, int recycling)
if (err)
return err;
+ new-nr_refs = 1;
new-cancel_lock = (pthread_mutex_t) PTHREAD_MUTEX_INITIALIZER;
new-cancel_hook = NULL;
new-cancel_hook_arg = NULL;
@@ -62,14 +63,6 @@ initialize_pthread (struct __pthread *new, int recycling)
new-cancel_type = PTHREAD_CANCEL_DEFERRED;
new-cancel_pending = 0;
- if (recycling)
-/* Since we are recycling PTHREAD, we can assume certains things
- about PTHREAD's current state and save some cycles by not
- rewriting the memory. */
-return 0;
-
- new-stack = 0;
-
new-state_lock = (pthread_mutex_t) PTHREAD_MUTEX_INITIALIZER;
new-state_cond = (pthread_cond_t) PTHREAD_COND_INITIALIZER;
@@ -117,12 +110,6 @@ __pthread_alloc (struct __pthread **pthread)
if (new)
{
- /* The thread may still be running. Make sure it is stopped.
-If this is the case, then the thread is either at the end of
-__pthread_dealloc or in __pthread_thread_halt. In both
-cases, we are interrupt it. */
- __pthread_thread_halt (new);
-
#ifdef ENABLE_TLS
if (new-tcb)
{
@@ -132,7 +119,7 @@ __pthread_alloc (struct __pthread **pthread)
}
#endif /* ENABLE_TLS */
- err = initialize_pthread (new, 1);
+
Re: [PATCH] Implement thread destruction
On Thu, Dec 26, 2013 at 05:44:56PM +0100, Samuel Thibault wrote: loses, actually :) (and ditto further down) Right, thanks. -- Richard Braun
Re: [PATCH] kern: simple futex for gnumach (version 4)
On Sun, Dec 22, 2013 at 03:58:55PM +0100, Marin Ramesa wrote: +simpleroutine futex_rpc( + thread : thread_t; + address : pointer_t; + value : int; + operation : int); + +simpleroutine futex_wait_rpc( + thread : thread_t; + address : pointer_t; + value : int); + +simpleroutine futex_wake_rpc( + thread : thread_t; + address : pointer_t; + thread_num : int); Obviously, you rushed your work without trying to understand what you were doing, and this is becoming frustrating. Don't blindly and mindlessly copy the first thing you see around... 1/ A simpleroutine is a one-way RPC, without a return value other than that of the message transmission. 2/ Futex calls should operate on futexes, not threads. If you actually only want to specify addresses in the calls, use a task_t as first parameter, because there is no risk of right reference leak when calling mach_task_self, whereas there are with mach_thread_self. And again, I insist on *not* creating ioctl-style calls with an operation parameter. The operation is the RPC itself. Also, AIUI, the operations are wait and wake, so why create a third futex RPC ? To finish with, don't suffix RPCs with _rpc ... Personal question: why are you working on this ? Considering the beginner mistakes you did at the C level itself, and that you've never even run a real GNU Mach instance, what makes you think you have the proper experience to work on such a low-level tool that involves both virtual memory and concurrency, two of the most difficult domains in computer science ? -- Richard Braun
Re: [PATCH] kern: simple futex for gnumach (version 4)
On Tue, Dec 24, 2013 at 12:52:23PM +0100, Marin Ramesa wrote: I need to start somewhere. I really want to learn how kernels are programmed. This task was listed in the small hacks entry, I really thought it would be a small hack. That's probably a mistake on our part. There is a one call to vm_map lookup to retrive the offset and the object. Simple How do you implement cross address space synchronization ? locks are used and there are calls to thread_resume() and thread_suspend(). I don't see anything so much difficult that I can't learn while doing it. Well, using thread_resume/thread_suspend might work for non preemptible kernels, but not for a preemptible one, or a multiprocessor one like Mach was. Look at your call to thread_suspend. If thread_resume is called after the futex lock is released (it can't be called before since it's called with that same lock reacquired), but before thread_suspend is called, the thread will miss its wakeup. This is why interlocks (usually hidden in wait queues) are normally used. See assert_wait(). But I won't work on this if it's frustrating. I don't want to cause any trouble. For now, I'm the only one reacting to this, not much of a trouble. But I'm pretty sure we would all like contributors to pay great care to what they're doing, whatever their technical level. -- Richard Braun
Re: [PATCH] kern: simple futex for gnumach (version 4)
On Tue, Dec 24, 2013 at 04:55:47PM +0100, Marin Ramesa wrote: I use a recursive futex_wake(). It first scans all the futexes if they are on the same offset and if they share the same vm_object. If they do, recursion is used to wake a number of threads in those futexes. The number of threads awakened is constant troughout the same offsets and in the same objects. In this way synchronization is possible. I don't do anything in futex_wait(). Right. Well, consider how small stacks usually are in a kernel, and you should understand why anything recursive (except if bounded) is unacceptable. Then, why a scan through vm_objects ? Why not directly find the one futex associated with a (map, address) and wake all threads waiting on it ? Also, why pass a number of threads to wake ? Why not simply choose between one or all ? -- Richard Braun
Re: [PATCH] kern: simple futex for gnumach (version 4)
On Tue, Dec 24, 2013 at 05:38:59PM +0100, Marin Ramesa wrote: I have read in Futexes are tricky that futex() is used with INT_MAX argument. I want to keep that usage. So, if a number of threads to wake is greater than number of threads in a futex I wake all. And it also says everything else than 1 and INT_MAX make little sense in practice. Please, take the habit to re-read and reconsider every time I (or someone else from the community) questions you about something. Don't wait for us to reply to start thinking... I'm saying this because almost every exchange we've had until now is 1/ you do something wrong 2/ I tell you it's wrong 3/ you justify why you did something wrong without making the effort to reconsider 4/ I show you it's definitely wrong 5/ you end up agreeing. Let's make it a 3-step process. -- Richard Braun
Re: [PATCH] kern: simple futex for gnumach (version 3)
On Sun, Dec 22, 2013 at 11:56:39PM +0100, Marin Ramesa wrote: Can you please show me the gnumach menuentry generated by grub? How about looking at the documentation ? For example http://www.gnu.org/software/grub/manual/grub.html#GNU_002fHurd -- Richard Braun
Re: [PATCH] kern: simple futex for gnumach (version 3)
On Sun, Dec 22, 2013 at 11:56:39PM +0100, Marin Ramesa wrote: On 22/12/13 22:04:15, Richard Braun wrote: Whether it's in a virtual machine or a real one doesn't matter at all. On Debian, simply copy the gnumach binary to /boot and run update-grub. You'll get a new entry at boot time for your kernel. I don't have the update-grub command (I'm not using Debian). So I have to modify files by hand and when I write 'multiboot /boot/gnumach.gz' I get a 'invalid magic number' error at boot time. Can you please show me the gnumach menuentry generated by grub? Now that I think about it, you should already have at least one such entry on your system, whether Debian or not. Or are you saying you're not even running the Hurd at all ?! -- Richard Braun
Re: [PATCH] kern: simple futex for gnumach (version 3)
On Sun, Dec 22, 2013 at 03:32:42PM +0100, Marin Ramesa wrote: On 22.12.2013 01:28:37, Richard Braun wrote: On Sat, Dec 21, 2013 at 11:29:34PM +0100, Marin Ramesa wrote: On 21.12.2013 23:20:43, Richard Braun wrote: How about adding everything necessary to actually test it from userspace ? Sure, what needs to be added? I don't have the conditions to test this. I earlier wrote some proof-of-concept stuff that might be used as a test, but I don't know how. Learn about MiG and how to write RPCs. There is a lot you can find on the wiki and through search engines. If possible, don't create an ioctl-style of call, with many possible behaviours through the same entry point, but make several distinct RPCs instead, then add them to include/mach/gnumach.defs. Look at how it's done for existing RPCs, but make sure you read and understand the documentation about MiG and Mach IPC if you want to work on such low-level stuff. OK, I'm reading the documentation. In the meantime I have defined several simple RPCs for testing purposes. I will send the updated patch shortly. Test it yourself from userspace before you resubmit. -- Richard Braun
Re: [PATCH] kern: simple futex for gnumach (version 3)
On Sun, Dec 22, 2013 at 06:22:03PM +0100, Marin Ramesa wrote: On 22.12.2013 17:56:32, Richard Braun wrote: Test it yourself from userspace before you resubmit. That's the problem. I don't know how to test this. How to boot up a modified gnumach and then execute a program over it? I tried with QEMU, but I can't get it to work. Are you saying you've been sending us patches without ever testing them ? -- Richard Braun
Re: [PATCH] kern: simple futex for gnumach (version 3)
On Sun, Dec 22, 2013 at 06:57:50PM +0100, Marin Ramesa wrote: On 22.12.2013 18:25:10, Richard Braun wrote: Are you saying you've been sending us patches without ever testing them ? Yes. But that's different, that's just cleaning the code. Removing forward declarations and unused variables doesn't change the behavior. Now, I need to test this, but I don't know how. Cleaning isn't supposed to change the behaviour, but that sometimes happens, in particular with the kind of optimizations applied by a modern compiler such as gcc, so please, test before sending... -- Richard Braun
Re: [PATCH] kern: simple futex for gnumach (version 3)
On Sun, Dec 22, 2013 at 06:22:03PM +0100, Marin Ramesa wrote: On 22.12.2013 17:56:32, Richard Braun wrote: OK, I'm reading the documentation. In the meantime I have defined several simple RPCs for testing purposes. I will send the updated patch shortly. Test it yourself from userspace before you resubmit. That's the problem. I don't know how to test this. How to boot up a modified gnumach and then execute a program over it? I tried with QEMU, but I can't get it to work. Whether it's in a virtual machine or a real one doesn't matter at all. On Debian, simply copy the gnumach binary to /boot and run update-grub. You'll get a new entry at boot time for your kernel. -- Richard Braun
Re: [PATCH] kern: simple futex for gnumach (version 3)
On Sat, Dec 21, 2013 at 10:55:14PM +0100, Marin Ramesa wrote: I noticed some bugs. I'm sending a fixed patch. The prevoius version is here: http://lists.gnu.org/archive/html/bug-hurd/2013-12/msg00493.html How about adding everything necessary to actually test it from userspace ? -- Richard Braun
Re: [RFC] Simple futex for gnumach (version 2)
On Sat, Dec 21, 2013 at 11:26:37PM +0100, Marin Ramesa wrote: Why not e.g. a GPL licence, even if this function is good it might not be accepted due to this. I don't know much about licenses, I copied this from kern/slab.c. It doesn't make a difference to me what license it is as long as it has this 'lack of warranty' text. It does matter. If you look closely, there are two licenses in the file kern/slab.c. You picked up the original one, not the one covering the GNU Mach fork. Currently, you're required to assign copyright of your contributions to the FSF. The slab allocator did not originate from GNU Mach and its history is a bit messy. It is really not a good template for contributions. -- Richard Braun
Re: [PATCH] kern: simple futex for gnumach (version 3)
On Sat, Dec 21, 2013 at 11:29:34PM +0100, Marin Ramesa wrote: On 21.12.2013 23:20:43, Richard Braun wrote: How about adding everything necessary to actually test it from userspace ? Sure, what needs to be added? I don't have the conditions to test this. I earlier wrote some proof-of-concept stuff that might be used as a test, but I don't know how. Learn about MiG and how to write RPCs. There is a lot you can find on the wiki and through search engines. If possible, don't create an ioctl-style of call, with many possible behaviours through the same entry point, but make several distinct RPCs instead, then add them to include/mach/gnumach.defs. Look at how it's done for existing RPCs, but make sure you read and understand the documentation about MiG and Mach IPC if you want to work on such low-level stuff. -- Richard Braun
Re: [PATCH 1/3] ipc: avoid dereference of null pointer and quiet the GCC warning about uninitialized variable
On Wed, Dec 18, 2013 at 10:37:03AM +0100, Marin Ramesa wrote: Compiler needs to check both !a and !b. In order to evaluate !b it must evaluate b. So when the code path is that when entry is a null pointer, the evaluation of b results in a dereference of a null pointer. No, that's wrong. The and || operators are guaranteed to be evaluated left-to-right, and yield if the first operand compares equal to 0. And that's exactly why this check against NULL is done first. -- Richard Braun
Re: [PATCH 1/3] ipc: avoid dereference of null pointer and quiet the GCC warning about uninitialized variable
On Wed, Dec 18, 2013 at 10:46:40AM +0100, Richard Braun wrote: On Wed, Dec 18, 2013 at 10:37:03AM +0100, Marin Ramesa wrote: Compiler needs to check both !a and !b. In order to evaluate !b it must evaluate b. So when the code path is that when entry is a null pointer, the evaluation of b results in a dereference of a null pointer. No, that's wrong. The and || operators are guaranteed to be evaluated left-to-right, and yield if the first operand compares equal to 0. And that's exactly why this check against NULL is done first. Compares equal to 0 for , and not equal to 0 for ||, obviously. -- Richard Braun
Re: [PATCH 1/3] ipc: avoid dereference of null pointer and quiet the GCC warning about uninitialized variable
On Wed, Dec 18, 2013 at 10:55:47AM +0100, Marin Ramesa wrote: On 18.12.2013 10:46:40, Richard Braun wrote: No, that's wrong. The and || operators are guaranteed to be evaluated left-to-right, and yield if the first operand compares equal to 0. And that's exactly why this check against NULL is done first. In the expression (!a !b), if !a equals 0, the compiler must check !b == 0 in order to return TRUE. If !a equals 0, that means the entry is a null pointer, and evaluation of !b is a dereference of a null pointer. The expression is ((a == NULL) || a-something), and I agree it is equivalent to !((a != NULL) !a-something). And again, both the and || operators are guaranteed to be evaluated left-to-right and *yield* without evaluating the second operand if the first compares or not to 0, depending on the operator. So, let's take the seconds form since that's what you've used, without the negation for simplicity : ((a != NULL) !a-something) If a isn't NULL, then it returns !a-something. If a is NULL, then (a != NULL) compares equal to 0, and returns 0 before evaluating !a-something. So no, there can't be a null pointer dereference here. And this really is basic C, so please double check your changes. -- Richard Braun
Re: [PATCH 2/3] ipc/mach_debug.c (host_ipc_hash_info): quiet GCC warning about uninitialized variable
On Wed, Dec 18, 2013 at 09:17:48AM +0100, Marin Ramesa wrote:
Don't initialize to zero, rather move the initialization of size before
the break statement. Break on the first iteration should never happen, so the
position of initialization doesn't matter.
* ipc/mach_debug.c (host_ipc_hash_info) (size): Don't initialize to zero.
(host_ipc_hash_info) (size): Move initialization before the break statement.
---
ipc/mach_debug.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/ipc/mach_debug.c b/ipc/mach_debug.c
index dd9057a..d7c454b 100644
--- a/ipc/mach_debug.c
+++ b/ipc/mach_debug.c
@@ -112,7 +112,7 @@ host_ipc_hash_info(
mach_msg_type_number_t *countp)
{
vm_offset_t addr;
- vm_size_t size = 0; /* Suppress gcc warning */
+ vm_size_t size;
hash_info_bucket_t *info;
unsigned int potential, actual;
kern_return_t kr;
@@ -127,6 +127,9 @@ host_ipc_hash_info(
for (;;) {
actual = ipc_hash_info(info, potential);
+
+ size = round_page(actual * sizeof *info);
+
if (actual = potential)
break;
@@ -135,7 +138,6 @@ host_ipc_hash_info(
if (info != *infop)
kmem_free(ipc_kernel_map, addr, size);
See that kmem_free() right here, it uses the size computed during the
previous iteration. It works as it is because ipc_hash_info returns
ipc_hash_global_size, a value that doesn't change after initialization.
But it will break if the hash table is made resizable.
- size = round_page(actual * sizeof *info);
kr = kmem_alloc_pageable(ipc_kernel_map, addr, size);
if (kr != KERN_SUCCESS)
return KERN_RESOURCE_SHORTAGE;
--
Richard Braun
Re: [PATCH 3/3] vm/vm_debug.c: quiet GCC warning about uninitialized variable
On Wed, Dec 18, 2013 at 09:17:49AM +0100, Marin Ramesa wrote: The same situation as the previous patch. Same comment as for the previous patch. -- Richard Braun
Re: [PATCH 1/3] ipc: avoid dereference of null pointer and quiet the GCC warning about uninitialized variable
On Wed, Dec 18, 2013 at 11:25:36AM +0100, Marin Ramesa wrote: Yes, vou're right. I got confused. But then something else is happening here. When I write the assertion this way: assert((entry == IE_NULL) || ((entry != IE_NULL) ? IE_BITS_TYPE (entry-ie_bits) : TRUE)); GCC stops complaing about uninitialized variable. I don't get this warning, can you tell us how you configure GNU Mach ? -- Richard Braun
Re: [PATCH 1/3] ipc: avoid dereference of null pointer and quiet the GCC warning about uninitialized variable
On Wed, Dec 18, 2013 at 11:40:09AM +0100, Marin Ramesa wrote: On 18.12.2013 11:34:03, Richard Braun wrote: I don't get this warning, can you tell us how you configure GNU Mach ? --enable-kdb --prefix= But the warning is turned off by: ipc_port_t notify_port = 0; in ipc/ipc_kmsg.c. I don't think that's a good solution. I think something else is happening here. How can a local variable in ipc_kmsg.c have any effect on another local variable in ipc_entry.c ?? -- Richard Braun
Re: [PATCH 3/4] device/chario.c: qualify pointers that access their dereferenced values under locks with __restrict__
On Tue, Dec 17, 2013 at 06:51:49PM +0100, Samuel Thibault wrote: I don't think we benefit very much here, do we? restrict is a very difficult thing to maintain, few programmers really understand what it means, I'd rather avoid introducing too many of them. I agree. Note that GNU Mach is built with -fno-strict-aliasing exactly for such reasons, as are other well known projects. Besides, the restrict keyword is only really useful when you have more than one pointer of the same type, since strict aliasing would already apply for pointers of different types. And as you mention it, the lock semantics are actually what matter most here with regard to code ordering, and they actually reduce the effectiveness of optimizations. -- Richard Braun
Re: [PATCH 3/4] device/chario.c: qualify pointers that access their dereferenced values under locks with __restrict__
On Tue, Dec 17, 2013 at 03:58:27PM +0100, Marin Ramesa wrote: Qualifier __restrict__ means that only the pointer under __restrict__ will be used to access dereferenced values. So if a code is under locks and no function is called in the critical section with pointer as an argument, it's safe to use __restrict__. This allows the compiler to make optimizations. Marin, to begin with, I'd like to thank you for the work you've done so far. It's tedious and not something people usually want to do, and yet it's important. But please, learn from experience and don't try to overengineer. Keep it simple, limit yourself to cleaning the code, not making it more complicated (and then dirtier) than it could be. -- Richard Braun
Re: [PATCH 4/4] device/chario.c (char_write): avoid segmentation fault
On Sat, Dec 14, 2013 at 01:32:27PM +0100, Marin Ramesa wrote: On 14.12.2013 12:45:32, Richard Braun wrote: I really don't see a problem in that code, you'll have to describe it better. So a pointer to io_data is cast to a pointer to a vm_map_copy structure and then passed to vm_map_copyout() which uses the vm_map_copy structure members as if the contents of the memory at the source address were vm_map_copy structure objects, not objects of io_data which is of unknown size and origin to the function. This can lead to a segmentation fault, as is shown in that short test program. My patch makes sure the io_data actually came from a vm_map_copy structure. What makes you think the content could be something else than a vm_map_copy object ? On the other hand, your patch relies on knowing the target types, although vm_map_copy_t is explicitely described as being opaque. I don't see a problem with it. There is an explicit cast to vm_map_copy_t. It's first member type is known. The target type of the cast is a pointer (a very bad historical practice in Mach that consists of typedef'ing pointers to structures in order to somehow add an abstraction level intended to make the coding style more object-oriented). In proper C, a cast means the programmer is certain about the data type. Here, it is used to call vm_map_copyout without emitting warnings. It has nothing to do with accessing the data itself, just passing it around, so no, the member type is not know, not from the point of view of modules outside the VM system. By doing that, you're violating the intended opacity of the type (see vm_map.h, vm_map_copy_t [exported; contents invisible]). The problem with that approach is that, if someone, someday, decides to change that type, he wouldn't know he would also have to change the code you introduce, and actually, he shouldn't have to. That knowledge should be completely contained in the vm_map module. and I also don't understand why you assume the data to be null- terminated. Structures are always null-terminated in memory. So if data came from a structure (like vm_map_copy) it has to be null-terminated. If not, a random '\0' will be found, sizes will not match, and function will return. No, nothing guarantees that structures are null-terminated. This is a very wrong assumption. In addition, even if it was possible for the data to be something else than a vm_map_copy (in which case we'd want an assertion, because it should *never* happen), the data size could be 0, in which case simply accessing the first byte might cause a crash. -- Richard Braun
Re: [PATCH 4/4] device/chario.c (char_write): avoid segmentation fault
On Sun, Dec 15, 2013 at 02:43:50PM +0100, Marin Ramesa wrote: On 15.12.2013 14:00:22, Richard Braun wrote: What makes you think the content could be something else than a vm_map_copy object ? Well io_data is a pointer to char, not a pointer to vm_map_copy. And there is not one member in io_req structure that keeps track of the io_data size. The function char_write() could be called with io_data having it's origin in something other than vm_map_copy. You're reasoning only on what you see at the language level. If you look more closely, you'll see this case only applies when the data hasn't been transferred in-band. I'm pretty certain the VM system will have used a vm_map_copy object to list the pages concerned in the ou-of-band case. No, nothing guarantees that structures are null-terminated. This is a very wrong assumption. In addition, even if it was possible for the data to be something else than a vm_map_copy (in which case we'd want an assertion, because it should *never* happen), the data size could be 0, in which case simply accessing the first byte might cause a crash. The tests I've run always show null-termination. But you're right, the structure could very well contain a '\0' in which case strlen() wouldn't work. But there has to be some way to detect the end of a structure in memory without knowing the types. Now you're reasoning only on what you saw with a few tests ! And no, what you want to do here makes no sense. You can't detect the end of a structure in memory. You have to know, either directly, or through some mechanism (e.G. a header containing the real type and length). -- Richard Braun
Re: [PATCH 4/4] device/chario.c (char_write): avoid segmentation fault
On Fri, Dec 13, 2013 at 09:06:55PM +0100, Marin Ramesa wrote: Check if the source of the data in the source structure memory is a target structure data. Do this by comparing the length of the char values starting with the source address until null-termination to the size of the target structure. Start by initializing the counter to the size of the first member in the target structure. Then char values in memory are counted until null-termination, and finally the counted result is compared to the size of the target structure. If the values don't match, return from the function. I really don't see a problem in that code, you'll have to describe it better. On the other hand, your patch relies on knowing the target types, although vm_map_copy_t is explicitely described as being opaque. In addition, you basically reimplement strlen inline, and I also don't understand why you assume the data to be null-terminated. -- Richard Braun
