| Message ID | 170905254443.2268463.935306988251313983.stgit@dwillia2-xfh.jf.intel.com |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cleanup: A couple extensions for conditional resource management | expand |
On Tue, 27 Feb 2024 at 08:49, Dan Williams <dan.j.williams@intel.com> wrote: > > - rc = down_read_interruptible(&cxl_region_rwsem); > - if (rc) > - return rc; > + cond_guard(rwsem_read_intr, return -EINTR, &cxl_region_rwsem); Yeah, this is an example of how NOT to do things. If you can't make the syntax be something clean and sane like if (!cond_guard(rwsem_read_intr, &cxl_region_rwsem)) return -EINTR; then this code should simply not be converted to guards AT ALL. Note that we have a perfectly fine way to do conditional lock guarding by simply using helper functions, which actually makes code MORE READABLE: if (!down_read_interruptible(&cxl_region_rwsem)) return -EINTR; rc = do_locked_function(); up_read(&cxl_region_rwsem); return rc; and notice how there are no special cases, no multiple unlocks, no NOTHING. And the syntax is clean. Honestly, if people are going to use 'guard' to write crap code, we need to really stop that in its tracks. There is no upside to making up new interfaces that only generate garbage. This is final. I'm not willing to even entertain this kind of crap. Linus
Linus Torvalds wrote: > On Tue, 27 Feb 2024 at 08:49, Dan Williams <dan.j.williams@intel.com> wrote: > > > > - rc = down_read_interruptible(&cxl_region_rwsem); > > - if (rc) > > - return rc; > > + cond_guard(rwsem_read_intr, return -EINTR, &cxl_region_rwsem); > > Yeah, this is an example of how NOT to do things. > > If you can't make the syntax be something clean and sane like > > if (!cond_guard(rwsem_read_intr, &cxl_region_rwsem)) > return -EINTR; > > then this code should simply not be converted to guards AT ALL. > > Note that we have a perfectly fine way to do conditional lock guarding > by simply using helper functions, which actually makes code MORE > READABLE: > > if (!down_read_interruptible(&cxl_region_rwsem)) > return -EINTR; > rc = do_locked_function(); > up_read(&cxl_region_rwsem); > return rc; > > and notice how there are no special cases, no multiple unlocks, no > NOTHING. And the syntax is clean. Ok, I took the wrong lessons from scoped_cond_guard(). Consider it dropped. > Honestly, if people are going to use 'guard' to write crap code, we > need to really stop that in its tracks. > > There is no upside to making up new interfaces that only generate garbage. > > This is final. I'm not willing to even entertain this kind of crap. I will also note that these last 3 statements, nuking the proposal from space, I find excessive. Yes, on the internet no one can hear you being subtle, but the "MORE READABLE" and "NOTHING" were pretty darn unequivocal, especially coming from the person who has absolute final say on what enters his project. I have been around a while so I take this as par for the course, and I appreciate blunt feedback. I have had dance teachers tell me my "dancing is shit", and sometimes that level of bluntness is needed, but that was also from somebody I have worked with for years. Fabio has not been around that long, and nothing about what Fabio did was crap, he carried through on an idea that I asked him to explore and it did not work out. So Fabio, keep going, thank you for patiently working through this investigation and my takeaway is that we have successfully discovered one way this mission to cleanup usage of goto in the CXL subsystem can not proceed. Back to the drawing board.
On Tue, 27 Feb 2024 at 13:42, Dan Williams <dan.j.williams@intel.com> wrote: > > I will also note that these last 3 statements, nuking the proposal from > space, I find excessive. Yes, on the internet no one can hear you being > subtle, but the "MORE READABLE" and "NOTHING" were pretty darn > unequivocal, especially coming from the person who has absolute final > say on what enters his project. Heh. It's not just " one can hear you being subtle", sometimes it's also "people don't take hints". It can be hard to tell.. Anyway, it's not that I hate the guard things in general. But I do think they need to be used carefully, and I do think it's very important that they have clean interfaces. The current setup came about after quite long discussions about getting reasonable syntax, and I'm still a bit worried even about the current simpler ones. And by "simpler ones" I don't mean our current scoped_cond_guard() thing. We have exactly one user of it, and I have considered getting rid of that one user because I think it's absolutely horrid. I haven't figured out a better syntax for it. For the non-scoped version, I actually think there *would* be a better syntax - putting the error case after the macro (the way we put the success case after the macro for the scoped one). In fact, maybe the solution is to make the scoped and non-scoped versions act very similar: we could do something like this: [scoped_]cond_guard(name, args) { success } else { fail }; and that syntax feels much more C-line to me. So maybe something like the attached (TOTALLY UNTESTED!!) patch for the scoped version, and then the non-scoped version would have the same syntax (except it would have to generate that __UNIQUE_ID() thing, of course). I haven't thought much about this. But I think this would be more acceptable to me, and also solve some of the ugliness with the current pre-existing scoped_cond_guard(). I dunno. PeterZ did the existing stuff, but he's offlined due to shoulder problems so not likely to chip in. Linus
Linus Torvalds wrote: > On Tue, 27 Feb 2024 at 13:42, Dan Williams <dan.j.williams@intel.com> wrote: > > > > I will also note that these last 3 statements, nuking the proposal from > > space, I find excessive. Yes, on the internet no one can hear you being > > subtle, but the "MORE READABLE" and "NOTHING" were pretty darn > > unequivocal, especially coming from the person who has absolute final > > say on what enters his project. > > Heh. It's not just " one can hear you being subtle", sometimes it's > also "people don't take hints". It can be hard to tell.. I appreciate that. It is difficult to judge what size clue bat to carry from one thread to the next. > Anyway, it's not that I hate the guard things in general. But I do > think they need to be used carefully, and I do think it's very > important that they have clean interfaces. > > The current setup came about after quite long discussions about > getting reasonable syntax, and I'm still a bit worried even about the > current simpler ones. > > And by "simpler ones" I don't mean our current scoped_cond_guard() > thing. We have exactly one user of it, and I have considered getting > rid of that one user because I think it's absolutely horrid. I haven't > figured out a better syntax for it. > > For the non-scoped version, I actually think there *would* be a better > syntax - putting the error case after the macro (the way we put the > success case after the macro for the scoped one). > > In fact, maybe the solution is to make the scoped and non-scoped > versions act very similar: we could do something like this: > > [scoped_]cond_guard(name, args) { success } else { fail }; > > and that syntax feels much more C-line to me. > > So maybe something like the attached (TOTALLY UNTESTED!!) patch for > the scoped version, and then the non-scoped version would have the > same syntax (except it would have to generate that __UNIQUE_ID() > thing, of course). This would have definitely saved me from thinking that passing a "return" statement to a macro was an idea worth copying. I like that it puts the onus on the caller to understand "this is a conditional" you are responsible for handling the conditions, the macro is only handling releasing the lock at the end of the scope". > I haven't thought much about this. But I think this would be more > acceptable to me, and also solve some of the ugliness with the current > pre-existing scoped_cond_guard(). > > I dunno. PeterZ did the existing stuff, but he's offlined due to > shoulder problems so not likely to chip in. Ah, ok, yeah has been quiet on this thread for a while. I will take some inspiration from this proposal and huddle again with Fabio. Thanks for the nudge.
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c index ce0e2d82bb2b..704102f75c14 100644 --- a/drivers/cxl/core/region.c +++ b/drivers/cxl/core/region.c @@ -666,28 +666,20 @@ static size_t show_targetN(struct cxl_region *cxlr, char *buf, int pos) { struct cxl_region_params *p = &cxlr->params; struct cxl_endpoint_decoder *cxled; - int rc; - rc = down_read_interruptible(&cxl_region_rwsem); - if (rc) - return rc; + cond_guard(rwsem_read_intr, return -EINTR, &cxl_region_rwsem); if (pos >= p->interleave_ways) { dev_dbg(&cxlr->dev, "position %d out of range %d\n", pos, p->interleave_ways); - rc = -ENXIO; - goto out; + return -ENXIO; } cxled = p->targets[pos]; if (!cxled) - rc = sysfs_emit(buf, "\n"); - else - rc = sysfs_emit(buf, "%s\n", dev_name(&cxled->cxld.dev)); -out: - up_read(&cxl_region_rwsem); + return sysfs_emit(buf, "\n"); - return rc; + return sysfs_emit(buf, "%s\n", dev_name(&cxled->cxld.dev)); } static int match_free_decoder(struct device *dev, void *data)