All of lore.kernel.org
 help / color / mirror / Atom feed
From: Ira Weiny <ira.weiny@intel.com>
To: Dan Williams <dan.j.williams@intel.com>,
	"Fabio M. De Francesco"
	<fabio.maria.de.francesco@linux.intel.com>,
	<linux-kernel@vger.kernel.org>
Cc: "Fabio M. De Francesco"
	<fabio.maria.de.francesco@linux.intel.com>,
	"Peter Zijlstra" <peterz@infradead.org>,
	Dan Williams <dan.j.williams@intel.com>,
	Ira Weiny <ira.weiny@intel.com>
Subject: Re: [RFC PATCH v2] cleanup: Add cond_guard() to conditional guards
Date: Tue, 30 Jan 2024 10:43:19 -0800	[thread overview]
Message-ID: <65b9434712b57_2d455e294a8@iweiny-mobl.notmuch> (raw)
In-Reply-To: <65b92b91a41a8_5cc6f29484@dwillia2-mobl3.amr.corp.intel.com.notmuch>

Dan Williams wrote:
> Fabio M. De Francesco wrote:
> > Add cond_guard() to conditional guards.
> > 
> > cond_guard() is used for the _interruptible(), _killable(), and _try
> > versions of locks. It expects a block where the failure can be handled
> > (e.g., calling printk() and returning -EINTR in case of failure).
> > 
> > As the other guards, it avoids to open code the release of the lock
> > after a goto to an 'out' label.
> > 
> > This remains an RFC because Dan suggested a slightly different syntax:
> > 
> > 	if (cond_guard(...))
> > 		return -EINTR;
> > 
> > But the scoped_cond_guard() macro omits the if statement:
> > 
> >     	scoped_cond_guard (...) {
> >     	}
> > 
> > Thus define cond_guard() similarly to scoped_cond_guard() but with a block
> > to handle the failure case:
> > 
> > 	cond_guard(...)
> > 		return -EINTR;
> 
> That's too subtle for me, because of the mistakes that can be made with
> brackets how about a syntax like:
> 
>  	cond_guard(..., return -EINTR, ...)
> 
> ...to make it clear what happens if the lock acquisition fails without
> having to remember there is a hidden incomplete "if ()" statement in
> that macro? More below...

I sympathize with the hidden "if" being confusing but there is already
precedent in the current *_guard macros.  So I'd like to know if Peter has
an opinion.

Ira

  parent reply	other threads:[~2024-01-30 18:43 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-01-30 16:38 [RFC PATCH v2] cleanup: Add cond_guard() to conditional guards Fabio M. De Francesco
2024-01-30 17:02 ` Dan Williams
2024-01-30 17:33   ` Fabio M. De Francesco
2024-01-30 17:58     ` Dan Williams
2024-01-31 13:11       ` Fabio M. De Francesco
2024-01-30 17:55   ` Fabio M. De Francesco
2024-01-30 18:43   ` Ira Weiny [this message]
2024-01-30 19:06     ` Dan Williams
2024-01-31  0:04       ` Ira Weiny
2024-01-31  0:43         ` Dan Williams

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=65b9434712b57_2d455e294a8@iweiny-mobl.notmuch \
    --to=ira.weiny@intel.com \
    --cc=dan.j.williams@intel.com \
    --cc=fabio.maria.de.francesco@linux.intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=peterz@infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.