Date: Thu, 6 Mar 2025 19:34:41 +0800 From: Zhenlei Huang <zlei@FreeBSD.org> To: Mateusz Guzik <mjg@FreeBSD.org> Cc: "src-committers@freebsd.org" <src-committers@FreeBSD.org>, "dev-commits-src-all@freebsd.org" <dev-commits-src-all@FreeBSD.org>, Warner Losh <imp@freebsd.org>, "dev-commits-src-main@freebsd.org" <dev-commits-src-main@FreeBSD.org> Subject: Re: git: 234683726708 - main - devclass: make devclass_alloc_unit use M_NOWAIT Message-ID: <6BE51FFD-DE19-42E6-B115-354BB74B4D78@FreeBSD.org> In-Reply-To: <F1B3652E-0D0C-402A-8509-D510992DAC15@FreeBSD.org> References: <202503061103.526B32Id022652@gitrepo.freebsd.org> <F1B3652E-0D0C-402A-8509-D510992DAC15@FreeBSD.org>
index | next in thread | previous in thread | raw e-mail
[-- Attachment #1 --] > On Mar 6, 2025, at 7:32 PM, Zhenlei Huang <zlei@FreeBSD.org> wrote: > > > >> On Mar 6, 2025, at 7:03 PM, Mateusz Guzik <mjg@FreeBSD.org <mailto:mjg@FreeBSD.org>> wrote: >> >> The branch main has been updated by mjg: >> >> URL: https://cgit.FreeBSD.org/src/commit/?id=234683726708cf5212d672d676d30056d4133859 <https://cgit.freebsd.org/src/commit/?id=234683726708cf5212d672d676d30056d4133859>; >> >> commit 234683726708cf5212d672d676d30056d4133859 >> Author: Mateusz Guzik <mjg@FreeBSD.org <mailto:mjg@FreeBSD.org>> >> AuthorDate: 2025-03-06 11:01:49 +0000 >> Commit: Mateusz Guzik <mjg@FreeBSD.org <mailto:mjg@FreeBSD.org>> >> CommitDate: 2025-03-06 11:01:49 +0000 >> >> devclass: make devclass_alloc_unit use M_NOWAIT >> >> The only caller already does this. >> >> The routine can be called with a mutex held making M_WAITOK illegal. >> >> Sponsored by: Rubicon Communications, LLC ("Netgate") >> --- >> sys/kern/subr_bus.c | 8 ++++++-- >> 1 file changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/sys/kern/subr_bus.c b/sys/kern/subr_bus.c >> index 9506e471705c..0422352bba51 100644 >> --- a/sys/kern/subr_bus.c >> +++ b/sys/kern/subr_bus.c >> @@ -1208,6 +1208,7 @@ devclass_get_sysctl_tree(devclass_t dc) >> static int >> devclass_alloc_unit(devclass_t dc, device_t dev, int *unitp) >> { >> + device_t *devices; >> const char *s; >> int unit = *unitp; >> >> @@ -1264,8 +1265,11 @@ devclass_alloc_unit(devclass_t dc, device_t dev, int *unitp) >> int newsize; >> >> newsize = unit + 1; >> - dc->devices = reallocf(dc->devices, >> - newsize * sizeof(*dc->devices), M_BUS, M_WAITOK); >> + devices = reallocf(dc->devices, >> + newsize * sizeof(*dc->devices), M_BUS, M_NOWAIT); > > I'd recommend against this. From the commit message of f3d3c63442ff, Warner said, > > In addition, transition to M_WAITOK since this is a sleepable context > So, the M_WAITOK is intentional. > > Rather than reverting this, the caller devclass_add_device() should use M_WAITOK. > > ``` > - dev->nameunit = malloc(buflen, M_BUS, M_NOWAIT|M_ZERO); > - if (!dev->nameunit) > - return (ENOMEM); > + dev->nameunit = malloc(buflen, M_BUS, M_WAITOK | M_ZERO); > ``` Emm, sorry for the noise. I missed the commit message, > The routine can be called with a mutex held making M_WAITOK illegal. > > Best regards, > Zhenlei > >> + if (devices == NULL) >> + return (ENOMEM); >> + dc->devices = devices; >> memset(dc->devices + dc->maxunit, 0, >> sizeof(device_t) * (newsize - dc->maxunit)); >> dc->maxunit = newsize; [-- Attachment #2 --] <html><head><meta http-equiv="Content-Type" content="text/html; charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><br class=""><div><br class=""><blockquote type="cite" class=""><div class="">On Mar 6, 2025, at 7:32 PM, Zhenlei Huang <<a href="mailto:zlei@FreeBSD.org" class="">zlei@FreeBSD.org</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><meta charset="UTF-8" class=""><div style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 13px; font-style: normal; font-variant-caps: normal; font-weight: 400; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><br class="Apple-interchange-newline"><br class=""><blockquote type="cite" class=""><div class="">On Mar 6, 2025, at 7:03 PM, Mateusz Guzik <<a href="mailto:mjg@FreeBSD.org" class="">mjg@FreeBSD.org</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div class="">The branch main has been updated by mjg:<br class=""><br class="">URL:<span class="Apple-converted-space"> </span><a href="https://cgit.freebsd.org/src/commit/?id=234683726708cf5212d672d676d30056d4133859" class="">https://cgit.FreeBSD.org/src/commit/?id=234683726708cf5212d672d676d30056d4133859</a><br class=""><br class="">commit 234683726708cf5212d672d676d30056d4133859<br class="">Author: Mateusz Guzik <<a href="mailto:mjg@FreeBSD.org" class="">mjg@FreeBSD.org</a>><br class="">AuthorDate: 2025-03-06 11:01:49 +0000<br class="">Commit: Mateusz Guzik <<a href="mailto:mjg@FreeBSD.org" class="">mjg@FreeBSD.org</a>><br class="">CommitDate: 2025-03-06 11:01:49 +0000<br class=""><br class=""> devclass: make devclass_alloc_unit use M_NOWAIT<br class=""><br class=""> The only caller already does this.<br class=""><br class=""> The routine can be called with a mutex held making M_WAITOK illegal.<br class=""><br class=""> Sponsored by: Rubicon Communications, LLC ("Netgate")<br class="">---<br class="">sys/kern/subr_bus.c | 8 ++++++--<br class="">1 file changed, 6 insertions(+), 2 deletions(-)<br class=""><br class="">diff --git a/sys/kern/subr_bus.c b/sys/kern/subr_bus.c<br class="">index 9506e471705c..0422352bba51 100644<br class="">--- a/sys/kern/subr_bus.c<br class="">+++ b/sys/kern/subr_bus.c<br class="">@@ -1208,6 +1208,7 @@ devclass_get_sysctl_tree(devclass_t dc)<br class="">static int<br class="">devclass_alloc_unit(devclass_t dc, device_t dev, int *unitp)<br class="">{<br class="">+<span class="Apple-tab-span" style="white-space: pre;"> </span>device_t *devices;<br class=""><span class="Apple-tab-span" style="white-space: pre;"> </span>const char *s;<br class=""><span class="Apple-tab-span" style="white-space: pre;"> </span>int unit = *unitp;<br class=""><br class="">@@ -1264,8 +1265,11 @@ devclass_alloc_unit(devclass_t dc, device_t dev, int *unitp)<br class=""><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span>int newsize;<br class=""><br class=""><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span>newsize = unit + 1;<br class="">-<span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span>dc->devices = reallocf(dc->devices,<br class="">-<span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-converted-space"> </span> newsize * sizeof(*dc->devices), M_BUS, M_WAITOK);<br class="">+<span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span>devices = reallocf(dc->devices,<br class="">+<span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-converted-space"> </span> newsize * sizeof(*dc->devices), M_BUS, M_NOWAIT);<br class=""></div></div></blockquote><div class=""><br class=""></div>I'd recommend against this. From the commit message of f3d3c63442ff, Warner said,</div><div style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 13px; font-style: normal; font-variant-caps: normal; font-weight: 400; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class="">> In addition, transition to M_WAITOK since this is a sleepable context</div><div style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 13px; font-style: normal; font-variant-caps: normal; font-weight: 400; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class="">So, the M_WAITOK is intentional.</div><div style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 13px; font-style: normal; font-variant-caps: normal; font-weight: 400; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><br class=""></div><div style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 13px; font-style: normal; font-variant-caps: normal; font-weight: 400; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class="">Rather than reverting this, the caller devclass_add_device() should use<span class="Apple-converted-space"> </span><span class="" style="caret-color: rgb(0, 0, 0);">M_WAITOK.</span></div><div style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 13px; font-style: normal; font-variant-caps: normal; font-weight: 400; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span class="" style="caret-color: rgb(0, 0, 0);"><br class=""></span></div><div style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 13px; font-style: normal; font-variant-caps: normal; font-weight: 400; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span class="" style="caret-color: rgb(0, 0, 0);">```</span></div><div style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 13px; font-style: normal; font-variant-caps: normal; font-weight: 400; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><div class=""><font class=""><span class="" style="caret-color: rgb(0, 0, 0);">- dev->nameunit = malloc(buflen, M_BUS, M_NOWAIT|M_ZERO);</span></font></div><div class=""><font class=""><span class="" style="caret-color: rgb(0, 0, 0);">- if (!dev->nameunit)</span></font></div><div class=""><font class=""><span class="" style="caret-color: rgb(0, 0, 0);">- return (ENOMEM);</span></font></div><div class=""><font class=""><span class="" style="caret-color: rgb(0, 0, 0);">+ dev->nameunit = malloc(buflen, M_BUS, M_WAITOK | M_ZERO);</span></font></div></div><div style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 13px; font-style: normal; font-variant-caps: normal; font-weight: 400; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span class="" style="caret-color: rgb(0, 0, 0);">```</span></div></div></blockquote><div><br class=""></div><div>Emm, sorry for the noise. I missed the commit message,</div><div>> The routine can be called with a mutex held making M_WAITOK illegal.</div><br class=""><blockquote type="cite" class=""><div class=""><div style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 13px; font-style: normal; font-variant-caps: normal; font-weight: 400; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span class="" style="caret-color: rgb(0, 0, 0);"><br class=""></span></div><div style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 13px; font-style: normal; font-variant-caps: normal; font-weight: 400; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><div style="caret-color: rgb(0, 0, 0);" class="">Best regards,</div><div style="caret-color: rgb(0, 0, 0);" class="">Zhenlei</div></div><div style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 13px; font-style: normal; font-variant-caps: normal; font-weight: 400; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><br class=""><blockquote type="cite" class=""><div class=""><div class="">+<span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span>if (devices == NULL)<br class="">+<span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span>return (ENOMEM);<br class="">+<span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span>dc->devices = devices;<br class=""><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span>memset(dc->devices + dc->maxunit, 0,<br class=""><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-converted-space"> </span> sizeof(device_t) * (newsize - dc->maxunit));<br class=""><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span>dc->maxunit = newsize;</div></div></blockquote></div></div></blockquote></div><br class=""><div class=""> <div><br class=""></div> </div> <br class=""></body></html>help
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?6BE51FFD-DE19-42E6-B115-354BB74B4D78>