Date: Tue, 3 Feb 2009 11:07:52 -0500 From: John Baldwin <jhb@freebsd.org> To: Marcel Moolenaar <xcllnt@mac.com> Cc: ia64@freebsd.org Subject: Re: Change ia64 mca sysctls to use standard dynamic sysctls Message-ID: <200902031107.52507.jhb@freebsd.org> In-Reply-To: <5396BD91-541F-4EDF-9034-106477C99B7F@mac.com> References: <200902021344.19249.jhb@freebsd.org> <5396BD91-541F-4EDF-9034-106477C99B7F@mac.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Monday 02 February 2009 7:59:11 pm Marcel Moolenaar wrote:
>
> On Feb 2, 2009, at 10:44 AM, John Baldwin wrote:
>
> > While working on the locking for the sysctl tree, I ran into the
> > pseudo-dynamic machine check sysctls in ia64. It is not really a
> > good idea
> > to call sysctl_register_oid() with a spin lock held and once I add
> > locking to
> > sysctl it becomes a bad LOR. This changes the code to use the
> > public API for
> > adding dynamic sysctls outside of the spin lock. Please test this
> > as it is a
> > prerequisite for finishing the locking of the sysctl tree.
>
> This won't be a solution, because eventually this code gets called
> for machine checks, which means that locks can be held (which is
> then the least of our worries :-)
>
> In any case: malloc(M_WAITOK) cannot be used. We should probably
> delay updating the sysctl tree and do it in a more controlled
> environment. If you can implement a simple linked list to store
> the saved state and remove the call to SYSCTL_ADD_OID, then I'll
> work on that part after you're done. Does that sound like a plan?
Ok. Checking for failures from M_NOWAIT is important, too. :) Here's
an updated patch that uses the queue. I haven't implemented the stuff
to schedule calls to the populate routine to drain the queue though.
Would you be able to do that?
--- //depot/projects/smpng/sys/ia64/ia64/mca.c 2007/05/19 15:31:43
+++ //depot/user/jhb/lock/ia64/ia64/mca.c 2009/02/03 16:04:36
@@ -42,6 +42,16 @@
MALLOC_DEFINE(M_MCA, "MCA", "Machine Check Architecture");
+struct mca_info {
+ STAILQ_ENTRY(mca_info) mi_link;
+ char mi_name[32];
+ size_t mi_recsz;
+ char mi_record[0];
+};
+
+static STAILQ_HEAD(, mca_info) mca_records =
+ STAILQ_HEAD_INITIALIZER(mca_records);
+
int64_t mca_info_size[SAL_INFO_TYPES];
vm_offset_t mca_info_block;
struct mtx mca_info_block_lock;
@@ -76,14 +86,31 @@
}
void
+ia64_mca_populate(void)
+{
+ struct mca_info *rec;
+
+ mtx_lock_spin(&mca_info_block_lock);
+ while (!STAILQ_EMPTY(&mca_records)) {
+ rec = STAILQ_FIRST(&mca_records);
+ STAILQ_REMOVE_HEAD(&mca_records, mi_link);
+ mtx_unlock_spin(&mca_info_block_lock);
+ (void)SYSCTL_ADD_PROC(NULL, SYSCTL_STATIC_CHILDREN(_hw_mca),
+ OID_AUTO, name, CTLTYPE_OPAQUE | CTLFLAG_RD, rec->mi_record,
+ rec->mi_recsz, mca_sysctl_handler, "S,MCA", "Error record");
+ mtx_lock_spin(&mca_info_block_lock);
+ }
+ mtx_unlock_spin(&mca_info_block_lock);
+}
+
+void
ia64_mca_save_state(int type)
{
struct ia64_sal_result result;
struct mca_record_header *hdr;
- struct sysctl_oid *oidp;
- char *name, *state;
+ struct mca_info *rec;
uint64_t seqnr;
- size_t recsz, totsz;
+ size_t recsz;
/*
* Don't try to get the state if we couldn't get the size of
@@ -95,9 +122,8 @@
if (mca_info_block == 0)
return;
+ mtx_lock_spin(&mca_info_block_lock);
while (1) {
- mtx_lock_spin(&mca_info_block_lock);
-
result = ia64_sal_entry(SAL_GET_STATE_INFO, type, 0,
mca_info_block, 0, 0, 0, 0);
if (result.sal_status < 0) {
@@ -111,11 +137,12 @@
mtx_unlock_spin(&mca_info_block_lock);
- totsz = sizeof(struct sysctl_oid) + recsz + 32;
- oidp = malloc(totsz, M_MCA, M_NOWAIT|M_ZERO);
- state = (char*)(oidp + 1);
- name = state + recsz;
- sprintf(name, "%lld", (long long)seqnr);
+ rec = malloc(sizeof(mca_info) + recsz, M_MCA, M_NOWAIT | M_ZERO);
+ if (rec == NULL)
+ /* XXX: Not sure what to do. */
+ return;
+
+ sprintf(rec->mi_name, "%lld", (long long)seqnr);
mtx_lock_spin(&mca_info_block_lock);
@@ -133,25 +160,15 @@
mca_info_block, 0, 0, 0, 0);
if (seqnr != hdr->rh_seqnr) {
mtx_unlock_spin(&mca_info_block_lock);
- free(oidp, M_MCA);
+ free(rec, M_MCA);
+ mtx_lock_spin(&mca_info_block_lock);
continue;
}
}
- bcopy((char*)mca_info_block, state, recsz);
+ rec->mi_recsz = recsz;
+ bcopy((char*)mca_info_block, rec->mi_record, recsz);
- oidp->oid_parent = &sysctl__hw_mca_children;
- oidp->oid_number = OID_AUTO;
- oidp->oid_kind = CTLTYPE_OPAQUE|CTLFLAG_RD|CTLFLAG_DYN;
- oidp->oid_arg1 = state;
- oidp->oid_arg2 = recsz;
- oidp->oid_name = name;
- oidp->oid_handler = mca_sysctl_handler;
- oidp->oid_fmt = "S,MCA";
- oidp->oid_descr = "Error record";
-
- sysctl_register_oid(oidp);
-
if (mca_count > 0) {
if (seqnr < mca_first)
mca_first = seqnr;
@@ -161,6 +178,7 @@
mca_first = mca_last = seqnr;
mca_count++;
+ STAILQ_INSERT_TAIL(&mca_records, rec, mi_link);
/*
* Clear the state so that we get any other records when
@@ -168,8 +186,6 @@
*/
result = ia64_sal_entry(SAL_CLEAR_STATE_INFO, type, 0, 0, 0,
0, 0, 0);
-
- mtx_unlock_spin(&mca_info_block_lock);
}
}
--
John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200902031107.52507.jhb>