From owner-freebsd-hackers@FreeBSD.ORG Thu Jul 31 04:12:37 2008 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id A884C106566B for ; Thu, 31 Jul 2008 04:12:37 +0000 (UTC) (envelope-from ioplex@gmail.com) Received: from nf-out-0910.google.com (nf-out-0910.google.com [64.233.182.186]) by mx1.freebsd.org (Postfix) with ESMTP id 3925C8FC08 for ; Thu, 31 Jul 2008 04:12:36 +0000 (UTC) (envelope-from ioplex@gmail.com) Received: by nf-out-0910.google.com with SMTP id h3so200122nfh.33 for ; Wed, 30 Jul 2008 21:12:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:received:received:message-id:date:from:to :subject:cc:in-reply-to:mime-version:content-type :content-transfer-encoding:content-disposition:references; bh=6KkBfp063vKfVPfpHeu0Qku81n3PuXkjQBYB4izmS/Q=; b=Hd1YVMCEukQudRk1xU59wSh9dypUa37/o8EYK6tGPvyKWI5nDoLqPBiLZa8QamMMn0 NneCksGqHYINyEkg5sycJA5W+r7HcKxPxunMBP2xCQ3Shpaa1PAFAS2l6SbmkuMVQdaC BJWxwxDPY2bDIUY0HV+ByMAPRb3pz6K3uGMAc= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=message-id:date:from:to:subject:cc:in-reply-to:mime-version :content-type:content-transfer-encoding:content-disposition :references; b=DDlo0cQledKdcyjrcI7nW1+1OoDgRzhMuEGvjth1IUHBsrk2JxZXhpV07moXBWgvQS wz7Br5Y5ZposqC/x3Udu62WW3f2JPxmCUH3GtTISYtCC+nekBF8Uzel3hjmTZJWnEnpg elwEZHfVdMX9JgWxwcCalfoe12gidGxIB7rwg= Received: by 10.210.74.17 with SMTP id w17mr7350330eba.3.1217477555920; Wed, 30 Jul 2008 21:12:35 -0700 (PDT) Received: by 10.210.109.15 with HTTP; Wed, 30 Jul 2008 21:12:35 -0700 (PDT) Message-ID: <78c6bd860807302112v22d07403hfbadfd6a9a10b0f0@mail.gmail.com> Date: Thu, 31 Jul 2008 00:12:35 -0400 From: "Michael B Allen" To: "Jilles Tjoelker" In-Reply-To: <20080718155856.GA96280@stack.nl> MIME-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Content-Disposition: inline References: <78c6bd860807121611w4f6ab44brbebfffea9929682a@mail.gmail.com> <20080718155856.GA96280@stack.nl> Cc: freebsd-hackers Subject: Re: Pls sanity check my semtimedop(2) implementation X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 31 Jul 2008 04:12:37 -0000 On Fri, Jul 18, 2008 at 11:58 AM, Jilles Tjoelker wrote: > On Sat, Jul 12, 2008 at 07:11:26PM -0400, Michael B Allen wrote: >> Below is a semtimedop(2) implementation that I'm using for FreeBSD. I >> was hoping someone could look it over and tell me if they think the >> implementation is sound. > >> [snip semtimedop implementation that uses SIGALRM and relies on EINTR] > >> The code seems to work ok but when stressing the FreeBSD build of my app >> I have managed to provoke errors related to concurrency (usually when a >> SIGALRM goes off). The Linux build works flawlessesly so I'm wondering >> about this one critical function that is different. > > In your implementation, the SIGALRM signal may happen before you even > call semop(2). If so, most likely the semop(2) will hang arbitrarily > long. Indeed. And I reconnoiter that condition is likely to happen if called with a sufficiently small timeout value. > Another dirty fix is to try non-blocking semop(2) several times with > sleeps in between. Actually that seems to work pretty well. If I force the nanosleep codepath to be used always, the application actually works pretty well under load. I wonder if the overhead of managing the signal and timer is worth the trouble. For posterity, below is the current implementation of semtimedop(2) for FreeBSD. Further ideas are welcome. void _timeval_diff(struct timeval *tv1, struct timeval *tv2, struct timeval *tvd) { tvd->tv_sec = tv1->tv_sec - tv2->tv_sec; tvd->tv_usec = tv1->tv_usec - tv2->tv_usec; if (tvd->tv_usec < 0) { tvd->tv_usec = 1000000 - tvd->tv_usec; tvd->tv_sec--; } } void signal_ignore(int s, siginfo_t *si, void *ctx) { } int _semtimedop(int semid, struct sembuf *array, size_t nops, struct timespec *_timeout) { struct timeval timeout, before, after; struct itimerval value, ovalue; struct sigaction sa, osa; int ret; if (_timeout) { timeout.tv_sec = _timeout->tv_sec; timeout.tv_usec = _timeout->tv_nsec / 1000; if (gettimeofday(&before, NULL) == -1) { errno = EFAULT; return -1; } if (timeout.tv_sec == 0 && timeout.tv_usec < 5000) { struct timeval tsleep, tend; struct sembuf wait; wait = *array; wait.sem_flg |= IPC_NOWAIT; tsleep.tv_sec = 0; tsleep.tv_usec = 1; timeradd(&before, &timeout, &tend); for ( ;; ) { struct timeval tnow, tleft; struct timespec ts; ret = semop(semid, &wait, nops); if (ret == 0) { return 0; } else if (errno != EAGAIN) { break; } if (gettimeofday(&tnow, NULL) == -1) { errno = EFAULT; break; } if (timercmp(&tnow, &tend, >=)) { errno = EAGAIN; break; } timersub(&tend, &tnow, &tleft); if (tsleep.tv_usec > tleft.tv_usec) tsleep.tv_usec = tleft.tv_usec; ts.tv_sec = 0; ts.tv_nsec = tsleep.tv_usec * 1000; nanosleep(&ts, NULL); tsleep.tv_usec *= 10; } return -1; } memset(&value, 0, sizeof value); value.it_value = timeout; memset(&sa, 0, sizeof sa); sa.sa_sigaction = signal_ignore; sa.sa_flags = SA_SIGINFO; sigemptyset(&sa.sa_mask); sigaction(SIGALRM, &sa, &osa); if (setitimer(ITIMER_REAL, &value, &ovalue) == -1) { sigaction(SIGALRM, &osa, NULL); return -1; } } ret = semop(semid, array, nops); if (_timeout) { ret = setitimer(ITIMER_REAL, &ovalue, NULL); if (ret < 0) errno = EFAULT; sigaction(SIGALRM, &osa, NULL); } if (ret == -1) { if (_timeout) { struct timeval elapsed; if (gettimeofday(&after, NULL) == -1) { } else { _timeval_diff(&after, &before, &elapsed); if (timercmp(&elapsed, &timeout, >=)) errno = EAGAIN; } } return -1; } return 0; }