From owner-svn-src-head@freebsd.org Sat Jun 8 19:58:27 2019 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 2BC4E15B2115; Sat, 8 Jun 2019 19:58:27 +0000 (UTC) (envelope-from yaneurabeya@gmail.com) Received: from mail-pl1-x634.google.com (mail-pl1-x634.google.com [IPv6:2607:f8b0:4864:20::634]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 8B5B170A1E; Sat, 8 Jun 2019 19:58:26 +0000 (UTC) (envelope-from yaneurabeya@gmail.com) Received: by mail-pl1-x634.google.com with SMTP id bi6so1691123plb.12; Sat, 08 Jun 2019 12:58:26 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:subject:from:in-reply-to:date:cc :content-transfer-encoding:message-id:references:to; bh=dkMC8bYvkxImUCK9nuqkIf72SFxZmZ3pMoXr7gqVKbA=; b=Pjv2DK88wN+TNSSuXlJATm1hQQonbUP4O5GeK/1H7KcLRFHM/eDD7n6X2LnFSkVSrC pSl+V5L7UwzS4XKW9uU5dDlFoRr8fXnBWbqKszn14BB+BdWTzF97jupfzJq98g6Cu+vO fPZbOYapf8s1TN6syEwOo7JKfcXboqGlY1Uqop0PmRTfaBCWHhWTOdzZmm9gynnEDZsm pLTMPUXEu8TErvzOpg266+mTcv76baPgmeKiHaubHK4dHQJ/Yt3LB9futk9JBzbGkXwx fYp7Ed1NiUnvFiO0bMg3D0dI3HV9hkrdXHkw7LCY/c9CXaEVWQQG4v8ImgouCDjhB1t3 /TbQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:subject:from:in-reply-to:date:cc :content-transfer-encoding:message-id:references:to; bh=dkMC8bYvkxImUCK9nuqkIf72SFxZmZ3pMoXr7gqVKbA=; b=l/lM7habilhHJLta5GpR5b2+cRoAC1qEYhTN1c/Mss+pqc+EIg8Yo9qKGmzOb0O9SK EAO96Q8gXsLhnBX2Fr2BS84YFxBtRfyqO22mOGgvsDsct4TExp0tcAwJkGedHeYgh4G/ CuHuZQCG1xjsSTXpSPGwv/30L6fWBQSPAOCmo+IiE/+/xknrlIktFKpBLKVS0BMHzxUb EeqjXyO+kdCvTt3wBzQfgInMwrd9cqLWcLbHaGEUvgG8ozc2ivNAxtu0EoPXuNI+97Ux FawuuRQsRo9/gPtPNjVOSEhc0lS7WhsGWUVNBjeXkH8LraQ/bS8oViAjeWA2XqVMxFdq bTmA== X-Gm-Message-State: APjAAAWgzymkr/el9lu+QO/1auZpUVnwSazCwz8RJowBVj1sCSoOHjaV vYISD/ev2/c+64Sd0rG4+5EBs4Mx X-Google-Smtp-Source: APXvYqyDIhKlXTZmKeZ4qcvLl+tt6bSeVIa6lgs0NEwZZl32FEvP2stJmacZkiutzrCD+id78elC9g== X-Received: by 2002:a17:902:42e2:: with SMTP id h89mr60511879pld.271.1560023904902; Sat, 08 Jun 2019 12:58:24 -0700 (PDT) Received: from ?IPv6:2601:204:d980:655e:4d4c:6779:c49b:bec? ([2601:204:d980:655e:4d4c:6779:c49b:bec]) by smtp.gmail.com with ESMTPSA id d123sm10531545pfc.144.2019.06.08.12.58.23 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sat, 08 Jun 2019 12:58:24 -0700 (PDT) Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (1.0) Subject: Re: svn commit: r348810 - head/sys/x86/x86 From: Enji Cooper X-Mailer: iPhone Mail (16F203) In-Reply-To: <201906081826.x58IQnQe067743@repo.freebsd.org> Date: Sat, 8 Jun 2019 12:58:23 -0700 Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Content-Transfer-Encoding: quoted-printable Message-Id: <760B9EAA-6FD7-4B23-9BE3-5011B3F4C21C@gmail.com> References: <201906081826.x58IQnQe067743@repo.freebsd.org> To: "Jonathan T. Looney" X-Rspamd-Queue-Id: 8B5B170A1E X-Spamd-Bar: ------ Authentication-Results: mx1.freebsd.org X-Spamd-Result: default: False [-6.94 / 15.00]; NEURAL_HAM_MEDIUM(-1.00)[-0.996,0]; NEURAL_HAM_LONG(-1.00)[-1.000,0]; REPLY(-4.00)[]; NEURAL_HAM_SHORT(-0.94)[-0.943,0] X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.29 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: Sat, 08 Jun 2019 19:58:27 -0000 Hi! > On Jun 8, 2019, at 11:26, Jonathan T. Looney wrote: >=20 > Author: jtl > Date: Sat Jun 8 18:26:48 2019 > New Revision: 348810 > URL: https://svnweb.freebsd.org/changeset/base/348810 >=20 > Log: > Currently, MCA entries remain on an every-growing linked list. This means= > that it becomes increasingly expensive to process a steady stream of > correctable errors. Additionally, the memory used by the MCA entries can > grow without bound. >=20 > Change the code to maintain two separate lists: a list of entries which > still need to be logged, and a list of entries which have already been > logged. Additionally, allow a user-configurable limit on the number of > entries which will be saved after they are logged. (The limit defaults > to -1 [unlimited], which is the current behavior.) >=20 > Reviewed by: imp, jhb > MFC after: 2 weeks > Sponsored by: Netflix > Differential Revision: https://reviews.freebsd.org/D20482 Briefly looking through the code, I was wondering if the type/locking for mc= a_freecount (before and after this commit) was correct, given that it seems l= ike it can be modified in multiple call sites and in different tasks. Thank you! -Enji > Modified: > head/sys/x86/x86/mca.c >=20 > Modified: head/sys/x86/x86/mca.c > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D > --- head/sys/x86/x86/mca.c Sat Jun 8 17:49:17 2019 (r348809) > +++ head/sys/x86/x86/mca.c Sat Jun 8 18:26:48 2019 (r348810) > @@ -86,7 +86,6 @@ struct amd_et_state { >=20 > struct mca_internal { > struct mca_record rec; > - int logged; > STAILQ_ENTRY(mca_internal) link; > }; >=20 > @@ -101,6 +100,7 @@ static MALLOC_DEFINE(M_MCA, "MCA", "Machine Check Arch= >=20 > static volatile int mca_count; /* Number of records stored. */ > static int mca_banks; /* Number of per-CPU register banks. */ > +static int mca_maxcount =3D -1; /* Limit on records stored. (-1 =3D un= limited) */ >=20 > static SYSCTL_NODE(_hw, OID_AUTO, mca, CTLFLAG_RD, NULL, > "Machine Check Architecture"); > @@ -125,10 +125,11 @@ SYSCTL_INT(_hw_mca, OID_AUTO, erratum383, CTLFLAG_RD= TU > static STAILQ_HEAD(, mca_internal) mca_freelist; > static int mca_freecount; > static STAILQ_HEAD(, mca_internal) mca_records; > +static STAILQ_HEAD(, mca_internal) mca_pending; > static struct callout mca_timer; > static int mca_ticks =3D 3600; /* Check hourly by default. */ > static struct taskqueue *mca_tq; > -static struct task mca_refill_task, mca_scan_task; > +static struct task mca_resize_task, mca_scan_task; > static struct mtx mca_lock; >=20 > static unsigned int > @@ -557,32 +558,49 @@ mca_check_status(int bank, struct mca_record *rec) > } >=20 > static void > -mca_fill_freelist(void) > +mca_resize_freelist(void) > { > - struct mca_internal *rec; > - int desired; > + struct mca_internal *next, *rec; > + STAILQ_HEAD(, mca_internal) tmplist; > + int count, i, desired_max, desired_min; >=20 > /* > * Ensure we have at least one record for each bank and one > - * record per CPU. > + * record per CPU, but no more than twice that amount. > */ > - desired =3D imax(mp_ncpus, mca_banks); > + desired_min =3D imax(mp_ncpus, mca_banks); > + desired_max =3D imax(mp_ncpus, mca_banks) * 2; > + STAILQ_INIT(&tmplist); > mtx_lock_spin(&mca_lock); > - while (mca_freecount < desired) { > + while (mca_freecount > desired_max) { > + rec =3D STAILQ_FIRST(&mca_freelist); > + KASSERT(rec !=3D NULL, ("mca_freecount is %d, but list is empty",= > + mca_freecount)); > + STAILQ_REMOVE_HEAD(&mca_freelist, link); > + mca_freecount--; > + STAILQ_INSERT_TAIL(&tmplist, rec, link); > + } > + while (mca_freecount < desired_min) { > + count =3D desired_min - mca_freecount; > mtx_unlock_spin(&mca_lock); Should this also be outside the loop, like it was before, to ensure the lock= ing is correct, the lock is always unlocked, and the current thread yields t= o the other threads, or should the lock be held over both loops? > - rec =3D malloc(sizeof(*rec), M_MCA, M_WAITOK); > + for (i =3D 0; i < count; i++) { > + rec =3D malloc(sizeof(*rec), M_MCA, M_WAITOK); > + STAILQ_INSERT_TAIL(&tmplist, rec, link); > + } > mtx_lock_spin(&mca_lock); > - STAILQ_INSERT_TAIL(&mca_freelist, rec, link); > - mca_freecount++; > + STAILQ_CONCAT(&mca_freelist, &tmplist); > + mca_freecount +=3D count; > } > mtx_unlock_spin(&mca_lock); > + STAILQ_FOREACH_SAFE(rec, &tmplist, link, next) > + free(rec, M_MCA); > } Thanks! -Enji=