<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40"> <head> <meta http-equiv="Content-Type" content="text/html; charset=us-ascii"> <meta name="ProgId" content="Word.Document"> <meta name="Generator" content="Microsoft Word 15"> <meta name="Originator" content="Microsoft Word 15"> <link rel="File-List" href="cid:filelist.xml@01D7C663.06EF7FA0"><style></style> </head> <body lang="EN-US" link="blue" vlink="purple" style="tab-interval:.5in;word-wrap:break-word"> <div class="WordSection1"> [AMD Official Use Only]<o:p></o:p> <o:p> </o:p> My editor won’t let me reply in-line without making it look like garbage. <o:p></o:p> <o:p> </o:p> Thanks for the insight, Luben! They’re all useful points, especially the consolidation and relying on the threshold_validation which has already occurred before we get to this point (which I should’ve checked). <o:p></o:p> <o:p> </o:p> And I overdid the transitive multiplication explanation, so I wouldn’t have to answer questions about it later. But your concise comment below pretty much covers things and shouldn’t cause any unnecessary inquiries.<o:p></o:p> <o:p> </o:p> Kent <o:p> </o:p> <o:p> </o:p> <div style="border:none;border-left:solid blue 1.5pt;padding:0in 0in 0in 4.0pt"> <div> <div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in"> From: Tuikov, Luben <Luben.Tuikov@amd.com> Sent: Wednesday, October 20, 2021 5:48 PM To: Russell, Kent <Kent.Russell@amd.com>; amd-gfx@lists.freedesktop.org Cc: Joshi, Mukul <Mukul.Joshi@amd.com> Subject: Re: [PATCH 1/3] drm/amdgpu: Warn when bad pages approaches 90% threshold<o:p></o:p> </div> </div> <o:p> </o:p> <div> On 2021-10-20 12:35, Kent Russell wrote:<o:p></o:p> </div> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> <pre>Currently dmesg doesn't warn when the number of bad pages approaches the<o:p></o:p></pre> </blockquote> "Currently" is redundant in this sentence as it is already in present simple tense. Fair point<o:p></o:p> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> <pre><o:p> </o:p></pre> <pre>threshold for page retirement. WARN when the number of bad pages<o:p></o:p></pre> <pre>is at 90% or greater for easier checks and planning, instead of waiting<o:p></o:p></pre> <pre>until the GPU is full of bad pages<o:p></o:p></pre> </blockquote> Missing full-stop (period) above. <![if !supportLineBreakNewLine]> <![endif]><o:p></o:p> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> <pre><o:p> </o:p></pre> <pre><o:p> </o:p></pre> <pre>Cc: Luben Tuikov <a href="mailto:luben.tuikov@amd.com"><luben.tuikov@amd.com></a><o:p></o:p></pre> <pre>Cc: Mukul Joshi <a href="mailto:Mukul.Joshi@amd.com"><Mukul.Joshi@amd.com></a><o:p></o:p></pre> <pre>Signed-off-by: Kent Russell <a href="mailto:kent.russell@amd.com"><kent.russell@amd.com></a><o:p></o:p></pre> <pre>---<o:p></o:p></pre> <pre> drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c | 17 +++++++++++++++++<o:p></o:p></pre> <pre> 1 file changed, 17 insertions(+)<o:p></o:p></pre> <pre><o:p> </o:p></pre> <pre>diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c<o:p></o:p></pre> <pre>index f4c05ff4b26c..1ede0f0d6f55 100644<o:p></o:p></pre> <pre>--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c<o:p></o:p></pre> <pre>+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c<o:p></o:p></pre> <pre>@@ -1071,12 +1071,29 @@ int amdgpu_ras_eeprom_init(struct amdgpu_ras_eeprom_control *control,<o:p></o:p></pre> <pre> control->ras_fri = RAS_OFFSET_TO_INDEX(control, hdr->first_rec_offset);<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre> if (hdr->header == RAS_TABLE_HDR_VAL) {<o:p></o:p></pre> <pre>+ int threshold = 0;<o:p></o:p></pre> <pre> DRM_DEBUG_DRIVER("Found existing EEPROM table with %d records",<o:p></o:p></pre> <pre> control->ras_num_recs);<o:p></o:p></pre> <pre> res = __verify_ras_table_checksum(control);<o:p></o:p></pre> <pre> if (res)<o:p></o:p></pre> <pre> DRM_ERROR("RAS table incorrect checksum or error:%d\n",<o:p></o:p></pre> <pre> res);<o:p></o:p></pre> <pre>+<o:p></o:p></pre> <pre>+ /* threshold = 0 means that page retirement is disabled, while<o:p></o:p></pre> <pre>+ * threshold = -1 means default behaviour<o:p></o:p></pre> <pre>+ */<o:p></o:p></pre> <pre>+ if (amdgpu_bad_page_threshold == -1)<o:p></o:p></pre> <pre>+ threshold = ras->bad_page_cnt_threshold;<o:p></o:p></pre> <pre>+ else if (amdgpu_bad_page_threshold > 0)<o:p></o:p></pre> <pre>+ threshold = amdgpu_bad_page_threshold;<o:p></o:p></pre> </blockquote> I believe we don't need this calculation here as it's already been done for us in amdgpu_ras_validate_threshold(), in amdgpu_ras.c. <![if !supportLineBreakNewLine]> <![endif]><o:p></o:p> I believe you want to use "ras->bad_page_cnt_threshold" here instead. For instance of this, see a bit further down in this very function this check including the comment, in italics: } else if (hdr->header == RAS_TABLE_HDR_BAD && amdgpu_bad_page_threshold != 0) { res = __verify_ras_table_checksum(control); if (res) DRM_ERROR("RAS Table incorrect checksum or error:%d\n", res); if (ras->bad_page_cnt_threshold > control->ras_num_recs) { /* This means that, the threshold was increased since * the last time the system was booted, and now, * ras->bad_page_cnt_threshold - control->num_recs > 0, * so that at least one more record can be saved, * before the page count threshold is reached. */ And on the "else", a bit further down, again in italics: } else { *exceed_err_limit = true; dev_err(adev->dev, "RAS records:%d exceed threshold:%d, " "maybe retire this GPU?", control->ras_num_recs, ras->bad_page_cnt_threshold); } See how it says "records exceed threshold"--well, with this patch you want to say "records exceed 90% of threshold." :-) So these are the quantities we gauge each other to. Clarification on this below. <![if !supportLineBreakNewLine]> <![endif]><o:p></o:p> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> <pre><o:p> </o:p></pre> <pre>+<o:p></o:p></pre> <pre>+ /* Since multiplcation is transitive, a = 9b/10 is the same<o:p></o:p></pre> <pre>+ * as 10a = 9b. Use this for our 90% limit to avoid rounding<o:p></o:p></pre> <pre>+ */<o:p></o:p></pre> </blockquote> I really like the format of the comment. But I feel that the comment itself isn't necessary... at least the way it is written ("9b" may mean "9 bits" or "9 binary". I'd avoid getting into arithmetic theory, and remove the comment completely. Anything else (explaining the math) really distracts from the real purpose of what we're doing. (After all, this is C, not a class on arithmetic--they who can, will figure it out.) Perhaps something like: /* Warn if we get past 90% of the threshold. */ <![if !supportLineBreakNewLine]> <![endif]><o:p></o:p> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> <pre>+ if (threshold > 0 && ((control->ras_num_recs * 10) >= (threshold * 9)))<o:p></o:p></pre> </blockquote> Right, so here perhaps we want to do this instead: diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c index 98732518543e53..2aab62fa488eba 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c @@ -1077,6 +1077,12 @@ int amdgpu_ras_eeprom_init(struct amdgpu_ras_eeprom_control *control, if (res) DRM_ERROR("RAS table incorrect checksum or error:%d\n", res); + /* Warn if we get past 90% of the threshold. + */ + if (10 * control->ras_num_recs >= 9 * ras->bad_page_cnt_threshold) + DRM_WARN("RAS records:%u exceeds 90%% of threshold:%d", + control->ras_num_recs, + ras->bad_page_cnt_threshold); } else if (hdr->header == RAS_TABLE_HDR_BAD && amdgpu_bad_page_threshold != 0) { res = __verify_ras_table_checksum(control); Also note that up until this point of the boot process, we don't qualify the boot by amdgpu_bad_page_threshold and I feel that this check in this embedded if-conditional shouldn't do that as well. Regards, Luben <![if !supportLineBreakNewLine]> <![endif]><o:p></o:p> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> <pre><o:p> </o:p></pre> <pre>+ DRM_WARN("RAS records:%u exceeds 90%% of threshold:%d",<o:p></o:p></pre> <pre>+ control->ras_num_recs,<o:p></o:p></pre> <pre>+ threshold);<o:p></o:p></pre> <pre> } else if (hdr->header == RAS_TABLE_HDR_BAD &&<o:p></o:p></pre> <pre> amdgpu_bad_page_threshold != 0) {<o:p></o:p></pre> <pre> res = __verify_ras_table_checksum(control);<o:p></o:p></pre> </blockquote> <o:p> </o:p> </div> </div> </body> </html>