Received: by 2002:a25:f815:0:0:0:0:0 with SMTP id u21csp3556389ybd; Tue, 25 Jun 2019 04:46:00 -0700 (PDT) X-Google-Smtp-Source: APXvYqxRTKCqfpeRjLHpPk8txWR36uhk4TI8wzwWgnXTaUPe0F01QypqB2Y2ARxK9N1UMqACYRmD X-Received: by 2002:a65:62ca:: with SMTP id m10mr2234759pgv.57.1561463160358; Tue, 25 Jun 2019 04:46:00 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1561463160; cv=none; d=google.com; s=arc-20160816; b=KA2RUB0dpxNOSCbcT+DzNldlo16qd8Qs4UXX9VIl15cEncXHktQ62V0T7YsYtKSz6/ 3gnG0KD3bbObQrE2S3yqj9/EPFZXi6Ie3uETfxBdICajBac8wOPZnoCxkX+pzVF2JNa0 wVKt3PMFDLTamQWi62ICdx4AXspmkgajayW9vLN5219MKG6uo1PUqnq9T6rT4sWreVnT +3DwsATPJtarCoSkLHOztryx6O58fHpB4XZq2OZzbiOrMIx7Dk/iz8p9+VRPBdQBuY5v XTTS5sk2bHyFKqlVs4mjFeyjeZcnDTTWMGPnETYB7hjchpWzuUwUOHc1zA4HcXbZdU9r b7Ew== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:mime-version:user-agent:message-id :in-reply-to:date:references:subject:cc:to:from; bh=RggIlgjN31/kfTQVIjJwgUzPBSgKVrsE6dGVglrt7Ak=; b=xrEK3lf9ComET7OJVbjDLErh3We60Z1EQEu8zp1btjCEHxcIKFO5qAPvntGLZL0opR b20F+MiANK5JCTiHA9WMzDYxVtcUlxzp9Z6KHCLx1I8QVBGqCxBJJnodgBOplcAKSERF lQruwoMTnm2mQyHDuDFCfz+0NjcY31pc7BjcpVfiIzBkPUDU8iqBOBEhmB3myC+Z0KyC 91uX1c3j0id8gQ2Hp8Abba9+n1fDBnkc8hT+iTIYrXpNEiQJa0yMdVNSd2SpBnvENObL ZTX38lWmLBD4gqZ/vAl3D4GYo8qYUJQk25PzZxQ4PtLHGmCOm84cTVeCQYioVupAjeBw 6OqQ== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id k8si12403709pgq.28.2019.06.25.04.45.44; Tue, 25 Jun 2019 04:46:00 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728524AbfFYJTi (ORCPT + 99 others); Tue, 25 Jun 2019 05:19:38 -0400 Received: from Galois.linutronix.de ([193.142.43.55]:41403 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726631AbfFYJTh (ORCPT ); Tue, 25 Jun 2019 05:19:37 -0400 Received: from localhost ([127.0.0.1] helo=vostro.local) by Galois.linutronix.de with esmtp (Exim 4.80) (envelope-from ) id 1hfhbT-0001K7-LU; Tue, 25 Jun 2019 11:19:03 +0200 From: John Ogness To: Sergey Senozhatsky Cc: linux-kernel@vger.kernel.org, Peter Zijlstra , Petr Mladek , Steven Rostedt , Linus Torvalds , Greg Kroah-Hartman , Andrea Parri , Thomas Gleixner , Sergey Senozhatsky Subject: Re: [RFC PATCH v2 1/2] printk-rb: add a new printk ringbuffer implementation References: <20190607162349.18199-1-john.ogness@linutronix.de> <20190607162349.18199-2-john.ogness@linutronix.de> <20190625085548.GA532@jagdpanzerIV> Date: Tue, 25 Jun 2019 11:19:02 +0200 In-Reply-To: <20190625085548.GA532@jagdpanzerIV> (Sergey Senozhatsky's message of "Tue, 25 Jun 2019 17:55:48 +0900") Message-ID: <87blymhvjt.fsf@linutronix.de> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.4 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2019-06-25, Sergey Senozhatsky wrote: > [..] >> +static void add_descr_list(struct prb_reserved_entry *e) >> +{ >> + struct printk_ringbuffer *rb = e->rb; >> + struct prb_list *l = &rb->descr_list; >> + struct prb_descr *d = e->descr; >> + struct prb_descr *newest_d; >> + unsigned long newest_id; >> + >> + /* set as newest */ >> + do { >> + /* MB5: synchronize add descr */ >> + newest_id = smp_load_acquire(&l->newest); >> + newest_d = TO_DESCR(rb, newest_id); >> + >> + if (newest_id == EOL) >> + WRITE_ONCE(d->seq, 1); >> + else >> + WRITE_ONCE(d->seq, READ_ONCE(newest_d->seq) + 1); >> + /* >> + * MB5: synchronize add descr >> + * >> + * In particular: next written before cmpxchg >> + */ >> + } while (cmpxchg_release(&l->newest, newest_id, e->id) != newest_id); >> + >> + if (unlikely(newest_id == EOL)) { >> + /* no previous newest means we *are* the list, set oldest */ >> + >> + /* >> + * MB UNPAIRED >> + * >> + * In particular: Force cmpxchg _after_ cmpxchg on newest. >> + */ >> + WARN_ON_ONCE(cmpxchg_release(&l->oldest, EOL, e->id) != EOL); This WARN_ON_ONCE... >> + } else { >> + /* link to previous chain */ >> + >> + /* >> + * MB6: synchronize link descr >> + * >> + * In particular: Force cmpxchg _after_ cmpxchg on newest. >> + */ >> + WARN_ON_ONCE(cmpxchg_release(&newest_d->next, >> + EOL, e->id) != EOL); ... and this WARN_ON_ONCE should both really be BUG_ON. These situations will not happen. Actually, they should both be xchg_release(). But until everyone is happy with the memory barriers, I wanted to leave this bug checking in place. >> + } >> +} > > [..] > >> +char *prb_reserve(struct prb_reserved_entry *e, struct printk_ringbuffer *rb, >> + unsigned int size) >> +{ >> + struct prb_datablock *b; >> + struct prb_descr *d; >> + char *buf; >> + >> + if (size == 0) >> + return NULL; >> + >> + size += sizeof(struct prb_datablock); >> + size = DATA_ALIGN_SIZE(size); >> + if (size > DATAARRAY_SIZE(rb)) >> + return NULL; >> + >> + e->rb = rb; >> + >> + local_irq_save(e->irqflags); >> + >> + if (!assign_descr(e)) >> + goto err_out; >> + >> + d = e->descr; >> + WRITE_ONCE(d->id, e->id); >> + >> + if (!data_reserve(e, size)) { >> + /* put invalid descriptor on list, can still be traversed */ >> + WRITE_ONCE(d->next, EOL); >> + add_descr_list(e); >> + goto err_out; >> + } > > I'm wondering if prb can always report about its problems. Including the > cases when things "go rather bad". > > Suppose we have > > printk() > prb_reserve() > !data_reserve() > add_descr_list() > WARN_ON_ONCE() > printk() > prb_reserve() > !assign_descr(e) << lost WARN_ON's "printk" or "printks"? > > In general, assuming that there might be more error printk-s either > called directly directly from prb->printk on indirectly, from > prb->ABC->printk. > > Also note, > Lost printk-s are not going to be accounted as 'lost' automatically. > It seems that for printk() there is no way to find out that it has > recursed from printk->prb_commit but hasn't succeeded in storing > recursive messages. I'd say that prb_reserve() err_out should probably > &rb->lost++. This is a good point. I have no problems with that. In that case, it should probably be called "fail" instead of "lost". John Ogness