From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-13.3 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_IN_DEF_DKIM_WL autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id BB22BC433DB for ; Fri, 15 Jan 2021 17:17:45 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 65991238EF for ; Fri, 15 Jan 2021 17:17:45 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 65991238EF Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id EB77F8D01B9; Fri, 15 Jan 2021 12:17:44 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id E3F9F8D01B2; Fri, 15 Jan 2021 12:17:44 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id D2E578D01B9; Fri, 15 Jan 2021 12:17:44 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0117.hostedemail.com [216.40.44.117]) by kanga.kvack.org (Postfix) with ESMTP id B92FF8D01B2 for ; Fri, 15 Jan 2021 12:17:44 -0500 (EST) Received: from smtpin08.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id 8103C1822B855 for ; Fri, 15 Jan 2021 17:17:44 +0000 (UTC) X-FDA: 77708666448.08.boats85_0b0442627531 Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin08.hostedemail.com (Postfix) with ESMTP id 5CB001819E785 for ; Fri, 15 Jan 2021 17:17:44 +0000 (UTC) X-HE-Tag: boats85_0b0442627531 X-Filterd-Recvd-Size: 8856 Received: from mail-qt1-f182.google.com (mail-qt1-f182.google.com [209.85.160.182]) by imf01.hostedemail.com (Postfix) with ESMTP for ; Fri, 15 Jan 2021 17:17:43 +0000 (UTC) Received: by mail-qt1-f182.google.com with SMTP id v5so6517715qtv.7 for ; Fri, 15 Jan 2021 09:17:43 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=/DHaZOpAOYW+CVAQpgGqLKRGRisryfn2+7uDhop5wiY=; b=nWG6nkLP2W832dXmwscLjoKUdOBGPYCgnSpWhfC8a3bWNfA3MO+kMOxC9x46VDUdHT +nYmt4sigwb0D1GXaDdtv5qWHeP+3Zxp9uhS5/+h2U7wkJKnWwbij7Cr8oysLba9DWDc UOcARMtGLoPIJlC4Se89RRNeP3lshYhbPNE983NFiOk7WtB817qJHroXPorYOZYR59Jt eV3WIM0xQkTXtushzJfbDQorIfZr3F3u7NU213oZa8qtYoWpizVyDiKGBJSpGj4UAtjo eE0B17ufL0T2FIVTRNXtJsQyjjT5Yf6x46HcWsJHO04glW3MhS5RhJSQHS5cjey4yg8v 2hMw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=/DHaZOpAOYW+CVAQpgGqLKRGRisryfn2+7uDhop5wiY=; b=leulvngRQFpgS/2J8mOFAL1XxLcvWKl1upi4QPsUKm5vXzUJ6b2n8Go6CnaObkqI+u 0ydSpeqCJ60bn/0pKMrCW5M89LZjdWSrS7xsCKdrZapTftE4gTDzlvrkB6gDsz0/pK2P Njc0Aa/Kb+mflQS6zalAcZy52pvm4/D9yn69/TgiUspQQv5EacL40JN17MSiSCLvYmyA rANZGL2py42giJwZyruy18XukwZZHfy1yd2zN0hHDeHtE5vtgr3O3eDNVBfkEPDlhi19 3qj2qMCUJnXh4bYvE8qqJWrqoeRXgczqvwKsDCvxNNevTIq3sKNsPseMTcZKElV0Rw5w j0yg== X-Gm-Message-State: AOAM533AXBP7rXHXona0N8yDYw2ZiTwdOFeGCtFabb6u7iXtNjZWL6MU v/iKfJIeX/KGCfsUhwEXjf67LYuRinZKzoVti7AcOw== X-Google-Smtp-Source: ABdhPJwrhbcCaDD/baTLOprNNrD/xSRJFUJeBmlRgVauzzrolfvDAlDbLX+jx+zbI9U2zXUvVlreppV8EWfQnIdOpN8= X-Received: by 2002:ac8:7512:: with SMTP id u18mr12796120qtq.300.1610731062778; Fri, 15 Jan 2021 09:17:42 -0800 (PST) MIME-Version: 1.0 References: <20210115130336.2520663-1-glider@google.com> <20210115130336.2520663-3-glider@google.com> In-Reply-To: From: Alexander Potapenko Date: Fri, 15 Jan 2021 18:17:30 +0100 Message-ID: Subject: Re: [PATCH v2 2/5] lib: add error_report_notify to collect debugging tools' reports To: Greg KH Cc: LKML , Andrew Morton , Andrey Konovalov , Dmitriy Vyukov , Ingo Molnar , Marco Elver , Petr Mladek , Steven Rostedt , Sergey Senozhatsky , Linux Memory Management List Content-Type: text/plain; charset="UTF-8" X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Fri, Jan 15, 2021 at 2:50 PM Greg KH wrote: > > Minor comments, if in the future, you really do want to mess around in sysfs: > Thanks! Guess most of these comments apply even if I choose another FS to mess around with. > No copyright notice for the file? While acceptable, odds are your > corporate lawyers will not be happy with that :( You are right, will fix. > > +/* > > + * Userspace notification interface for debugging tools. > > + * > > + * Provide two sysfs files: > > + * - /sys/kernel/error_report/last_report > > + * - /sys/kernel/error_report/report_count > > + * that contain the last debugging tool report (taken from dmesg, delimited by > > + * the error_report_start/error_report_end tracing events) and the total report > > + * count. > > + */ > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +static struct kobject *error_report_kobj; > > + > > +/* sysfs files are capped at PAGE_SIZE. */ > > +#define BUF_SIZE PAGE_SIZE > > +/* Two buffers to store the finished report and the report being recorded. */ > > +static char report_buffer[2][BUF_SIZE]; > > +/* > > + * Total report count. Also serves as a latch for report_buffer: > > + * report_buffer[num_reports % 2] is the currently available report, > > + * report_buffer[(num_reports + 1) % 2] is the report being recorded. > > + */ > > +static atomic_t num_reports; > > + > > +/* > > + * PID of the task currently recording the report, as returned by > > + * get_encoded_pid(), or -1. Used as a writer lock for report_buffer. > > + * A regular spinlock couldn't be used here, as probe_console() can be called > > + * from any thread, and it needs to know whether that thread is holding the > > + * lock. > > + */ > > +static atomic_t current_pid = ATOMIC_INIT(-1); > > how do you handle pid namespaces? Doesn't current->pid hold the global PID of the task? See the description of task_pid_nr() here: https://elixir.bootlin.com/linux/latest/source/include/linux/sched.h#L1386, which is supposed to return a global task ID. > > + if (atomic_cmpxchg_acquire(¤t_pid, -1, get_encoded_pid()) != -1) > > + return; > > pid namespaces? See above. > > pid namespaces? > Same. > > + int idx; > > + > > + if (pid != get_encoded_pid()) > > + return; > > + > > + idx = (atomic_read(&num_reports) + 1) % 2; > > You read, but it could change before: Not sure I follow. num_reports can be only incremented by the same task that started the report, this cannot happen concurrently. > > > + if (current_pos == BUF_SIZE) > > + report_buffer[idx][current_pos - 1] = 0; > > + else > > + report_buffer[idx][current_pos] = 0; > > + > > + /* Pairs with acquire in last_report_show(). */ > > + atomic_inc_return_release(&num_reports); > > Not good? > > +static ssize_t last_report_show(struct kobject *kobj, > > + struct kobj_attribute *attr, char *buf) > > +{ > > + ssize_t ret; > > + int index; > > + > > + do { > > + /* Pairs with release in probe_report_end(). */ > > + index = atomic_read_acquire(&num_reports); > > + /* > > + * If index and old_index mismatch, we might be accessing > > + * report_buffer concurrently with a writer thread. In that > > + * case the read data will be discarded. > > + */ > > + ret = data_race(strscpy(buf, report_buffer[index % 2], BUF_SIZE)); > > + /* > > + * Prevent reordering between the memcpy above and the atomic > > + * read below. > > + * See the comments in include/linux/seqlock.h for more > > + * details. > > + */ > > + smp_rmb(); > > + } while (index != atomic_read(&num_reports)); > > endless loops, what could go wrong... Fair enough, this needs to be fixed. > > Why are you rolling your own hacky locks in here? We've also considered using a seqlock here, but thought that required too much boilerplate code (the current implementation reuses the report counter as a seqlock latch, whereas otherwise we'd need to introduce an extra seqcount_latch_t plus call the seqlock API functions). I think this can be reconsidered. > And again, sysfs is "one value" not "one buffer". > > > + return ret; > > +} > > + > > +/* > > + * read() handler for /sys/kernel/error_report/report_count. > > + */ > > +static ssize_t report_count_show(struct kobject *kobj, > > + struct kobj_attribute *attr, char *buf) > > +{ > > + return scnprintf(buf, PAGE_SIZE, "%d\n", atomic_read(&num_reports)); > > sysfs_emit()? Good, haven't seen that one. I think I just took Documentation/filesystems/sysfs.txt as an example. > And you just read it, but what keeps it from changing? Nothing; we can't really guarantee nobody reported another error while we were processing the previous one. Similarly, we cannot be sure that any other vfs file still has the same contents once we read it ;) > > +static const struct attribute_group error_report_sysfs_attr_group = { > > + .attrs = error_report_sysfs_attrs, > > +}; > > ATTRIBUTE_GROUPS()? Ack. > > +late_initcall(error_report_notify_setup); > > You never clean up the kobject or files? Will fix, thanks! > Anyway, please move this to tracefs, that's where it belongs. Will do in v3.