From owner-svn-src-head@freebsd.org Thu Jan 7 09:03:15 2021 Return-Path: Delivered-To: svn-src-head@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 6F9544C8E77; Thu, 7 Jan 2021 09:03:15 +0000 (UTC) (envelope-from hps@selasky.org) Received: from mail.turbocat.net (turbocat.net [IPv6:2a01:4f8:c17:6c4b::2]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 4DBKyW22RZz4kTj; Thu, 7 Jan 2021 09:03:14 +0000 (UTC) (envelope-from hps@selasky.org) Received: from hps2020.home.selasky.org (unknown [178.17.145.105]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mail.turbocat.net (Postfix) with ESMTPSA id 8DED1260344; Thu, 7 Jan 2021 10:03:06 +0100 (CET) Subject: Re: svn commit: r322272 - head/sys/compat/linuxkpi/common/src To: Ryan Stone , Alexander Motin Cc: src-committers , svn-src-all , svn-src-head References: <201708081936.v78JaY4m074051@repo.freebsd.org> From: Hans Petter Selasky Message-ID: Date: Thu, 7 Jan 2021 10:02:56 +0100 User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:78.0) Gecko/20100101 Thunderbird/78.5.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Rspamd-Queue-Id: 4DBKyW22RZz4kTj X-Spamd-Bar: ---- Authentication-Results: mx1.freebsd.org; none X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[] X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 07 Jan 2021 09:03:15 -0000 On 1/6/21 9:44 PM, Ryan Stone wrote: > On Tue, Aug 8, 2017 at 3:36 PM Alexander Motin wrote: >> >> Author: mav >> Date: Tue Aug 8 19:36:34 2017 >> New Revision: 322272 >> URL: https://svnweb.freebsd.org/changeset/base/322272 >> >> Log: >> Fix few issues of LinuxKPI workqueue. >> >> LinuxKPI workqueue wrappers reported "successful" cancellation for works >> already completed in normal way. This change brings reported status and >> real cancellation fact into sync. This required for drm-next operation. >> >> Reviewed by: hselasky (earlier version) >> Sponsored by: iXsystems, Inc. >> Differential Revision: https://reviews.freebsd.org/D11904 >> > >> @@ -266,13 +267,14 @@ linux_delayed_work_timer_fn(void *arg) >> [WORK_ST_IDLE] = WORK_ST_IDLE, /* NOP */ >> [WORK_ST_TIMER] = WORK_ST_TASK, /* start queueing task */ >> [WORK_ST_TASK] = WORK_ST_TASK, /* NOP */ >> - [WORK_ST_EXEC] = WORK_ST_TASK, /* queue task another time */ >> - [WORK_ST_CANCEL] = WORK_ST_IDLE, /* complete cancel */ >> + [WORK_ST_EXEC] = WORK_ST_EXEC, /* NOP */ >> + [WORK_ST_CANCEL] = WORK_ST_TASK, /* failed to cancel */ >> }; >> struct delayed_work *dwork = arg; >> >> switch (linux_update_state(&dwork->work.state, states)) { >> case WORK_ST_TIMER: >> + case WORK_ST_CANCEL: >> linux_delayed_work_enqueue(dwork); >> break; >> default: > > I believe that this hunk introduced a regression into workqueue. > Consider the following scenario: > > 1. A delayed_work struct in the WORK_ST_TIMER state. > 2. Thread A calls mod_delayed_work() > 3. Thread B (a callout thread) simultaneously calls > linux_delayed_work_timer_fn() > > The following sequence of events is possible: > > A: Call linux_cancel_delayed_work() > A: Change state from TIMER TO CANCEL > B: Change state from CANCEL to TASK > B: taskqueue_enqueue() the task > A: taskqueue_cancel() the task > A: Call linux_queue_delayed_work_on(). This is a no-op because the > state is WORK_ST_TASK. > > As a result, the delayed_work struct will never be invoked. This is > causing address resolution in ib_addr.c to stop permanently, as it > never tries to reschedule a task that it thinks is already scheduled. > > Do you have a recommendation? Should we unconditionally > taskqueue_enqueue() when in the WORK_ST_TASK state and > linux_queue_delayed_work_on() is called? That is harmless for a > pending task but will break the deadlock if the race is lost. > Hi Ryan, Do you have a patch to fix this? --HPS