* Re: [PATCH v2 0/2] Add CCPI2 PMU support
From: Ganapatrao Kulkarni @ 2019-07-23 4:33 UTC (permalink / raw)
To: Ganapatrao Kulkarni
Cc: linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Will.Deacon@arm.com,
mark.rutland@arm.com, corbet@lwn.net,
Jayachandran Chandrasekharan Nair, rrichter@marvell.coma,
Jan Glauber
In-Reply-To: <1562317967-16329-1-git-send-email-gkulkarni@marvell.com>
Hi Will,
Any further comments for this patch?
On Fri, Jul 5, 2019 at 2:43 PM Ganapatrao Kulkarni
<gkulkarni@marvell.com> wrote:
>
> Add Cavium Coherent Processor Interconnect (CCPI2) PMU
> support in ThunderX2 Uncore driver.
>
> v2: Updated with review comments [1]
>
> [1] https://lkml.org/lkml/2019/6/14/965
>
> v1: initial patch
>
> Ganapatrao Kulkarni (2):
> Documentation: perf: Update documentation for ThunderX2 PMU uncore
> driver
> drivers/perf: Add CCPI2 PMU support in ThunderX2 UNCORE driver.
>
> Documentation/perf/thunderx2-pmu.txt | 20 ++-
> drivers/perf/thunderx2_pmu.c | 248 +++++++++++++++++++++++----
> 2 files changed, 225 insertions(+), 43 deletions(-)
>
> --
> 2.17.1
>
Thanks,
Ganapat
^ permalink raw reply
* [PATCH 2/2] kernel-doc: core-api: Include string.h into core-api
From: Joe Perches @ 2019-07-23 0:38 UTC (permalink / raw)
To: Linus Torvalds, linux-kernel
Cc: Jonathan Corbet, Stephen Kitt, Kees Cook, Nitin Gote, jannh,
kernel-hardening, Rasmus Villemoes, Andrew Morton, linux-doc
In-Reply-To: <cover.1563841972.git.joe@perches.com>
core-api should show all the various string functions including the
newly added stracpy and stracpy_pad.
Miscellanea:
o Update the Returns: value for strscpy
o fix a defect with %NUL)
Signed-off-by: Joe Perches <joe@perches.com>
---
Documentation/core-api/kernel-api.rst | 3 +++
include/linux/string.h | 5 +++--
lib/string.c | 10 ++++++----
3 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/Documentation/core-api/kernel-api.rst b/Documentation/core-api/kernel-api.rst
index 08af5caf036d..f77de49b1d51 100644
--- a/Documentation/core-api/kernel-api.rst
+++ b/Documentation/core-api/kernel-api.rst
@@ -42,6 +42,9 @@ String Manipulation
.. kernel-doc:: lib/string.c
:export:
+.. kernel-doc:: include/linux/string.h
+ :internal:
+
.. kernel-doc:: mm/util.c
:functions: kstrdup kstrdup_const kstrndup kmemdup kmemdup_nul memdup_user
vmemdup_user strndup_user memdup_user_nul
diff --git a/include/linux/string.h b/include/linux/string.h
index f80b0973f0e5..329188fffc11 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -515,8 +515,9 @@ static inline void memcpy_and_pad(void *dest, size_t dest_len,
* But this can lead to bugs due to typos, or if prefix is a pointer
* and not a constant. Instead use str_has_prefix().
*
- * Returns: 0 if @str does not start with @prefix
- strlen(@prefix) if @str does start with @prefix
+ * Returns:
+ * * strlen(@prefix) if @str starts with @prefix
+ * * 0 if @str does not start with @prefix
*/
static __always_inline size_t str_has_prefix(const char *str, const char *prefix)
{
diff --git a/lib/string.c b/lib/string.c
index 461fb620f85f..53582b6dce2a 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -173,8 +173,9 @@ EXPORT_SYMBOL(strlcpy);
* doesn't unnecessarily force the tail of the destination buffer to be
* zeroed. If zeroing is desired please use strscpy_pad().
*
- * Return: The number of characters copied (not including the trailing
- * %NUL) or -E2BIG if the destination buffer wasn't big enough.
+ * Returns:
+ * * The number of characters copied (not including the trailing %NUL)
+ * * -E2BIG if count is 0.
*/
ssize_t strscpy(char *dest, const char *src, size_t count)
{
@@ -253,8 +254,9 @@ EXPORT_SYMBOL(strscpy);
* For full explanation of why you may want to consider using the
* 'strscpy' functions please see the function docstring for strscpy().
*
- * Return: The number of characters copied (not including the trailing
- * %NUL) or -E2BIG if the destination buffer wasn't big enough.
+ * Returns:
+ * * The number of characters copied (not including the trailing %NUL)
+ * * -E2BIG if count is 0.
*/
ssize_t strscpy_pad(char *dest, const char *src, size_t count)
{
--
2.15.0
^ permalink raw reply related
* [PATCH 0/2] string: Add stracpy and stracpy_pad
From: Joe Perches @ 2019-07-23 0:38 UTC (permalink / raw)
To: Linus Torvalds, linux-kernel
Cc: Jonathan Corbet, Stephen Kitt, Kees Cook, Nitin Gote, jannh,
kernel-hardening, Rasmus Villemoes, Andrew Morton, linux-doc
Add more string copy mechanisms to help avoid defects
Joe Perches (2):
string: Add stracpy and stracpy_pad mechanisms
kernel-doc: core-api: Include string.h into core-api
Documentation/core-api/kernel-api.rst | 3 +++
include/linux/string.h | 46 +++++++++++++++++++++++++++++++++--
lib/string.c | 10 +++++---
3 files changed, 53 insertions(+), 6 deletions(-)
--
2.15.0
^ permalink raw reply
* Re: [PATCH v9 04/18] kunit: test: add kunit_stream a std::stream like logger
From: Brendan Higgins @ 2019-07-23 0:32 UTC (permalink / raw)
To: Stephen Boyd
Cc: Frank Rowand, Greg KH, Josh Poimboeuf, Kees Cook, Kieran Bingham,
Luis Chamberlain, Peter Zijlstra, Rob Herring, shuah,
Theodore Ts'o, Masahiro Yamada, devicetree, dri-devel,
kunit-dev, open list:DOCUMENTATION, linux-fsdevel, linux-kbuild,
Linux Kernel Mailing List, open list:KERNEL SELFTEST FRAMEWORK,
linux-nvdimm, linux-um, Sasha Levin, Bird, Timothy,
Amir Goldstein, Dan Carpenter, Daniel Vetter, Jeff Dike,
Joel Stanley, Julia Lawall, Kevin Hilman, Knut Omang,
Logan Gunthorpe, Michael Ellerman, Petr Mladek, Randy Dunlap,
Richard Weinberger, David Rientjes, Steven Rostedt, wfg
In-Reply-To: <20190722235411.06C1320840@mail.kernel.org>
On Mon, Jul 22, 2019 at 4:54 PM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Brendan Higgins (2019-07-22 15:30:49)
> > On Mon, Jul 22, 2019 at 1:03 PM Stephen Boyd <sboyd@kernel.org> wrote:
> > >
> > >
> > > What's the calling context of the assertions and expectations? I still
> > > don't like the fact that string stream needs to allocate buffers and
> > > throw them into a list somewhere because the calling context matters
> > > there.
> >
> > The calling context is the same as before, which is anywhere.
>
> Ok. That's concerning then.
Yeah. Luis suggested just not supporting the IRQ context until later.
See my later comment.
> > > I'd prefer we just wrote directly to the console/log via printk
> > > instead. That way things are simple because we use the existing
> > > buffering path of printk, but maybe there's some benefit to the string
> > > stream that I don't see? Right now it looks like it builds a string and
> > > then dumps it to printk so I'm sort of lost what the benefit is over
> > > just writing directly with printk.
> >
> > It's just buffering it so the whole string gets printed uninterrupted.
> > If we were to print out piecemeal to printk, couldn't we have another
> > call to printk come in causing it to garble the KUnit message we are
> > in the middle of printing?
>
> Yes, printing piecemeal by calling printk many times could lead to
> interleaving of messages if something else comes in such as an interrupt
> printing something. Printk has some support to hold "records" but I'm
> not sure how that would work here because KERN_CONT talks about only
> being used early on in boot code. I haven't looked at printk in detail
> though so maybe I'm all wrong and KERN_CONT just works?
It says KERN_CONT is not SMP safe, and it isn't supposed to contain
newlines, so it doesn't sound like it does any buffering for you. I
looked at it a while ago and those comments agreed with my
understanding of the code, but I could be wrong.
> Can printk be called once with whatever is in the struct?
Unfortunately, no. That is part of what I was trying to illustrate
with this patch. Most of the messages are deterministic, but
hardcoding all the possible message types would lead to a massive
number of hard coded strings. However, even this would break down for
the mocking formatters. All the different ways a function can be
called are just too complex to encode into a finite set of hard coded
fmt strings.
> Otherwise if
> this is about making printk into a structured log then maybe printk
> isn't the proper solution anyway. Maybe a dev interface should be used
> instead that can handle starting and stopping tests (via ioctl) in
> addition to reading test results, records, etc. with read() and a
> clearing of the records. Then the seqfile API works naturally. All of
Ehhh...I wouldn't mind providing such an interface, but I would really
like to be able to provide the results without having to depend on a
userland doing something to get test results. That has always been a
pretty important goal for me.
> this is a bit premature, but it looks like you're going down the path of
> making something akin to ftrace that stores binary formatted
> assertion/expectation records in a lockless ring buffer that then
> formats those records when the user asks for them.
Like you said, I think it is a bit premature to go that far.
In anycase, I don't see a way to get rid of string_stream, without
significantly sacrificing usability.
> I can imagine someone wanting to write unit tests that check conditions
> from a simulated hardirq context via irq works (a driver mock
> framework?), so this doesn't seem far off.
Yep, I actually presented the first pieces of that in the RFC v1 that
I linked to you earlier in this discussion. I have a more fleshed out
example here:
https://kunit.googlesource.com/linux/+/e10484ad2f9fc7926412ec84739fe105981b4771/drivers/i2c/busses/i2c-aspeed-test.c
I actually already have some people at Google playing around with it.
So yeah, not far off at all! However, in these cases we are not
actually running in the IRQ context (despite the fact that we are
testing IRQ code) because we provide a fake IRQ chip, or some other
fake mechanism that triggers the IRQ. Still, I could see someone
wanting to do it in a non-fake-IRQ context.
Luis' suggestion was just to hold off on the IRQ safe stuff at the
outset, since that is going to require a lot more effort to review. I
know that's kind of the future coding argument again, but maybe the
answer will be to just restrict what features an IRQ user has access
to (maybe just really simple expectations, for example). I mean, we
will probably have to restrict what they are allowed to use anyway.
Luis, do you have any ideas?
Cheers
^ permalink raw reply
* Re: [PATCH v9 04/18] kunit: test: add kunit_stream a std::stream like logger
From: Stephen Boyd @ 2019-07-22 23:54 UTC (permalink / raw)
To: Brendan Higgins
Cc: Frank Rowand, Greg KH, Josh Poimboeuf, Kees Cook, Kieran Bingham,
Luis Chamberlain, Peter Zijlstra, Rob Herring, shuah,
Theodore Ts'o, Masahiro Yamada, devicetree, dri-devel,
kunit-dev, open list:DOCUMENTATION, linux-fsdevel, linux-kbuild,
Linux Kernel Mailing List, open list:KERNEL SELFTEST FRAMEWORK,
linux-nvdimm, linux-um, Sasha Levin, Bird, Timothy,
Amir Goldstein, Dan Carpenter, Daniel Vetter, Jeff Dike,
Joel Stanley, Julia Lawall, Kevin Hilman, Knut Omang,
Logan Gunthorpe, Michael Ellerman, Petr Mladek, Randy Dunlap,
Richard Weinberger, David Rientjes, Steven Rostedt, wfg
In-Reply-To: <CAFd5g45hdCxEavSxirr0un_uLzo5Z-J4gHRA06qjzcQrTzmjVg@mail.gmail.com>
Quoting Brendan Higgins (2019-07-22 15:30:49)
> On Mon, Jul 22, 2019 at 1:03 PM Stephen Boyd <sboyd@kernel.org> wrote:
> >
> >
> > What's the calling context of the assertions and expectations? I still
> > don't like the fact that string stream needs to allocate buffers and
> > throw them into a list somewhere because the calling context matters
> > there.
>
> The calling context is the same as before, which is anywhere.
Ok. That's concerning then.
>
> > I'd prefer we just wrote directly to the console/log via printk
> > instead. That way things are simple because we use the existing
> > buffering path of printk, but maybe there's some benefit to the string
> > stream that I don't see? Right now it looks like it builds a string and
> > then dumps it to printk so I'm sort of lost what the benefit is over
> > just writing directly with printk.
>
> It's just buffering it so the whole string gets printed uninterrupted.
> If we were to print out piecemeal to printk, couldn't we have another
> call to printk come in causing it to garble the KUnit message we are
> in the middle of printing?
Yes, printing piecemeal by calling printk many times could lead to
interleaving of messages if something else comes in such as an interrupt
printing something. Printk has some support to hold "records" but I'm
not sure how that would work here because KERN_CONT talks about only
being used early on in boot code. I haven't looked at printk in detail
though so maybe I'm all wrong and KERN_CONT just works?
Can printk be called once with whatever is in the struct? Otherwise if
this is about making printk into a structured log then maybe printk
isn't the proper solution anyway. Maybe a dev interface should be used
instead that can handle starting and stopping tests (via ioctl) in
addition to reading test results, records, etc. with read() and a
clearing of the records. Then the seqfile API works naturally. All of
this is a bit premature, but it looks like you're going down the path of
making something akin to ftrace that stores binary formatted
assertion/expectation records in a lockless ring buffer that then
formats those records when the user asks for them.
I can imagine someone wanting to write unit tests that check conditions
from a simulated hardirq context via irq works (a driver mock
framework?), so this doesn't seem far off.
^ permalink raw reply
* Re: [PATCH 1/4] block: elevator.c: Remove now unused elevator= argument
From: Bob Liu @ 2019-07-22 23:51 UTC (permalink / raw)
To: Marcos Paulo de Souza, linux-kernel; +Cc: linux-block, linux-doc, Jens Axboe
In-Reply-To: <20190714053453.1655-2-marcos.souza.org@gmail.com>
On 7/14/19 1:34 PM, Marcos Paulo de Souza wrote:
> Since the inclusion of blk-mq, elevator argument was not being
> considered anymore, and it's utility died long with the legacy IO path,
> now removed too.
>
> Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com>
> ---
> block/elevator.c | 14 --------------
> 1 file changed, 14 deletions(-)
>
> diff --git a/block/elevator.c b/block/elevator.c
> index 2f17d66d0e61..f56d9c7d5cbc 100644
> --- a/block/elevator.c
> +++ b/block/elevator.c
> @@ -135,20 +135,6 @@ static struct elevator_type *elevator_get(struct request_queue *q,
> return e;
> }
>
> -static char chosen_elevator[ELV_NAME_MAX];
> -
> -static int __init elevator_setup(char *str)
> -{
> - /*
> - * Be backwards-compatible with previous kernels, so users
> - * won't get the wrong elevator.
> - */
> - strncpy(chosen_elevator, str, sizeof(chosen_elevator) - 1);
> - return 1;
> -}
> -
> -__setup("elevator=", elevator_setup);
> -
> static struct kobj_type elv_ktype;
>
> struct elevator_queue *elevator_alloc(struct request_queue *q,
>
Reviewed-by: Bob Liu <bob.liu@oracle.com>
^ permalink raw reply
* Re: [PATCH v9 04/18] kunit: test: add kunit_stream a std::stream like logger
From: Brendan Higgins @ 2019-07-22 22:30 UTC (permalink / raw)
To: Stephen Boyd
Cc: Frank Rowand, Greg KH, Josh Poimboeuf, Kees Cook, Kieran Bingham,
Luis Chamberlain, Peter Zijlstra, Rob Herring, shuah,
Theodore Ts'o, Masahiro Yamada, devicetree, dri-devel,
kunit-dev, open list:DOCUMENTATION, linux-fsdevel, linux-kbuild,
Linux Kernel Mailing List, open list:KERNEL SELFTEST FRAMEWORK,
linux-nvdimm, linux-um, Sasha Levin, Bird, Timothy,
Amir Goldstein, Dan Carpenter, Daniel Vetter, Jeff Dike,
Joel Stanley, Julia Lawall, Kevin Hilman, Knut Omang,
Logan Gunthorpe, Michael Ellerman, Petr Mladek, Randy Dunlap,
Richard Weinberger, David Rientjes, Steven Rostedt, wfg
In-Reply-To: <20190722200347.261D3218C9@mail.kernel.org>
On Mon, Jul 22, 2019 at 1:03 PM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Brendan Higgins (2019-07-18 17:08:34)
> > On Thu, Jul 18, 2019 at 12:22:33PM -0700, Brendan Higgins wrote:
> >
> > I started poking around with your suggestion while we are waiting. A
> > couple early observations:
> >
> > 1) It is actually easier to do than I previously thought and will probably
> > help with getting more of the planned TAP output stuff working.
> >
> > That being said, this is still a pretty substantial undertaking and
> > will likely take *at least* a week to implement and properly review.
> > Assuming everything goes extremely well (no unexpected issues on my
> > end, very responsive reviewers, etc).
> >
> > 2) It *will* eliminate the need for kunit_stream.
> >
> > 3) ...but, it *will not* eliminate the need for string_stream.
> >
> > Based on my early observations, I do think it is worth doing, but I
> > don't think it is worth trying to make it in this patchset (unless I
> > have already missed the window, or it is going to be open for a while):
>
> The merge window is over. Typically code needs to be settled a few weeks
> before it opens (i.e. around -rc4 or -rc5) for most maintainers to pick
> up patches for the next merge window.
Yeah, it closed on Sunday, right?
I thought we might be able to squeak in since it was *mostly* settled,
and Shuah sent me an email two weeks ago which I interpreted to mean
she was still willing to take it.
In any case, it doesn't matter now.
> > I do think it will make things much cleaner, but I don't think it will
> > achieve your desired goal of getting rid of an unstructured
> > {kunit|string}_stream style interface; it just adds a layer on top of it
> > that makes it harder to misuse.
>
> Ok.
>
> >
> > I attached a patch of what I have so far at the end of this email so you
> > can see what I am talking about. And of course, if you agree with my
> > assessment, so we can start working on it as a future patch.
> >
> > A couple things in regard to the patch I attached:
> >
> > 1) I wrote it pretty quickly so there are almost definitely mistakes.
> > You should consider it RFC. I did verify it compiles though.
> >
> > 2) Also, I did use kunit_stream in writing it: all occurences should be
> > pretty easy to replace with string_stream; nevertheless, the reason
> > for this is just to make it easier to play with the current APIs. I
> > wanted to have something working before I went through a big tedious
> > refactoring. So sorry if it causes any confusion.
> >
> > 3) I also based the patch on all the KUnit patches I have queued up
> > (includes things like mocking and such) since I want to see how this
> > serialization thing will work with mocks and matchers and things like
> > that.
>
> Great!
>
> >
> > From 53d475d3d56afcf92b452c6d347dbedfa1a17d34 Mon Sep 17 00:00:00 2001
> > From: Brendan Higgins <brendanhiggins@google.com>
> > Date: Thu, 18 Jul 2019 16:08:52 -0700
> > Subject: [PATCH v1] DO NOT MERGE: started playing around with the
> > serialization api
> >
> > ---
> > include/kunit/assert.h | 130 ++++++++++++++++++++++++++++++
> > include/kunit/mock.h | 4 +
> > kunit/Makefile | 3 +-
> > kunit/assert.c | 179 +++++++++++++++++++++++++++++++++++++++++
> > kunit/mock.c | 6 +-
> > 5 files changed, 318 insertions(+), 4 deletions(-)
> > create mode 100644 include/kunit/assert.h
> > create mode 100644 kunit/assert.c
> >
> > diff --git a/include/kunit/assert.h b/include/kunit/assert.h
> > new file mode 100644
> > index 0000000000000..e054fdff4642f
> > --- /dev/null
> > +++ b/include/kunit/assert.h
> > @@ -0,0 +1,130 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Assertion and expectation serialization API.
> > + *
> > + * Copyright (C) 2019, Google LLC.
> > + * Author: Brendan Higgins <brendanhiggins@google.com>
> > + */
> > +
> > +#ifndef _KUNIT_ASSERT_H
> > +#define _KUNIT_ASSERT_H
> > +
> > +#include <kunit/test.h>
> > +#include <kunit/mock.h>
> > +
> > +enum kunit_assert_type {
> > + KUNIT_ASSERTION,
> > + KUNIT_EXPECTATION,
> > +};
> > +
> > +struct kunit_assert {
> > + enum kunit_assert_type type;
> > + const char *line;
> > + const char *file;
> > + struct va_format message;
> > + void (*format)(struct kunit_assert *assert,
> > + struct kunit_stream *stream);
>
> Would passing in the test help too?
Yeah, it would probably be good to put one in `struct kunit_assert`.
> > +};
> > +
> > +void kunit_base_assert_format(struct kunit_assert *assert,
> > + struct kunit_stream *stream);
> > +
> > +void kunit_assert_print_msg(struct kunit_assert *assert,
> > + struct kunit_stream *stream);
> > +
> > +struct kunit_unary_assert {
> > + struct kunit_assert assert;
> > + const char *condition;
> > + bool expected_true;
> > +};
> > +
> > +void kunit_unary_assert_format(struct kunit_assert *assert,
> > + struct kunit_stream *stream);
> > +
> > +struct kunit_ptr_not_err_assert {
> > + struct kunit_assert assert;
> > + const char *text;
> > + void *value;
> > +};
> > +
> > +void kunit_ptr_not_err_assert_format(struct kunit_assert *assert,
> > + struct kunit_stream *stream);
> > +
> > +struct kunit_binary_assert {
> > + struct kunit_assert assert;
> > + const char *operation;
> > + const char *left_text;
> > + long long left_value;
> > + const char *right_text;
> > + long long right_value;
> > +};
> > +
> > +void kunit_binary_assert_format(struct kunit_assert *assert,
> > + struct kunit_stream *stream);
> > +
> > +struct kunit_binary_ptr_assert {
> > + struct kunit_assert assert;
> > + const char *operation;
> > + const char *left_text;
> > + void *left_value;
> > + const char *right_text;
> > + void *right_value;
> > +};
> > +
> > +void kunit_binary_ptr_assert_format(struct kunit_assert *assert,
> > + struct kunit_stream *stream);
> > +
> > +struct kunit_binary_str_assert {
> > + struct kunit_assert assert;
> > + const char *operation;
> > + const char *left_text;
> > + const char *left_value;
> > + const char *right_text;
> > + const char *right_value;
> > +};
> > +
> > +void kunit_binary_str_assert_format(struct kunit_assert *assert,
> > + struct kunit_stream *stream);
> > +
> > +struct kunit_mock_assert {
> > + struct kunit_assert assert;
> > +};
> > +
> > +struct kunit_mock_no_expectations {
> > + struct kunit_mock_assert assert;
> > +};
>
> What's the purpose of making a wrapper struct with no other members?
> Just to make a different struct for some sort of type checking? I guess
> it's OK but I don't think it will be very useful in practice.
Yeah, just for typing purposes. I don't mind integrating this into the
current patchset and then deciding if we want it or not.
> > +
> > +struct kunit_mock_declaration {
> > + const char *function_name;
> > + const char **type_names;
> > + const void **params;
> > + int len;
> > +};
> > +
> > +void kunit_mock_declaration_format(struct kunit_mock_declaration *declaration,
> > + struct kunit_stream *stream);
> > +
> > +struct kunit_matcher_result {
> > + struct kunit_assert assert;
> > +};
> > +
> > +struct kunit_mock_failed_match {
> > + struct list_head node;
> > + const char *expectation_text;
> > + struct kunit_matcher_result *matcher_list;
>
> Minor nitpick: this code could use some const sprinkling.
Will do.
> > + size_t matcher_list_len;
> > +};
> > +
> > +void kunit_mock_failed_match_format(struct kunit_mock_failed_match *match,
> > + struct kunit_stream *stream);
> > +
> > +struct kunit_mock_no_match {
> > + struct kunit_mock_assert assert;
> > + struct kunit_mock_declaration declaration;
> > + struct list_head failed_match_list;
> > +};
> > +
> > +void kunit_mock_no_match_format(struct kunit_assert *assert,
> > + struct kunit_stream *stream);
> > +
> > +#endif /* _KUNIT_ASSERT_H */
> > diff --git a/kunit/assert.c b/kunit/assert.c
> > new file mode 100644
> > index 0000000000000..75bb6922a994e
> > --- /dev/null
> > +++ b/kunit/assert.c
> > @@ -0,0 +1,179 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Assertion and expectation serialization API.
> > + *
> > + * Copyright (C) 2019, Google LLC.
> > + * Author: Brendan Higgins <brendanhiggins@google.com>
> > + */
> > +#include <kunit/assert.h>
> > +
> > +void kunit_base_assert_format(struct kunit_assert *assert,
> > + struct kunit_stream *stream)
> > +{
> > + const char *expect_or_assert;
> > +
> > + if (assert->type == KUNIT_EXPECTATION)
> > + expect_or_assert = "EXPECTATION";
> > + else
> > + expect_or_assert = "ASSERTION";
>
> Make this is a switch statement so we can have the compiler complain if
> an enum is missing.
Nice call! I didn't know the compiler warned about that. Will fix.
> > +
> > + kunit_stream_add(stream, "%s FAILED at %s:%s\n",
> > + expect_or_assert, assert->file, assert->line);
> > +}
> > +
> > +void kunit_assert_print_msg(struct kunit_assert *assert,
> > + struct kunit_stream *stream)
> > +{
> > + if (assert->message.fmt)
> > + kunit_stream_add(stream, "\n%pV", &assert->message);
> > +}
> > +
> [...]
> > +
> > +void kunit_mock_failed_match_format(struct kunit_mock_failed_match *match,
> > + struct kunit_stream *stream)
> > +{
> > + struct kunit_matcher_result *result;
> > + size_t i;
> > +
> > + kunit_stream_add(stream,
> > + "Tried expectation: %s, but\n",
> > + match->expectation_text);
> > + for (i = 0; i < match->matcher_list_len; i++) {
> > + result = &match->matcher_list[i];
> > + kunit_stream_add(stream, "\t");
> > + result->assert.format(&result->assert, stream);
> > + kunit_stream_add(stream, "\n");
> > + }
>
> What's the calling context of the assertions and expectations? I still
> don't like the fact that string stream needs to allocate buffers and
> throw them into a list somewhere because the calling context matters
> there.
The calling context is the same as before, which is anywhere.
> I'd prefer we just wrote directly to the console/log via printk
> instead. That way things are simple because we use the existing
> buffering path of printk, but maybe there's some benefit to the string
> stream that I don't see? Right now it looks like it builds a string and
> then dumps it to printk so I'm sort of lost what the benefit is over
> just writing directly with printk.
It's just buffering it so the whole string gets printed uninterrupted.
If we were to print out piecemeal to printk, couldn't we have another
call to printk come in causing it to garble the KUnit message we are
in the middle of printing?
> Maybe it's this part that you wrote up above?
>
> > > Nevertheless, I think the debate over the usefulness of the
> > > string_stream and kunit_stream are separate topics. Even if we made
> > > kunit_stream more structured, I am pretty sure I would want to use
> > > string_stream or some variation for constructing the message.
>
> Why do we need string_stream to construct the message? Can't we just
> print it as we process it?
See preceding comment.
^ permalink raw reply
* Re: [PATCH v1 1/2] mm/page_idle: Add support for per-pid page_idle using virtual indexing
From: Andrew Morton @ 2019-07-22 22:06 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: linux-kernel, vdavydov.dev, Brendan Gregg, kernel-team,
Alexey Dobriyan, Al Viro, carmenjackson, Christian Hansen,
Colin Ian King, dancol, David Howells, fmayer, joaodias, joelaf,
Jonathan Corbet, Kees Cook, Kirill Tkhai, Konstantin Khlebnikov,
linux-doc, linux-fsdevel, linux-mm, Michal Hocko, Mike Rapoport,
minchan, minchan, namhyung, sspatil, surenb, Thomas Gleixner,
timmurray, tkjos, Vlastimil Babka, wvw
In-Reply-To: <20190722213205.140845-1-joel@joelfernandes.org>
On Mon, 22 Jul 2019 17:32:04 -0400 "Joel Fernandes (Google)" <joel@joelfernandes.org> wrote:
> The page_idle tracking feature currently requires looking up the pagemap
> for a process followed by interacting with /sys/kernel/mm/page_idle.
> This is quite cumbersome and can be error-prone too. If between
> accessing the per-PID pagemap and the global page_idle bitmap, if
> something changes with the page then the information is not accurate.
Well, it's never going to be "accurate" - something could change one
nanosecond after userspace has read the data...
Presumably with this approach the data will be "more" accurate. How
big a problem has this inaccuracy proven to be in real-world usage?
> More over looking up PFN from pagemap in Android devices is not
> supported by unprivileged process and requires SYS_ADMIN and gives 0 for
> the PFN.
>
> This patch adds support to directly interact with page_idle tracking at
> the PID level by introducing a /proc/<pid>/page_idle file. This
> eliminates the need for userspace to calculate the mapping of the page.
> It follows the exact same semantics as the global
> /sys/kernel/mm/page_idle, however it is easier to use for some usecases
> where looking up PFN is not needed and also does not require SYS_ADMIN.
> It ended up simplifying userspace code, solving the security issue
> mentioned and works quite well. SELinux does not need to be turned off
> since no pagemap look up is needed.
>
> In Android, we are using this for the heap profiler (heapprofd) which
> profiles and pin points code paths which allocates and leaves memory
> idle for long periods of time.
>
> Documentation material:
> The idle page tracking API for virtual address indexing using virtual page
> frame numbers (VFN) is located at /proc/<pid>/page_idle. It is a bitmap
> that follows the same semantics as /sys/kernel/mm/page_idle/bitmap
> except that it uses virtual instead of physical frame numbers.
>
> This idle page tracking API can be simpler to use than physical address
> indexing, since the pagemap for a process does not need to be looked up
> to mark or read a page's idle bit. It is also more accurate than
> physical address indexing since in physical address indexing, address
> space changes can occur between reading the pagemap and reading the
> bitmap. In virtual address indexing, the process's mmap_sem is held for
> the duration of the access.
>
> ...
>
> --- a/mm/page_idle.c
> +++ b/mm/page_idle.c
> @@ -11,6 +11,7 @@
> #include <linux/mmu_notifier.h>
> #include <linux/page_ext.h>
> #include <linux/page_idle.h>
> +#include <linux/sched/mm.h>
>
> #define BITMAP_CHUNK_SIZE sizeof(u64)
> #define BITMAP_CHUNK_BITS (BITMAP_CHUNK_SIZE * BITS_PER_BYTE)
> @@ -28,15 +29,12 @@
> *
> * This function tries to get a user memory page by pfn as described above.
> */
Above comment needs updating or moving?
> -static struct page *page_idle_get_page(unsigned long pfn)
> +static struct page *page_idle_get_page(struct page *page_in)
> {
> struct page *page;
> pg_data_t *pgdat;
>
> - if (!pfn_valid(pfn))
> - return NULL;
> -
> - page = pfn_to_page(pfn);
> + page = page_in;
> if (!page || !PageLRU(page) ||
> !get_page_unless_zero(page))
> return NULL;
>
> ...
>
> +static int page_idle_get_frames(loff_t pos, size_t count, struct mm_struct *mm,
> + unsigned long *start, unsigned long *end)
> +{
> + unsigned long max_frame;
> +
> + /* If an mm is not given, assume we want physical frames */
> + max_frame = mm ? (mm->task_size >> PAGE_SHIFT) : max_pfn;
> +
> + if (pos % BITMAP_CHUNK_SIZE || count % BITMAP_CHUNK_SIZE)
> + return -EINVAL;
> +
> + *start = pos * BITS_PER_BYTE;
> + if (*start >= max_frame)
> + return -ENXIO;
Is said to mean "The system tried to use the device represented by a
file you specified, and it couldnt find the device. This can mean that
the device file was installed incorrectly, or that the physical device
is missing or not correctly attached to the computer."
This doesn't seem appropriate in this usage and is hence possibly
misleading. Someone whose application fails with ENXIO will be
scratching their heads.
> + *end = *start + count * BITS_PER_BYTE;
> + if (*end > max_frame)
> + *end = max_frame;
> + return 0;
> +}
> +
>
> ...
>
> +static void add_page_idle_list(struct page *page,
> + unsigned long addr, struct mm_walk *walk)
> +{
> + struct page *page_get;
> + struct page_node *pn;
> + int bit;
> + unsigned long frames;
> + struct page_idle_proc_priv *priv = walk->private;
> + u64 *chunk = (u64 *)priv->buffer;
> +
> + if (priv->write) {
> + /* Find whether this page was asked to be marked */
> + frames = (addr - priv->start_addr) >> PAGE_SHIFT;
> + bit = frames % BITMAP_CHUNK_BITS;
> + chunk = &chunk[frames / BITMAP_CHUNK_BITS];
> + if (((*chunk >> bit) & 1) == 0)
> + return;
> + }
> +
> + page_get = page_idle_get_page(page);
> + if (!page_get)
> + return;
> +
> + pn = kmalloc(sizeof(*pn), GFP_ATOMIC);
I'm not liking this GFP_ATOMIC. If I'm reading the code correctly,
userspace can ask for an arbitrarily large number of GFP_ATOMIC
allocations by doing a large read. This can potentially exhaust page
reserves which things like networking Rx interrupts need and can make
this whole feature less reliable.
> + if (!pn)
> + return;
> +
> + pn->page = page_get;
> + pn->addr = addr;
> + list_add(&pn->list, &idle_page_list);
> +}
> +
> +static int pte_page_idle_proc_range(pmd_t *pmd, unsigned long addr,
> + unsigned long end,
> + struct mm_walk *walk)
> +{
> + struct vm_area_struct *vma = walk->vma;
> + pte_t *pte;
> + spinlock_t *ptl;
> + struct page *page;
> +
> + ptl = pmd_trans_huge_lock(pmd, vma);
> + if (ptl) {
> + if (pmd_present(*pmd)) {
> + page = follow_trans_huge_pmd(vma, addr, pmd,
> + FOLL_DUMP|FOLL_WRITE);
> + if (!IS_ERR_OR_NULL(page))
> + add_page_idle_list(page, addr, walk);
> + }
> + spin_unlock(ptl);
> + return 0;
> + }
> +
> + if (pmd_trans_unstable(pmd))
> + return 0;
> +
> + pte = pte_offset_map_lock(vma->vm_mm, pmd, addr, &ptl);
> + for (; addr != end; pte++, addr += PAGE_SIZE) {
> + if (!pte_present(*pte))
> + continue;
> +
> + page = vm_normal_page(vma, addr, *pte);
> + if (page)
> + add_page_idle_list(page, addr, walk);
> + }
> +
> + pte_unmap_unlock(pte - 1, ptl);
> + return 0;
> +}
> +
> +ssize_t page_idle_proc_generic(struct file *file, char __user *ubuff,
> + size_t count, loff_t *pos,
> + struct task_struct *tsk, int write)
> +{
> + int ret;
> + char *buffer;
> + u64 *out;
> + unsigned long start_addr, end_addr, start_frame, end_frame;
> + struct mm_struct *mm = file->private_data;
> + struct mm_walk walk = { .pmd_entry = pte_page_idle_proc_range, };
> + struct page_node *cur, *next;
> + struct page_idle_proc_priv priv;
> + bool walk_error = false;
> +
> + if (!mm || !mmget_not_zero(mm))
> + return -EINVAL;
> +
> + if (count > PAGE_SIZE)
> + count = PAGE_SIZE;
> +
> + buffer = kzalloc(PAGE_SIZE, GFP_KERNEL);
> + if (!buffer) {
> + ret = -ENOMEM;
> + goto out_mmput;
> + }
> + out = (u64 *)buffer;
> +
> + if (write && copy_from_user(buffer, ubuff, count)) {
> + ret = -EFAULT;
> + goto out;
> + }
> +
> + ret = page_idle_get_frames(*pos, count, mm, &start_frame, &end_frame);
> + if (ret)
> + goto out;
> +
> + start_addr = (start_frame << PAGE_SHIFT);
> + end_addr = (end_frame << PAGE_SHIFT);
> + priv.buffer = buffer;
> + priv.start_addr = start_addr;
> + priv.write = write;
> + walk.private = &priv;
> + walk.mm = mm;
> +
> + down_read(&mm->mmap_sem);
> +
> + /*
> + * Protects the idle_page_list which is needed because
> + * walk_page_vma() holds ptlock which deadlocks with
> + * page_idle_clear_pte_refs(). So we have to collect all
> + * pages first, and then call page_idle_clear_pte_refs().
> + */
> + spin_lock(&idle_page_list_lock);
> + ret = walk_page_range(start_addr, end_addr, &walk);
> + if (ret)
> + walk_error = true;
> +
> + list_for_each_entry_safe(cur, next, &idle_page_list, list) {
> + int bit, index;
> + unsigned long off;
> + struct page *page = cur->page;
> +
> + if (unlikely(walk_error))
> + goto remove_page;
> +
> + if (write) {
> + page_idle_clear_pte_refs(page);
> + set_page_idle(page);
> + } else {
> + if (page_really_idle(page)) {
> + off = ((cur->addr) >> PAGE_SHIFT) - start_frame;
> + bit = off % BITMAP_CHUNK_BITS;
> + index = off / BITMAP_CHUNK_BITS;
> + out[index] |= 1ULL << bit;
> + }
> + }
> +remove_page:
> + put_page(page);
> + list_del(&cur->list);
> + kfree(cur);
> + }
> + spin_unlock(&idle_page_list_lock);
> +
> + if (!write && !walk_error)
> + ret = copy_to_user(ubuff, buffer, count);
> +
> + up_read(&mm->mmap_sem);
> +out:
> + kfree(buffer);
> +out_mmput:
> + mmput(mm);
> + if (!ret)
> + ret = count;
> + return ret;
> +
> +}
> +
> +ssize_t page_idle_proc_read(struct file *file, char __user *ubuff,
> + size_t count, loff_t *pos, struct task_struct *tsk)
> +{
> + return page_idle_proc_generic(file, ubuff, count, pos, tsk, 0);
> +}
> +
> +ssize_t page_idle_proc_write(struct file *file, char __user *ubuff,
> + size_t count, loff_t *pos, struct task_struct *tsk)
> +{
> + return page_idle_proc_generic(file, ubuff, count, pos, tsk, 1);
> +}
> +
> static int __init page_idle_init(void)
> {
> int err;
>
> + INIT_LIST_HEAD(&idle_page_list);
> +
> err = sysfs_create_group(mm_kobj, &page_idle_attr_group);
> if (err) {
> pr_err("page_idle: register sysfs failed\n");
> --
>
> ...
>
^ permalink raw reply
* [PATCH v1 1/2] mm/page_idle: Add support for per-pid page_idle using virtual indexing
From: Joel Fernandes (Google) @ 2019-07-22 21:32 UTC (permalink / raw)
To: linux-kernel
Cc: Joel Fernandes (Google), vdavydov.dev, Brendan Gregg, kernel-team,
Alexey Dobriyan, Al Viro, Andrew Morton, carmenjackson,
Christian Hansen, Colin Ian King, dancol, David Howells, fmayer,
joaodias, joelaf, Jonathan Corbet, Kees Cook, Kirill Tkhai,
Konstantin Khlebnikov, linux-doc, linux-fsdevel, linux-mm,
Michal Hocko, Mike Rapoport, minchan, minchan, namhyung, sspatil,
surenb, Thomas Gleixner, timmurray, tkjos, Vlastimil Babka, wvw
The page_idle tracking feature currently requires looking up the pagemap
for a process followed by interacting with /sys/kernel/mm/page_idle.
This is quite cumbersome and can be error-prone too. If between
accessing the per-PID pagemap and the global page_idle bitmap, if
something changes with the page then the information is not accurate.
More over looking up PFN from pagemap in Android devices is not
supported by unprivileged process and requires SYS_ADMIN and gives 0 for
the PFN.
This patch adds support to directly interact with page_idle tracking at
the PID level by introducing a /proc/<pid>/page_idle file. This
eliminates the need for userspace to calculate the mapping of the page.
It follows the exact same semantics as the global
/sys/kernel/mm/page_idle, however it is easier to use for some usecases
where looking up PFN is not needed and also does not require SYS_ADMIN.
It ended up simplifying userspace code, solving the security issue
mentioned and works quite well. SELinux does not need to be turned off
since no pagemap look up is needed.
In Android, we are using this for the heap profiler (heapprofd) which
profiles and pin points code paths which allocates and leaves memory
idle for long periods of time.
Documentation material:
The idle page tracking API for virtual address indexing using virtual page
frame numbers (VFN) is located at /proc/<pid>/page_idle. It is a bitmap
that follows the same semantics as /sys/kernel/mm/page_idle/bitmap
except that it uses virtual instead of physical frame numbers.
This idle page tracking API can be simpler to use than physical address
indexing, since the pagemap for a process does not need to be looked up
to mark or read a page's idle bit. It is also more accurate than
physical address indexing since in physical address indexing, address
space changes can occur between reading the pagemap and reading the
bitmap. In virtual address indexing, the process's mmap_sem is held for
the duration of the access.
Cc: vdavydov.dev@gmail.com
Cc: Brendan Gregg <bgregg@netflix.com>
Cc: kernel-team@android.com
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
Internal review -> v1:
Fixes from Suren.
Corrections to change log, docs (Florian, Sandeep)
fs/proc/base.c | 3 +
fs/proc/internal.h | 1 +
fs/proc/task_mmu.c | 57 +++++++
include/linux/page_idle.h | 4 +
mm/page_idle.c | 305 +++++++++++++++++++++++++++++++++-----
5 files changed, 330 insertions(+), 40 deletions(-)
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 77eb628ecc7f..a58dd74606e9 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -3021,6 +3021,9 @@ static const struct pid_entry tgid_base_stuff[] = {
REG("smaps", S_IRUGO, proc_pid_smaps_operations),
REG("smaps_rollup", S_IRUGO, proc_pid_smaps_rollup_operations),
REG("pagemap", S_IRUSR, proc_pagemap_operations),
+#ifdef CONFIG_IDLE_PAGE_TRACKING
+ REG("page_idle", S_IRUSR|S_IWUSR, proc_page_idle_operations),
+#endif
#endif
#ifdef CONFIG_SECURITY
DIR("attr", S_IRUGO|S_IXUGO, proc_attr_dir_inode_operations, proc_attr_dir_operations),
diff --git a/fs/proc/internal.h b/fs/proc/internal.h
index cd0c8d5ce9a1..bc9371880c63 100644
--- a/fs/proc/internal.h
+++ b/fs/proc/internal.h
@@ -293,6 +293,7 @@ extern const struct file_operations proc_pid_smaps_operations;
extern const struct file_operations proc_pid_smaps_rollup_operations;
extern const struct file_operations proc_clear_refs_operations;
extern const struct file_operations proc_pagemap_operations;
+extern const struct file_operations proc_page_idle_operations;
extern unsigned long task_vsize(struct mm_struct *);
extern unsigned long task_statm(struct mm_struct *,
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 4d2b860dbc3f..11ccc53da38e 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -1642,6 +1642,63 @@ const struct file_operations proc_pagemap_operations = {
.open = pagemap_open,
.release = pagemap_release,
};
+
+#ifdef CONFIG_IDLE_PAGE_TRACKING
+static ssize_t proc_page_idle_read(struct file *file, char __user *buf,
+ size_t count, loff_t *ppos)
+{
+ int ret;
+ struct task_struct *tsk = get_proc_task(file_inode(file));
+
+ if (!tsk)
+ return -EINVAL;
+ ret = page_idle_proc_read(file, buf, count, ppos, tsk);
+ put_task_struct(tsk);
+ return ret;
+}
+
+static ssize_t proc_page_idle_write(struct file *file, const char __user *buf,
+ size_t count, loff_t *ppos)
+{
+ int ret;
+ struct task_struct *tsk = get_proc_task(file_inode(file));
+
+ if (!tsk)
+ return -EINVAL;
+ ret = page_idle_proc_write(file, (char __user *)buf, count, ppos, tsk);
+ put_task_struct(tsk);
+ return ret;
+}
+
+static int proc_page_idle_open(struct inode *inode, struct file *file)
+{
+ struct mm_struct *mm;
+
+ mm = proc_mem_open(inode, PTRACE_MODE_READ);
+ if (IS_ERR(mm))
+ return PTR_ERR(mm);
+ file->private_data = mm;
+ return 0;
+}
+
+static int proc_page_idle_release(struct inode *inode, struct file *file)
+{
+ struct mm_struct *mm = file->private_data;
+
+ if (mm)
+ mmdrop(mm);
+ return 0;
+}
+
+const struct file_operations proc_page_idle_operations = {
+ .llseek = mem_lseek, /* borrow this */
+ .read = proc_page_idle_read,
+ .write = proc_page_idle_write,
+ .open = proc_page_idle_open,
+ .release = proc_page_idle_release,
+};
+#endif /* CONFIG_IDLE_PAGE_TRACKING */
+
#endif /* CONFIG_PROC_PAGE_MONITOR */
#ifdef CONFIG_NUMA
diff --git a/include/linux/page_idle.h b/include/linux/page_idle.h
index 1e894d34bdce..f1bc2640d85e 100644
--- a/include/linux/page_idle.h
+++ b/include/linux/page_idle.h
@@ -106,6 +106,10 @@ static inline void clear_page_idle(struct page *page)
}
#endif /* CONFIG_64BIT */
+ssize_t page_idle_proc_write(struct file *file,
+ char __user *buf, size_t count, loff_t *ppos, struct task_struct *tsk);
+ssize_t page_idle_proc_read(struct file *file,
+ char __user *buf, size_t count, loff_t *ppos, struct task_struct *tsk);
#else /* !CONFIG_IDLE_PAGE_TRACKING */
static inline bool page_is_young(struct page *page)
diff --git a/mm/page_idle.c b/mm/page_idle.c
index 295512465065..874a60c41fef 100644
--- a/mm/page_idle.c
+++ b/mm/page_idle.c
@@ -11,6 +11,7 @@
#include <linux/mmu_notifier.h>
#include <linux/page_ext.h>
#include <linux/page_idle.h>
+#include <linux/sched/mm.h>
#define BITMAP_CHUNK_SIZE sizeof(u64)
#define BITMAP_CHUNK_BITS (BITMAP_CHUNK_SIZE * BITS_PER_BYTE)
@@ -28,15 +29,12 @@
*
* This function tries to get a user memory page by pfn as described above.
*/
-static struct page *page_idle_get_page(unsigned long pfn)
+static struct page *page_idle_get_page(struct page *page_in)
{
struct page *page;
pg_data_t *pgdat;
- if (!pfn_valid(pfn))
- return NULL;
-
- page = pfn_to_page(pfn);
+ page = page_in;
if (!page || !PageLRU(page) ||
!get_page_unless_zero(page))
return NULL;
@@ -51,6 +49,15 @@ static struct page *page_idle_get_page(unsigned long pfn)
return page;
}
+static struct page *page_idle_get_page_pfn(unsigned long pfn)
+{
+
+ if (!pfn_valid(pfn))
+ return NULL;
+
+ return page_idle_get_page(pfn_to_page(pfn));
+}
+
static bool page_idle_clear_pte_refs_one(struct page *page,
struct vm_area_struct *vma,
unsigned long addr, void *arg)
@@ -118,6 +125,47 @@ static void page_idle_clear_pte_refs(struct page *page)
unlock_page(page);
}
+/* Helper to get the start and end frame given a pos and count */
+static int page_idle_get_frames(loff_t pos, size_t count, struct mm_struct *mm,
+ unsigned long *start, unsigned long *end)
+{
+ unsigned long max_frame;
+
+ /* If an mm is not given, assume we want physical frames */
+ max_frame = mm ? (mm->task_size >> PAGE_SHIFT) : max_pfn;
+
+ if (pos % BITMAP_CHUNK_SIZE || count % BITMAP_CHUNK_SIZE)
+ return -EINVAL;
+
+ *start = pos * BITS_PER_BYTE;
+ if (*start >= max_frame)
+ return -ENXIO;
+
+ *end = *start + count * BITS_PER_BYTE;
+ if (*end > max_frame)
+ *end = max_frame;
+ return 0;
+}
+
+static bool page_really_idle(struct page *page)
+{
+ if (!page)
+ return false;
+
+ if (page_is_idle(page)) {
+ /*
+ * The page might have been referenced via a
+ * pte, in which case it is not idle. Clear
+ * refs and recheck.
+ */
+ page_idle_clear_pte_refs(page);
+ if (page_is_idle(page))
+ return true;
+ }
+
+ return false;
+}
+
static ssize_t page_idle_bitmap_read(struct file *file, struct kobject *kobj,
struct bin_attribute *attr, char *buf,
loff_t pos, size_t count)
@@ -125,35 +173,21 @@ static ssize_t page_idle_bitmap_read(struct file *file, struct kobject *kobj,
u64 *out = (u64 *)buf;
struct page *page;
unsigned long pfn, end_pfn;
- int bit;
-
- if (pos % BITMAP_CHUNK_SIZE || count % BITMAP_CHUNK_SIZE)
- return -EINVAL;
-
- pfn = pos * BITS_PER_BYTE;
- if (pfn >= max_pfn)
- return 0;
+ int bit, ret;
- end_pfn = pfn + count * BITS_PER_BYTE;
- if (end_pfn > max_pfn)
- end_pfn = max_pfn;
+ ret = page_idle_get_frames(pos, count, NULL, &pfn, &end_pfn);
+ if (ret == -ENXIO)
+ return 0; /* Reads beyond max_pfn do nothing */
+ else if (ret)
+ return ret;
for (; pfn < end_pfn; pfn++) {
bit = pfn % BITMAP_CHUNK_BITS;
if (!bit)
*out = 0ULL;
- page = page_idle_get_page(pfn);
- if (page) {
- if (page_is_idle(page)) {
- /*
- * The page might have been referenced via a
- * pte, in which case it is not idle. Clear
- * refs and recheck.
- */
- page_idle_clear_pte_refs(page);
- if (page_is_idle(page))
- *out |= 1ULL << bit;
- }
+ page = page_idle_get_page_pfn(pfn);
+ if (page && page_really_idle(page)) {
+ *out |= 1ULL << bit;
put_page(page);
}
if (bit == BITMAP_CHUNK_BITS - 1)
@@ -170,23 +204,16 @@ static ssize_t page_idle_bitmap_write(struct file *file, struct kobject *kobj,
const u64 *in = (u64 *)buf;
struct page *page;
unsigned long pfn, end_pfn;
- int bit;
+ int bit, ret;
- if (pos % BITMAP_CHUNK_SIZE || count % BITMAP_CHUNK_SIZE)
- return -EINVAL;
-
- pfn = pos * BITS_PER_BYTE;
- if (pfn >= max_pfn)
- return -ENXIO;
-
- end_pfn = pfn + count * BITS_PER_BYTE;
- if (end_pfn > max_pfn)
- end_pfn = max_pfn;
+ ret = page_idle_get_frames(pos, count, NULL, &pfn, &end_pfn);
+ if (ret)
+ return ret;
for (; pfn < end_pfn; pfn++) {
bit = pfn % BITMAP_CHUNK_BITS;
if ((*in >> bit) & 1) {
- page = page_idle_get_page(pfn);
+ page = page_idle_get_page_pfn(pfn);
if (page) {
page_idle_clear_pte_refs(page);
set_page_idle(page);
@@ -224,10 +251,208 @@ struct page_ext_operations page_idle_ops = {
};
#endif
+/* page_idle tracking for /proc/<pid>/page_idle */
+
+static DEFINE_SPINLOCK(idle_page_list_lock);
+struct list_head idle_page_list;
+
+struct page_node {
+ struct page *page;
+ unsigned long addr;
+ struct list_head list;
+};
+
+struct page_idle_proc_priv {
+ unsigned long start_addr;
+ char *buffer;
+ int write;
+};
+
+static void add_page_idle_list(struct page *page,
+ unsigned long addr, struct mm_walk *walk)
+{
+ struct page *page_get;
+ struct page_node *pn;
+ int bit;
+ unsigned long frames;
+ struct page_idle_proc_priv *priv = walk->private;
+ u64 *chunk = (u64 *)priv->buffer;
+
+ if (priv->write) {
+ /* Find whether this page was asked to be marked */
+ frames = (addr - priv->start_addr) >> PAGE_SHIFT;
+ bit = frames % BITMAP_CHUNK_BITS;
+ chunk = &chunk[frames / BITMAP_CHUNK_BITS];
+ if (((*chunk >> bit) & 1) == 0)
+ return;
+ }
+
+ page_get = page_idle_get_page(page);
+ if (!page_get)
+ return;
+
+ pn = kmalloc(sizeof(*pn), GFP_ATOMIC);
+ if (!pn)
+ return;
+
+ pn->page = page_get;
+ pn->addr = addr;
+ list_add(&pn->list, &idle_page_list);
+}
+
+static int pte_page_idle_proc_range(pmd_t *pmd, unsigned long addr,
+ unsigned long end,
+ struct mm_walk *walk)
+{
+ struct vm_area_struct *vma = walk->vma;
+ pte_t *pte;
+ spinlock_t *ptl;
+ struct page *page;
+
+ ptl = pmd_trans_huge_lock(pmd, vma);
+ if (ptl) {
+ if (pmd_present(*pmd)) {
+ page = follow_trans_huge_pmd(vma, addr, pmd,
+ FOLL_DUMP|FOLL_WRITE);
+ if (!IS_ERR_OR_NULL(page))
+ add_page_idle_list(page, addr, walk);
+ }
+ spin_unlock(ptl);
+ return 0;
+ }
+
+ if (pmd_trans_unstable(pmd))
+ return 0;
+
+ pte = pte_offset_map_lock(vma->vm_mm, pmd, addr, &ptl);
+ for (; addr != end; pte++, addr += PAGE_SIZE) {
+ if (!pte_present(*pte))
+ continue;
+
+ page = vm_normal_page(vma, addr, *pte);
+ if (page)
+ add_page_idle_list(page, addr, walk);
+ }
+
+ pte_unmap_unlock(pte - 1, ptl);
+ return 0;
+}
+
+ssize_t page_idle_proc_generic(struct file *file, char __user *ubuff,
+ size_t count, loff_t *pos,
+ struct task_struct *tsk, int write)
+{
+ int ret;
+ char *buffer;
+ u64 *out;
+ unsigned long start_addr, end_addr, start_frame, end_frame;
+ struct mm_struct *mm = file->private_data;
+ struct mm_walk walk = { .pmd_entry = pte_page_idle_proc_range, };
+ struct page_node *cur, *next;
+ struct page_idle_proc_priv priv;
+ bool walk_error = false;
+
+ if (!mm || !mmget_not_zero(mm))
+ return -EINVAL;
+
+ if (count > PAGE_SIZE)
+ count = PAGE_SIZE;
+
+ buffer = kzalloc(PAGE_SIZE, GFP_KERNEL);
+ if (!buffer) {
+ ret = -ENOMEM;
+ goto out_mmput;
+ }
+ out = (u64 *)buffer;
+
+ if (write && copy_from_user(buffer, ubuff, count)) {
+ ret = -EFAULT;
+ goto out;
+ }
+
+ ret = page_idle_get_frames(*pos, count, mm, &start_frame, &end_frame);
+ if (ret)
+ goto out;
+
+ start_addr = (start_frame << PAGE_SHIFT);
+ end_addr = (end_frame << PAGE_SHIFT);
+ priv.buffer = buffer;
+ priv.start_addr = start_addr;
+ priv.write = write;
+ walk.private = &priv;
+ walk.mm = mm;
+
+ down_read(&mm->mmap_sem);
+
+ /*
+ * Protects the idle_page_list which is needed because
+ * walk_page_vma() holds ptlock which deadlocks with
+ * page_idle_clear_pte_refs(). So we have to collect all
+ * pages first, and then call page_idle_clear_pte_refs().
+ */
+ spin_lock(&idle_page_list_lock);
+ ret = walk_page_range(start_addr, end_addr, &walk);
+ if (ret)
+ walk_error = true;
+
+ list_for_each_entry_safe(cur, next, &idle_page_list, list) {
+ int bit, index;
+ unsigned long off;
+ struct page *page = cur->page;
+
+ if (unlikely(walk_error))
+ goto remove_page;
+
+ if (write) {
+ page_idle_clear_pte_refs(page);
+ set_page_idle(page);
+ } else {
+ if (page_really_idle(page)) {
+ off = ((cur->addr) >> PAGE_SHIFT) - start_frame;
+ bit = off % BITMAP_CHUNK_BITS;
+ index = off / BITMAP_CHUNK_BITS;
+ out[index] |= 1ULL << bit;
+ }
+ }
+remove_page:
+ put_page(page);
+ list_del(&cur->list);
+ kfree(cur);
+ }
+ spin_unlock(&idle_page_list_lock);
+
+ if (!write && !walk_error)
+ ret = copy_to_user(ubuff, buffer, count);
+
+ up_read(&mm->mmap_sem);
+out:
+ kfree(buffer);
+out_mmput:
+ mmput(mm);
+ if (!ret)
+ ret = count;
+ return ret;
+
+}
+
+ssize_t page_idle_proc_read(struct file *file, char __user *ubuff,
+ size_t count, loff_t *pos, struct task_struct *tsk)
+{
+ return page_idle_proc_generic(file, ubuff, count, pos, tsk, 0);
+}
+
+ssize_t page_idle_proc_write(struct file *file, char __user *ubuff,
+ size_t count, loff_t *pos, struct task_struct *tsk)
+{
+ return page_idle_proc_generic(file, ubuff, count, pos, tsk, 1);
+}
+
static int __init page_idle_init(void)
{
int err;
+ INIT_LIST_HEAD(&idle_page_list);
+
err = sysfs_create_group(mm_kobj, &page_idle_attr_group);
if (err) {
pr_err("page_idle: register sysfs failed\n");
--
2.22.0.657.g960e92d24f-goog
^ permalink raw reply related
* [PATCH v1 2/2] doc: Update documentation for page_idle virtual address indexing
From: Joel Fernandes (Google) @ 2019-07-22 21:32 UTC (permalink / raw)
To: linux-kernel
Cc: Joel Fernandes (Google), Alexey Dobriyan, Al Viro, Andrew Morton,
Brendan Gregg, carmenjackson, Christian Hansen, Colin Ian King,
dancol, David Howells, fmayer, joaodias, joelaf, Jonathan Corbet,
Kees Cook, kernel-team, Kirill Tkhai, Konstantin Khlebnikov,
linux-doc, linux-fsdevel, linux-mm, Michal Hocko, Mike Rapoport,
minchan, minchan, namhyung, sspatil, surenb, Thomas Gleixner,
timmurray, tkjos, vdavydov.dev, Vlastimil Babka, wvw
In-Reply-To: <20190722213205.140845-1-joel@joelfernandes.org>
This patch updates the documentation with the new page_idle tracking
feature which uses virtual address indexing.
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
.../admin-guide/mm/idle_page_tracking.rst | 41 +++++++++++++++----
1 file changed, 34 insertions(+), 7 deletions(-)
diff --git a/Documentation/admin-guide/mm/idle_page_tracking.rst b/Documentation/admin-guide/mm/idle_page_tracking.rst
index df9394fb39c2..70d3bf6f1f8c 100644
--- a/Documentation/admin-guide/mm/idle_page_tracking.rst
+++ b/Documentation/admin-guide/mm/idle_page_tracking.rst
@@ -19,10 +19,14 @@ It is enabled by CONFIG_IDLE_PAGE_TRACKING=y.
User API
========
+There are 2 ways to access the idle page tracking API. One uses physical
+address indexing, another uses a simpler virtual address indexing scheme.
-The idle page tracking API is located at ``/sys/kernel/mm/page_idle``.
-Currently, it consists of the only read-write file,
-``/sys/kernel/mm/page_idle/bitmap``.
+Physical address indexing
+-------------------------
+The idle page tracking API for physical address indexing using page frame
+numbers (PFN) is located at ``/sys/kernel/mm/page_idle``. Currently, it
+consists of the only read-write file, ``/sys/kernel/mm/page_idle/bitmap``.
The file implements a bitmap where each bit corresponds to a memory page. The
bitmap is represented by an array of 8-byte integers, and the page at PFN #i is
@@ -74,6 +78,29 @@ See :ref:`Documentation/admin-guide/mm/pagemap.rst <pagemap>` for more
information about ``/proc/pid/pagemap``, ``/proc/kpageflags``, and
``/proc/kpagecgroup``.
+Virtual address indexing
+------------------------
+The idle page tracking API for virtual address indexing using virtual page
+frame numbers (VFN) is located at ``/proc/<pid>/page_idle``. It is a bitmap
+that follows the same semantics as ``/sys/kernel/mm/page_idle/bitmap``
+except that it uses virtual instead of physical frame numbers.
+
+This idle page tracking API can be simpler to use than physical address
+indexing, since the ``pagemap`` for a process does not need to be looked up to
+mark or read a page's idle bit. It is also more accurate than physical address
+indexing since in physical address indexing, address space changes can occur
+between reading the ``pagemap`` and reading the ``bitmap``. In virtual address
+indexing, the process's ``mmap_sem`` is held for the duration of the access.
+
+To estimate the amount of pages that are not used by a workload one should:
+
+ 1. Mark all the workload's pages as idle by setting corresponding bits in
+ ``/proc/<pid>/page_idle``.
+
+ 2. Wait until the workload accesses its working set.
+
+ 3. Read ``/proc/<pid>/page_idle`` and count the number of bits set.
+
.. _impl_details:
Implementation Details
@@ -99,10 +126,10 @@ When a dirty page is written to swap or disk as a result of memory reclaim or
exceeding the dirty memory limit, it is not marked referenced.
The idle memory tracking feature adds a new page flag, the Idle flag. This flag
-is set manually, by writing to ``/sys/kernel/mm/page_idle/bitmap`` (see the
-:ref:`User API <user_api>`
-section), and cleared automatically whenever a page is referenced as defined
-above.
+is set manually, by writing to ``/sys/kernel/mm/page_idle/bitmap`` for physical
+addressing or by writing to ``/proc/<pid>/page_idle`` for virtual
+addressing (see the :ref:`User API <user_api>` section), and cleared
+automatically whenever a page is referenced as defined above.
When a page is marked idle, the Accessed bit must be cleared in all PTEs it is
mapped to, otherwise we will not be able to detect accesses to the page coming
--
2.22.0.657.g960e92d24f-goog
^ permalink raw reply related
* Re: [PATCH] docs/vm: transhuge: fix typo in madvise reference
From: Jonathan Corbet @ 2019-07-22 20:45 UTC (permalink / raw)
To: Jeremy Cline; +Cc: linux-doc, linux-kernel
In-Reply-To: <20190716144908.25843-1-jcline@redhat.com>
On Tue, 16 Jul 2019 10:49:08 -0400
Jeremy Cline <jcline@redhat.com> wrote:
> Fix an off-by-one typo in the transparent huge pages admin
> documentation.
>
> Signed-off-by: Jeremy Cline <jcline@redhat.com>
Applied, thanks.
jon
^ permalink raw reply
* Re: [PATCH v9 04/18] kunit: test: add kunit_stream a std::stream like logger
From: Stephen Boyd @ 2019-07-22 20:03 UTC (permalink / raw)
To: Brendan Higgins
Cc: Frank Rowand, Greg KH, Josh Poimboeuf, Kees Cook, Kieran Bingham,
Luis Chamberlain, Peter Zijlstra, Rob Herring, shuah,
Theodore Ts'o, Masahiro Yamada, devicetree, dri-devel,
kunit-dev, open list:DOCUMENTATION, linux-fsdevel, linux-kbuild,
Linux Kernel Mailing List, open list:KERNEL SELFTEST FRAMEWORK,
linux-nvdimm, linux-um, Sasha Levin, Bird, Timothy,
Amir Goldstein, Dan Carpenter, Daniel Vetter, Jeff Dike,
Joel Stanley, Julia Lawall, Kevin Hilman, Knut Omang,
Logan Gunthorpe, Michael Ellerman, Petr Mladek, Randy Dunlap,
Richard Weinberger, David Rientjes, Steven Rostedt, wfg
In-Reply-To: <20190719000834.GA3228@google.com>
Quoting Brendan Higgins (2019-07-18 17:08:34)
> On Thu, Jul 18, 2019 at 12:22:33PM -0700, Brendan Higgins wrote:
>
> I started poking around with your suggestion while we are waiting. A
> couple early observations:
>
> 1) It is actually easier to do than I previously thought and will probably
> help with getting more of the planned TAP output stuff working.
>
> That being said, this is still a pretty substantial undertaking and
> will likely take *at least* a week to implement and properly review.
> Assuming everything goes extremely well (no unexpected issues on my
> end, very responsive reviewers, etc).
>
> 2) It *will* eliminate the need for kunit_stream.
>
> 3) ...but, it *will not* eliminate the need for string_stream.
>
> Based on my early observations, I do think it is worth doing, but I
> don't think it is worth trying to make it in this patchset (unless I
> have already missed the window, or it is going to be open for a while):
The merge window is over. Typically code needs to be settled a few weeks
before it opens (i.e. around -rc4 or -rc5) for most maintainers to pick
up patches for the next merge window.
> I do think it will make things much cleaner, but I don't think it will
> achieve your desired goal of getting rid of an unstructured
> {kunit|string}_stream style interface; it just adds a layer on top of it
> that makes it harder to misuse.
Ok.
>
> I attached a patch of what I have so far at the end of this email so you
> can see what I am talking about. And of course, if you agree with my
> assessment, so we can start working on it as a future patch.
>
> A couple things in regard to the patch I attached:
>
> 1) I wrote it pretty quickly so there are almost definitely mistakes.
> You should consider it RFC. I did verify it compiles though.
>
> 2) Also, I did use kunit_stream in writing it: all occurences should be
> pretty easy to replace with string_stream; nevertheless, the reason
> for this is just to make it easier to play with the current APIs. I
> wanted to have something working before I went through a big tedious
> refactoring. So sorry if it causes any confusion.
>
> 3) I also based the patch on all the KUnit patches I have queued up
> (includes things like mocking and such) since I want to see how this
> serialization thing will work with mocks and matchers and things like
> that.
Great!
>
> From 53d475d3d56afcf92b452c6d347dbedfa1a17d34 Mon Sep 17 00:00:00 2001
> From: Brendan Higgins <brendanhiggins@google.com>
> Date: Thu, 18 Jul 2019 16:08:52 -0700
> Subject: [PATCH v1] DO NOT MERGE: started playing around with the
> serialization api
>
> ---
> include/kunit/assert.h | 130 ++++++++++++++++++++++++++++++
> include/kunit/mock.h | 4 +
> kunit/Makefile | 3 +-
> kunit/assert.c | 179 +++++++++++++++++++++++++++++++++++++++++
> kunit/mock.c | 6 +-
> 5 files changed, 318 insertions(+), 4 deletions(-)
> create mode 100644 include/kunit/assert.h
> create mode 100644 kunit/assert.c
>
> diff --git a/include/kunit/assert.h b/include/kunit/assert.h
> new file mode 100644
> index 0000000000000..e054fdff4642f
> --- /dev/null
> +++ b/include/kunit/assert.h
> @@ -0,0 +1,130 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Assertion and expectation serialization API.
> + *
> + * Copyright (C) 2019, Google LLC.
> + * Author: Brendan Higgins <brendanhiggins@google.com>
> + */
> +
> +#ifndef _KUNIT_ASSERT_H
> +#define _KUNIT_ASSERT_H
> +
> +#include <kunit/test.h>
> +#include <kunit/mock.h>
> +
> +enum kunit_assert_type {
> + KUNIT_ASSERTION,
> + KUNIT_EXPECTATION,
> +};
> +
> +struct kunit_assert {
> + enum kunit_assert_type type;
> + const char *line;
> + const char *file;
> + struct va_format message;
> + void (*format)(struct kunit_assert *assert,
> + struct kunit_stream *stream);
Would passing in the test help too?
> +};
> +
> +void kunit_base_assert_format(struct kunit_assert *assert,
> + struct kunit_stream *stream);
> +
> +void kunit_assert_print_msg(struct kunit_assert *assert,
> + struct kunit_stream *stream);
> +
> +struct kunit_unary_assert {
> + struct kunit_assert assert;
> + const char *condition;
> + bool expected_true;
> +};
> +
> +void kunit_unary_assert_format(struct kunit_assert *assert,
> + struct kunit_stream *stream);
> +
> +struct kunit_ptr_not_err_assert {
> + struct kunit_assert assert;
> + const char *text;
> + void *value;
> +};
> +
> +void kunit_ptr_not_err_assert_format(struct kunit_assert *assert,
> + struct kunit_stream *stream);
> +
> +struct kunit_binary_assert {
> + struct kunit_assert assert;
> + const char *operation;
> + const char *left_text;
> + long long left_value;
> + const char *right_text;
> + long long right_value;
> +};
> +
> +void kunit_binary_assert_format(struct kunit_assert *assert,
> + struct kunit_stream *stream);
> +
> +struct kunit_binary_ptr_assert {
> + struct kunit_assert assert;
> + const char *operation;
> + const char *left_text;
> + void *left_value;
> + const char *right_text;
> + void *right_value;
> +};
> +
> +void kunit_binary_ptr_assert_format(struct kunit_assert *assert,
> + struct kunit_stream *stream);
> +
> +struct kunit_binary_str_assert {
> + struct kunit_assert assert;
> + const char *operation;
> + const char *left_text;
> + const char *left_value;
> + const char *right_text;
> + const char *right_value;
> +};
> +
> +void kunit_binary_str_assert_format(struct kunit_assert *assert,
> + struct kunit_stream *stream);
> +
> +struct kunit_mock_assert {
> + struct kunit_assert assert;
> +};
> +
> +struct kunit_mock_no_expectations {
> + struct kunit_mock_assert assert;
> +};
What's the purpose of making a wrapper struct with no other members?
Just to make a different struct for some sort of type checking? I guess
it's OK but I don't think it will be very useful in practice.
> +
> +struct kunit_mock_declaration {
> + const char *function_name;
> + const char **type_names;
> + const void **params;
> + int len;
> +};
> +
> +void kunit_mock_declaration_format(struct kunit_mock_declaration *declaration,
> + struct kunit_stream *stream);
> +
> +struct kunit_matcher_result {
> + struct kunit_assert assert;
> +};
> +
> +struct kunit_mock_failed_match {
> + struct list_head node;
> + const char *expectation_text;
> + struct kunit_matcher_result *matcher_list;
Minor nitpick: this code could use some const sprinkling.
> + size_t matcher_list_len;
> +};
> +
> +void kunit_mock_failed_match_format(struct kunit_mock_failed_match *match,
> + struct kunit_stream *stream);
> +
> +struct kunit_mock_no_match {
> + struct kunit_mock_assert assert;
> + struct kunit_mock_declaration declaration;
> + struct list_head failed_match_list;
> +};
> +
> +void kunit_mock_no_match_format(struct kunit_assert *assert,
> + struct kunit_stream *stream);
> +
> +#endif /* _KUNIT_ASSERT_H */
> diff --git a/kunit/assert.c b/kunit/assert.c
> new file mode 100644
> index 0000000000000..75bb6922a994e
> --- /dev/null
> +++ b/kunit/assert.c
> @@ -0,0 +1,179 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Assertion and expectation serialization API.
> + *
> + * Copyright (C) 2019, Google LLC.
> + * Author: Brendan Higgins <brendanhiggins@google.com>
> + */
> +#include <kunit/assert.h>
> +
> +void kunit_base_assert_format(struct kunit_assert *assert,
> + struct kunit_stream *stream)
> +{
> + const char *expect_or_assert;
> +
> + if (assert->type == KUNIT_EXPECTATION)
> + expect_or_assert = "EXPECTATION";
> + else
> + expect_or_assert = "ASSERTION";
Make this is a switch statement so we can have the compiler complain if
an enum is missing.
> +
> + kunit_stream_add(stream, "%s FAILED at %s:%s\n",
> + expect_or_assert, assert->file, assert->line);
> +}
> +
> +void kunit_assert_print_msg(struct kunit_assert *assert,
> + struct kunit_stream *stream)
> +{
> + if (assert->message.fmt)
> + kunit_stream_add(stream, "\n%pV", &assert->message);
> +}
> +
[...]
> +
> +void kunit_mock_failed_match_format(struct kunit_mock_failed_match *match,
> + struct kunit_stream *stream)
> +{
> + struct kunit_matcher_result *result;
> + size_t i;
> +
> + kunit_stream_add(stream,
> + "Tried expectation: %s, but\n",
> + match->expectation_text);
> + for (i = 0; i < match->matcher_list_len; i++) {
> + result = &match->matcher_list[i];
> + kunit_stream_add(stream, "\t");
> + result->assert.format(&result->assert, stream);
> + kunit_stream_add(stream, "\n");
> + }
What's the calling context of the assertions and expectations? I still
don't like the fact that string stream needs to allocate buffers and
throw them into a list somewhere because the calling context matters
there. I'd prefer we just wrote directly to the console/log via printk
instead. That way things are simple because we use the existing
buffering path of printk, but maybe there's some benefit to the string
stream that I don't see? Right now it looks like it builds a string and
then dumps it to printk so I'm sort of lost what the benefit is over
just writing directly with printk.
Maybe it's this part that you wrote up above?
> > Nevertheless, I think the debate over the usefulness of the
> > string_stream and kunit_stream are separate topics. Even if we made
> > kunit_stream more structured, I am pretty sure I would want to use
> > string_stream or some variation for constructing the message.
Why do we need string_stream to construct the message? Can't we just
print it as we process it?
^ permalink raw reply
* Re: [PATCH 1/2] doc:it_IT: align translation to mainline
From: Jonathan Corbet @ 2019-07-22 19:44 UTC (permalink / raw)
To: Federico Vaga; +Cc: linux-doc, linux-kernel
In-Reply-To: <20190718074724.29807-1-federico.vaga@vaga.pv.it>
On Thu, 18 Jul 2019 09:47:24 +0200
Federico Vaga <federico.vaga@vaga.pv.it> wrote:
> The patch translates the following patches in Italian:
>
> d9d7c0c497b8 docs: Note that :c:func: should no longer be used
> 83e8b971f81c sphinx.rst: Add note about code snippets embedded in the text
> cca5e0b8a430 Documentation: PGP: update for newer HW devices
>
> Signed-off-by: Federico Vaga <federico.vaga@vaga.pv.it>
Applied (and the second one as well), thanks.
jon
^ permalink raw reply
* Re: [PATCH 07/22] docs: admin-guide: add auxdisplay files to it after conversion to ReST
From: Miguel Ojeda @ 2019-07-22 19:41 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: Jonathan Corbet, Linux Doc Mailing List
In-Reply-To: <ed8bb8935bb67d294b5e3bee7647dbdd72c5b608.1563792334.git.mchehab+samsung@kernel.org>
On Mon, Jul 22, 2019 at 1:08 PM Mauro Carvalho Chehab
<mchehab+samsung@kernel.org> wrote:
>
> Those two files describe userspace-faced information. While part of
> it might fit on uAPI, it sounds to me that the admin guide is the
> best place for them.
Acked-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
Thanks a lot for all the work on the Docs, Mauro!
On the topic of these files: don't worry too much about these, they
are old drivers (they used the parallel port, which probably nobody
has anymore :-) so I should remove them at some point, I guess.
Cheers,
Miguel
^ permalink raw reply
* Re: [RFC PATCH] string.h: Add stracpy/stracpy_pad (was: Re: [PATCH] checkpatch: Added warnings in favor of strscpy().)
From: Joe Perches @ 2019-07-22 18:35 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Kees Cook, Nitin Gote, akpm, corbet, apw, linux-doc, linux-kernel,
kernel-hardening, Rasmus Villemoes
In-Reply-To: <20190722182703.GE363@bombadil.infradead.org>
On Mon, 2019-07-22 at 11:27 -0700, Matthew Wilcox wrote:
> On Mon, Jul 22, 2019 at 10:58:15AM -0700, Joe Perches wrote:
> > On Mon, 2019-07-22 at 10:43 -0700, Joe Perches wrote:
> > > On Mon, 2019-07-22 at 10:33 -0700, Kees Cook wrote:
> > > > On Thu, Jul 04, 2019 at 05:15:57PM -0700, Joe Perches wrote:
> > > > > On Thu, 2019-07-04 at 13:46 -0700, Joe Perches wrote:
[]
> > > > > +#define stracpy(to, from) \
> > > > > +({ \
> > > > > + size_t size = ARRAY_SIZE(to); \
> > > > > + BUILD_BUG_ON(!__same_type(typeof(*to), char)); \
> > > > > + \
> > > > > + strscpy(to, from, size); \
> > > > > +})
>
> Where does the 'a' in 'stracpy' come from?
No place in particular.
I used it because dst has to be an 'a'rray rather
than a pointer.
^ permalink raw reply
* Re: [RFC PATCH] string.h: Add stracpy/stracpy_pad (was: Re: [PATCH] checkpatch: Added warnings in favor of strscpy().)
From: Matthew Wilcox @ 2019-07-22 18:27 UTC (permalink / raw)
To: Joe Perches
Cc: Kees Cook, Nitin Gote, akpm, corbet, apw, linux-doc, linux-kernel,
kernel-hardening, Rasmus Villemoes
In-Reply-To: <2c959c56c23d0052e5c35ecfa2f6051b17fb2798.camel@perches.com>
On Mon, Jul 22, 2019 at 10:58:15AM -0700, Joe Perches wrote:
> On Mon, 2019-07-22 at 10:43 -0700, Joe Perches wrote:
> > On Mon, 2019-07-22 at 10:33 -0700, Kees Cook wrote:
> > > On Thu, Jul 04, 2019 at 05:15:57PM -0700, Joe Perches wrote:
> > > > On Thu, 2019-07-04 at 13:46 -0700, Joe Perches wrote:
> > > > > On Thu, 2019-07-04 at 11:24 +0530, Nitin Gote wrote:
> > > > > > Added warnings in checkpatch.pl script to :
> > > > > >
> > > > > > 1. Deprecate strcpy() in favor of strscpy().
> > > > > > 2. Deprecate strlcpy() in favor of strscpy().
> > > > > > 3. Deprecate strncpy() in favor of strscpy() or strscpy_pad().
> > > > > >
> > > > > > Updated strncpy() section in Documentation/process/deprecated.rst
> > > > > > to cover strscpy_pad() case.
> > > >
> > > > []
> > > >
> > > > I sent a patch series for some strscpy/strlcpy misuses.
> > > >
> > > > How about adding a macro helper to avoid the misuses like:
> > > > ---
> > > > include/linux/string.h | 16 ++++++++++++++++
> > > > 1 file changed, 16 insertions(+)
> > > >
> > > > diff --git a/include/linux/string.h b/include/linux/string.h
> > > > index 4deb11f7976b..ef01bd6f19df 100644
> > > > --- a/include/linux/string.h
> > > > +++ b/include/linux/string.h
> > > > @@ -35,6 +35,22 @@ ssize_t strscpy(char *, const char *, size_t);
> > > > /* Wraps calls to strscpy()/memset(), no arch specific code required */
> > > > ssize_t strscpy_pad(char *dest, const char *src, size_t count);
> > > >
> > > > +#define stracpy(to, from) \
> > > > +({ \
> > > > + size_t size = ARRAY_SIZE(to); \
> > > > + BUILD_BUG_ON(!__same_type(typeof(*to), char)); \
> > > > + \
> > > > + strscpy(to, from, size); \
> > > > +})
Where does the 'a' in 'stracpy' come from? Googling around finds other
people using a function called stracpy, but it takes different arguments.
http://stracpy.blogspot.com/ takes a size argument, as does
https://docs.polserver.com/doxygen/html/d5/dce/stracpy_8cpp_source.html
The one in the 'Links' webbrowser (can't find a link to its source) seems
like a strdup clone.
^ permalink raw reply
* Re: [RFC PATCH] string.h: Add stracpy/stracpy_pad (was: Re: [PATCH] checkpatch: Added warnings in favor of strscpy().)
From: Kees Cook @ 2019-07-22 18:21 UTC (permalink / raw)
To: Joe Perches
Cc: Nitin Gote, akpm, corbet, apw, linux-doc, linux-kernel,
kernel-hardening, Rasmus Villemoes
In-Reply-To: <2c959c56c23d0052e5c35ecfa2f6051b17fb2798.camel@perches.com>
On Mon, Jul 22, 2019 at 10:58:15AM -0700, Joe Perches wrote:
> On Mon, 2019-07-22 at 10:43 -0700, Joe Perches wrote:
> > On Mon, 2019-07-22 at 10:33 -0700, Kees Cook wrote:
> > > On Thu, Jul 04, 2019 at 05:15:57PM -0700, Joe Perches wrote:
> > > > On Thu, 2019-07-04 at 13:46 -0700, Joe Perches wrote:
> > > > > On Thu, 2019-07-04 at 11:24 +0530, Nitin Gote wrote:
> > > > > > Added warnings in checkpatch.pl script to :
> > > > > >
> > > > > > 1. Deprecate strcpy() in favor of strscpy().
> > > > > > 2. Deprecate strlcpy() in favor of strscpy().
> > > > > > 3. Deprecate strncpy() in favor of strscpy() or strscpy_pad().
> > > > > >
> > > > > > Updated strncpy() section in Documentation/process/deprecated.rst
> > > > > > to cover strscpy_pad() case.
> > > >
> > > > []
> > > >
> > > > I sent a patch series for some strscpy/strlcpy misuses.
> > > >
> > > > How about adding a macro helper to avoid the misuses like:
> > > > ---
> > > > include/linux/string.h | 16 ++++++++++++++++
> > > > 1 file changed, 16 insertions(+)
> > > >
> > > > diff --git a/include/linux/string.h b/include/linux/string.h
> > > > index 4deb11f7976b..ef01bd6f19df 100644
> > > > --- a/include/linux/string.h
> > > > +++ b/include/linux/string.h
> > > > @@ -35,6 +35,22 @@ ssize_t strscpy(char *, const char *, size_t);
> > > > /* Wraps calls to strscpy()/memset(), no arch specific code required */
> > > > ssize_t strscpy_pad(char *dest, const char *src, size_t count);
> > > >
> > > > +#define stracpy(to, from) \
> > > > +({ \
> > > > + size_t size = ARRAY_SIZE(to); \
> > > > + BUILD_BUG_ON(!__same_type(typeof(*to), char)); \
> > > > + \
> > > > + strscpy(to, from, size); \
> > > > +})
> > > > +
> > > > +#define stracpy_pad(to, from) \
> > > > +({ \
> > > > + size_t size = ARRAY_SIZE(to); \
> > > > + BUILD_BUG_ON(!__same_type(typeof(*to), char)); \
> > > > + \
> > > > + strscpy_pad(to, from, size); \
> > > > +})
> > > > +
> > > > #ifndef __HAVE_ARCH_STRCAT
> > > > extern char * strcat(char *, const char *);
> > > > #endif
> > >
> > > This seems like a reasonable addition, yes. I think Coccinelle might
> > > actually be able to find all the existing strscpy(dst, src, sizeof(dst))
> > > cases to jump-start this conversion.
> >
> > I did that. It works. It's a lot of conversions.
> >
> > $ cat str.cpy.cocci
> > @@
> > expression e1;
> > expression e2;
> > @@
> >
> > - strscpy(e1, e2, sizeof(e1))
> > + stracpy(e1, e2)
> >
> > @@
> > expression e1;
> > expression e2;
> > @@
> >
> > - strlcpy(e1, e2, sizeof(e1))
> > + stracpy(e1, e2)
> >
> > > Devil's advocate: this adds yet more string handling functions... will
> > > this cause even more confusion?
> >
> > Documentation is good.
> > Actual in-kernel use and examples better.
>
> btw: I just ran this again and it produces:
>
> $ spatch --in-place -sp-file str.cpy.cocci .
> $ git checkout tools/
> $ git diff --shortstat
> 958 files changed, 2179 insertions(+), 2655 deletions(-)
Cool. Well, assuming no one hates this, let's do it. :) Can you send a
more complete patch with docs, etc? Maybe Linus will take it for late
in the next merge window, perhaps?
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v9 04/18] kunit: test: add kunit_stream a std::stream like logger
From: Brendan Higgins @ 2019-07-22 18:10 UTC (permalink / raw)
To: Stephen Boyd
Cc: Frank Rowand, Greg KH, Josh Poimboeuf, Kees Cook, Kieran Bingham,
Luis Chamberlain, Peter Zijlstra, Rob Herring, shuah,
Theodore Ts'o, Masahiro Yamada, devicetree, dri-devel,
kunit-dev, open list:DOCUMENTATION, linux-fsdevel, linux-kbuild,
Linux Kernel Mailing List, open list:KERNEL SELFTEST FRAMEWORK,
linux-nvdimm, linux-um, Sasha Levin, Bird, Timothy,
Amir Goldstein, Dan Carpenter, Daniel Vetter, Jeff Dike,
Joel Stanley, Julia Lawall, Kevin Hilman, Knut Omang,
Logan Gunthorpe, Michael Ellerman, Petr Mladek, Randy Dunlap,
Richard Weinberger, David Rientjes, Steven Rostedt, wfg
In-Reply-To: <20190719000834.GA3228@google.com>
On Thu, Jul 18, 2019 at 5:08 PM Brendan Higgins
<brendanhiggins@google.com> wrote:
>
> On Thu, Jul 18, 2019 at 12:22:33PM -0700, Brendan Higgins wrote:
> > On Thu, Jul 18, 2019 at 10:50 AM Stephen Boyd <sboyd@kernel.org> wrote:
> > >
> > > Quoting Brendan Higgins (2019-07-16 11:52:01)
> > > > On Tue, Jul 16, 2019 at 10:50 AM Stephen Boyd <sboyd@kernel.org> wrote:
> [...]
> > > Do you have a link to those earlier patches?
> >
> > This is the first patchset:
> >
> > https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1788057.html
> >
> > In particular you can see the code for matching functions here:
> >
> > https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1788073.html
> >
> > And parameter matching code here:
> >
> > https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1788072.html
> >
> > https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1788086.html
> >
> > My apologies in advance, but the code at this early stage had not
> > adopted the kunit_* prefix and was still using the test_* and mock_*
> > prefix. (Hence, struct kunit_stream was known as struct test_stream).
> [...]
> > > The crux of my complaint is that the string stream API is too loosely
> > > defined to be usable. It allows tests to build up a string of
> > > unstructured information, but with certain calling constraints so we
> > > have to tread carefully. If there was more structure to the data that's
> > > being recorded then the test case runner could operate on the data
> > > without having to do string/stream operations, allocations, etc. This
> > > would make the assertion logic much more concrete and specific to kunit,
> > > instead of this small kunit wrapper that's been placed on top of string
> > > stream.
> >
> > Yeah, I can see the point of wanting something that provides more
> > structure than the raw `struct kunit_stream` interface. In fact, it is
> > something I had already started working on, when I had determined it
> > would be a large effort to capture all the variations. I was further
> > put off from the idea when I had been asked to convert the KUnit
> > intermediate format from what I was using to TAP, because, as it is,
> > the current data printed out by KUnit doesn't contain all the data I
> > would like to put in it in a way that best takes advantage of the TAP
> > specification. One problematic area in particular: TAP already
> > provides a way to present a lot of the data I would like to export,
> > but it involves JSON serialization which was an idea that some of the
> > other reviewers understandably weren't too keen on. TAP also wants to
> > report data some time after it is available, which is generally not a
> > good idea for test debug information; you want to make it available as
> > soon as you can or you risk crashing with the data still inside.
> >
> > Hence, I decided we could probably spend a good long while debating
> > how I present the information. So the idea of having a loose
> > definition seemed attractive to me in its own right since it would
> > likely conform to whatever we ended up deciding in the long run. Also,
> > all the better that it was what I already had and no one seemed to
> > mind too much.
> >
> > The only constant I expect is that `struct kunit` will likely need to
> > take an abstract object with a `commit` method, or a `format` method
> > or whatever so it could control when data was going to be printed out
> > to the user. We will probably also use a string builder in there
> > somewhere.
> >
> > > TL;DR: If we can get rid of the string stream API I'd view that as an
> > > improvement because building arbitrary strings in the kernel is complex,
> > > error prone and has calling context concerns.
> >
> > True. No argument there.
> >
> > > Is the intention that other code besides unit tests will use this string
> > > stream API to build up strings? Any targets in mind? This would be a
> > > good way to get the API merged upstream given that its 2019 and we
> > > haven't had such an API in the kernel so far.
> >
> > Someone, (was it you?) asked about code sharing with a string builder
> > thingy that was used for creating structured human readable files, but
> > that seemed like a pretty massive undertaking.
> >
> > Aside from that, no. I would kind of prefered that nobody used it for
> > anything else because I the issues you described.
> >
> > Nevertheless, I think the debate over the usefulness of the
> > string_stream and kunit_stream are separate topics. Even if we made
> > kunit_stream more structured, I am pretty sure I would want to use
> > string_stream or some variation for constructing the message.
> >
> > > An "object oriented" (strong quotes!) approach where kunit_fail_msg is
> > > the innermost struct in some assertion specific structure might work
> > > nicely and allow the test runner to call a generic 'format' function to
> > > print out the message based on the type of assertion/expectation it is.
> > > It probably would mean less code size too because the strings that are
> > > common will be in the common printing function instead of created twice,
> > > in the macros/code and then copied to the heap for the string stream.
> > >
> > > struct kunit_assert {
> > > const char *line;
> > > const char *file;
> > > const char *func;
> > > void (*format)(struct kunit_assert *assert);
> > > };
> > >
> > > struct kunit_comparison_assert {
> > > enum operator operator;
> > > const char *left;
> > > const char *right;
> > > struct kunit_assert assert;
> > > };
> > >
> > > struct kunit_bool_assert {
> > > const char *truth;
> > > const char *statement;
> > > struct kunit_assert assert;
> > > };
> > >
> > > void kunit_format_comparison(struct kunit_assert *assert)
> > > {
> > > struct kunit_comparison_assert *comp = container_of(assert, ...)
> > >
> > > kunit_printk(...)
> > > }
>
> I started poking around with your suggestion while we are waiting. A
> couple early observations:
>
> 1) It is actually easier to do than I previously thought and will probably
> help with getting more of the planned TAP output stuff working.
>
> That being said, this is still a pretty substantial undertaking and
> will likely take *at least* a week to implement and properly review.
> Assuming everything goes extremely well (no unexpected issues on my
> end, very responsive reviewers, etc).
>
> 2) It *will* eliminate the need for kunit_stream.
>
> 3) ...but, it *will not* eliminate the need for string_stream.
>
> Based on my early observations, I do think it is worth doing, but I
> don't think it is worth trying to make it in this patchset (unless I
> have already missed the window, or it is going to be open for a while):
> I do think it will make things much cleaner, but I don't think it will
> achieve your desired goal of getting rid of an unstructured
> {kunit|string}_stream style interface; it just adds a layer on top of it
> that makes it harder to misuse.
>
> I attached a patch of what I have so far at the end of this email so you
> can see what I am talking about. And of course, if you agree with my
> assessment, so we can start working on it as a future patch.
>
> A couple things in regard to the patch I attached:
>
> 1) I wrote it pretty quickly so there are almost definitely mistakes.
> You should consider it RFC. I did verify it compiles though.
>
> 2) Also, I did use kunit_stream in writing it: all occurences should be
> pretty easy to replace with string_stream; nevertheless, the reason
> for this is just to make it easier to play with the current APIs. I
> wanted to have something working before I went through a big tedious
> refactoring. So sorry if it causes any confusion.
>
> 3) I also based the patch on all the KUnit patches I have queued up
> (includes things like mocking and such) since I want to see how this
> serialization thing will work with mocks and matchers and things like
> that.
>
> > I started working on something similarish, but by the time I ended up
> > coming up with a parent object whose definition was loose enough to
> > satisfy all the properties required by the child classes it ended up
> > basically being the same as what I have now just with a more complex
> > hierarchy of message manipulation logic.
> >
> > On the other hand, I didn't have the idea of doing the parent object
> > quite the way you did and that would clean up a lot of the duplicated
> > first line logic.
> >
> > I would like to give it a try, but I am afraid I am going to get
> > sucked down a really deep rabbit hole.
> >
> > > Maybe other people have opinions here on if you should do it now or
> > > later. Future coding is not a great argument because it's hard to
> > > predict the future. On the other hand, this patchset is in good shape to
> >
> > Yeah, that's kind of why I am afraid to go down this road when I have
> > something that works now and I know works with the mocking stuff I
> > want to do.
> >
> > I would like to try your suggestion, but I want to try to make it work
> > with my mocking patches before I commit to it because otherwise I am
> > just going to have to back it out in a follow up patchset.
> >
> > > merge and I'd like to use it to write unit tests for code I maintain so
> > > I don't want to see this stall out. Sorry if I'm opening the can of
> > > worms you're talking about.
> >
> > Don't be sorry. I agree with you that the kunit_stream stuff is not very pretty.
> >
> > Shuah, have we missed the merge window for 5.3?
> >
> > I saw you only sent one PR out so far for this release, and there
> > wasn't much in it; I imagine you are going to send at least one more?
> >
> > I figure, if we still got time to try out your suggestion, Stephen, no
> > harm in trying.
> >
> > Also if we missed it, then I have another couple months to play around with it.
> >
> > What do you think?
I talked to Shuah off thread, she would like us to resolve this
discussion before accepting the patchset.
She also said that this is probably going to have to wait until v5.4.
Nevertheless, Stephen, would you mind taking a look at the patch I
posted below? I would like to get your thoughts on the sum of all the
changes I am going to have to make before I try to integrate them into
the existing patches.
Sorry for being lazy, but I suspect you won't like the first pass of
how I am doing it, and I think it will probably be easier for you to
give early feedback on it as its own change anyway.
> I attached the patch mentioned above below. Let me know what you think!
>
> Cheers!
>
> From 53d475d3d56afcf92b452c6d347dbedfa1a17d34 Mon Sep 17 00:00:00 2001
> From: Brendan Higgins <brendanhiggins@google.com>
> Date: Thu, 18 Jul 2019 16:08:52 -0700
> Subject: [PATCH v1] DO NOT MERGE: started playing around with the
> serialization api
>
> ---
> include/kunit/assert.h | 130 ++++++++++++++++++++++++++++++
> include/kunit/mock.h | 4 +
> kunit/Makefile | 3 +-
> kunit/assert.c | 179 +++++++++++++++++++++++++++++++++++++++++
> kunit/mock.c | 6 +-
> 5 files changed, 318 insertions(+), 4 deletions(-)
> create mode 100644 include/kunit/assert.h
> create mode 100644 kunit/assert.c
>
> diff --git a/include/kunit/assert.h b/include/kunit/assert.h
> new file mode 100644
> index 0000000000000..e054fdff4642f
> --- /dev/null
> +++ b/include/kunit/assert.h
> @@ -0,0 +1,130 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Assertion and expectation serialization API.
> + *
> + * Copyright (C) 2019, Google LLC.
> + * Author: Brendan Higgins <brendanhiggins@google.com>
> + */
> +
> +#ifndef _KUNIT_ASSERT_H
> +#define _KUNIT_ASSERT_H
> +
> +#include <kunit/test.h>
> +#include <kunit/mock.h>
> +
> +enum kunit_assert_type {
> + KUNIT_ASSERTION,
> + KUNIT_EXPECTATION,
> +};
> +
> +struct kunit_assert {
> + enum kunit_assert_type type;
> + const char *line;
> + const char *file;
> + struct va_format message;
> + void (*format)(struct kunit_assert *assert,
> + struct kunit_stream *stream);
> +};
> +
> +void kunit_base_assert_format(struct kunit_assert *assert,
> + struct kunit_stream *stream);
> +
> +void kunit_assert_print_msg(struct kunit_assert *assert,
> + struct kunit_stream *stream);
> +
> +struct kunit_unary_assert {
> + struct kunit_assert assert;
> + const char *condition;
> + bool expected_true;
> +};
> +
> +void kunit_unary_assert_format(struct kunit_assert *assert,
> + struct kunit_stream *stream);
> +
> +struct kunit_ptr_not_err_assert {
> + struct kunit_assert assert;
> + const char *text;
> + void *value;
> +};
> +
> +void kunit_ptr_not_err_assert_format(struct kunit_assert *assert,
> + struct kunit_stream *stream);
> +
> +struct kunit_binary_assert {
> + struct kunit_assert assert;
> + const char *operation;
> + const char *left_text;
> + long long left_value;
> + const char *right_text;
> + long long right_value;
> +};
> +
> +void kunit_binary_assert_format(struct kunit_assert *assert,
> + struct kunit_stream *stream);
> +
> +struct kunit_binary_ptr_assert {
> + struct kunit_assert assert;
> + const char *operation;
> + const char *left_text;
> + void *left_value;
> + const char *right_text;
> + void *right_value;
> +};
> +
> +void kunit_binary_ptr_assert_format(struct kunit_assert *assert,
> + struct kunit_stream *stream);
> +
> +struct kunit_binary_str_assert {
> + struct kunit_assert assert;
> + const char *operation;
> + const char *left_text;
> + const char *left_value;
> + const char *right_text;
> + const char *right_value;
> +};
> +
> +void kunit_binary_str_assert_format(struct kunit_assert *assert,
> + struct kunit_stream *stream);
> +
> +struct kunit_mock_assert {
> + struct kunit_assert assert;
> +};
> +
> +struct kunit_mock_no_expectations {
> + struct kunit_mock_assert assert;
> +};
> +
> +struct kunit_mock_declaration {
> + const char *function_name;
> + const char **type_names;
> + const void **params;
> + int len;
> +};
> +
> +void kunit_mock_declaration_format(struct kunit_mock_declaration *declaration,
> + struct kunit_stream *stream);
> +
> +struct kunit_matcher_result {
> + struct kunit_assert assert;
> +};
> +
> +struct kunit_mock_failed_match {
> + struct list_head node;
> + const char *expectation_text;
> + struct kunit_matcher_result *matcher_list;
> + size_t matcher_list_len;
> +};
> +
> +void kunit_mock_failed_match_format(struct kunit_mock_failed_match *match,
> + struct kunit_stream *stream);
> +
> +struct kunit_mock_no_match {
> + struct kunit_mock_assert assert;
> + struct kunit_mock_declaration declaration;
> + struct list_head failed_match_list;
> +};
> +
> +void kunit_mock_no_match_format(struct kunit_assert *assert,
> + struct kunit_stream *stream);
> +
> +#endif /* _KUNIT_ASSERT_H */
> diff --git a/include/kunit/mock.h b/include/kunit/mock.h
> index 001b96af62f1e..52c9e427c831b 100644
> --- a/include/kunit/mock.h
> +++ b/include/kunit/mock.h
> @@ -144,6 +144,10 @@ void mock_register_formatter(struct mock_param_formatter *formatter);
>
> void mock_unregister_formatter(struct mock_param_formatter *formatter);
>
> +void mock_format_param(struct kunit_stream *stream,
> + const char *type_name,
> + const void *param);
> +
> struct mock *mock_get_global_mock(void);
>
> #define MOCK(name) name##_mock
> diff --git a/kunit/Makefile b/kunit/Makefile
> index bbf43fcfb93a9..149d856a30f04 100644
> --- a/kunit/Makefile
> +++ b/kunit/Makefile
> @@ -3,7 +3,8 @@ obj-$(CONFIG_KUNIT) += test.o \
> common-mocks.o \
> string-stream.o \
> kunit-stream.o \
> - try-catch.o
> + try-catch.o \
> + assert.o
>
> obj-$(CONFIG_KUNIT_TEST) += test-test.o \
> test-mock.o \
> diff --git a/kunit/assert.c b/kunit/assert.c
> new file mode 100644
> index 0000000000000..75bb6922a994e
> --- /dev/null
> +++ b/kunit/assert.c
> @@ -0,0 +1,179 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Assertion and expectation serialization API.
> + *
> + * Copyright (C) 2019, Google LLC.
> + * Author: Brendan Higgins <brendanhiggins@google.com>
> + */
> +#include <kunit/assert.h>
> +
> +void kunit_base_assert_format(struct kunit_assert *assert,
> + struct kunit_stream *stream)
> +{
> + const char *expect_or_assert;
> +
> + if (assert->type == KUNIT_EXPECTATION)
> + expect_or_assert = "EXPECTATION";
> + else
> + expect_or_assert = "ASSERTION";
> +
> + kunit_stream_add(stream, "%s FAILED at %s:%s\n",
> + expect_or_assert, assert->file, assert->line);
> +}
> +
> +void kunit_assert_print_msg(struct kunit_assert *assert,
> + struct kunit_stream *stream)
> +{
> + if (assert->message.fmt)
> + kunit_stream_add(stream, "\n%pV", &assert->message);
> +}
> +
> +void kunit_unary_assert_format(struct kunit_assert *assert,
> + struct kunit_stream *stream)
> +{
> + struct kunit_unary_assert *unary_assert = container_of(
> + assert, struct kunit_unary_assert, assert);
> +
> + kunit_base_assert_format(assert, stream);
> + if (unary_assert->expected_true)
> + kunit_stream_add(stream,
> + "\tExpected %s to be true, but is false\n",
> + unary_assert->condition);
> + else
> + kunit_stream_add(stream,
> + "\tExpected %s to be false, but is true\n",
> + unary_assert->condition);
> + kunit_assert_print_msg(assert, stream);
> +}
> +
> +void kunit_ptr_not_err_assert_format(struct kunit_assert *assert,
> + struct kunit_stream *stream)
> +{
> + struct kunit_ptr_not_err_assert *ptr_assert = container_of(
> + assert, struct kunit_ptr_not_err_assert, assert);
> +
> + kunit_base_assert_format(assert, stream);
> + if (!ptr_assert->value) {
> + kunit_stream_add(stream,
> + "\tExpected %s is not null, but is\n",
> + ptr_assert->text);
> + } else if (IS_ERR(ptr_assert->value)) {
> + kunit_stream_add(stream,
> + "\tExpected %s is not error, but is: %ld\n",
> + ptr_assert->text,
> + PTR_ERR(ptr_assert->value));
> + }
> + kunit_assert_print_msg(assert, stream);
> +}
> +
> +void kunit_binary_assert_format(struct kunit_assert *assert,
> + struct kunit_stream *stream)
> +{
> + struct kunit_binary_assert *binary_assert = container_of(
> + assert, struct kunit_binary_assert, assert);
> +
> + kunit_base_assert_format(assert, stream);
> + kunit_stream_add(stream,
> + "\tExpected %s %s %s, but\n",
> + binary_assert->left_text,
> + binary_assert->operation,
> + binary_assert->right_text);
> + kunit_stream_add(stream, "\t\t%s == %lld\n",
> + binary_assert->left_text,
> + binary_assert->left_value);
> + kunit_stream_add(stream, "\t\t%s == %lld",
> + binary_assert->right_text,
> + binary_assert->right_value);
> + kunit_assert_print_msg(assert, stream);
> +}
> +
I could probably reduce some of the code duplication here by using a
variable type struct for left_value and right_value, but that would
actually increase the usage of {kunit|string}_stream; it is probably
the right thing to do, but I wanted to get your thoughts on it first.
> +void kunit_binary_ptr_assert_format(struct kunit_assert *assert,
> + struct kunit_stream *stream)
> +{
> + struct kunit_binary_ptr_assert *binary_assert = container_of(
> + assert, struct kunit_binary_ptr_assert, assert);
> +
> + kunit_base_assert_format(assert, stream);
> + kunit_stream_add(stream,
> + "\tExpected %s %s %s, but\n",
> + binary_assert->left_text,
> + binary_assert->operation,
> + binary_assert->right_text);
> + kunit_stream_add(stream, "\t\t%s == %pK\n",
> + binary_assert->left_text,
> + binary_assert->left_value);
> + kunit_stream_add(stream, "\t\t%s == %pK",
> + binary_assert->right_text,
> + binary_assert->right_value);
> + kunit_assert_print_msg(assert, stream);
> +}
> +
> +void kunit_binary_str_assert_format(struct kunit_assert *assert,
> + struct kunit_stream *stream)
> +{
> + struct kunit_binary_str_assert *binary_assert = container_of(
> + assert, struct kunit_binary_str_assert, assert);
> +
> + kunit_base_assert_format(assert, stream);
> + kunit_stream_add(stream,
> + "\tExpected %s %s %s, but\n",
> + binary_assert->left_text,
> + binary_assert->operation,
> + binary_assert->right_text);
> + kunit_stream_add(stream, "\t\t%s == %s\n",
> + binary_assert->left_text,
> + binary_assert->left_value);
> + kunit_stream_add(stream, "\t\t%s == %s",
> + binary_assert->right_text,
> + binary_assert->right_value);
> + kunit_assert_print_msg(assert, stream);
> +}
> +
> +void kunit_mock_declaration_format(struct kunit_mock_declaration *declaration,
> + struct kunit_stream *stream)
> +{
> + int i;
> +
> + kunit_stream_add(stream, "%s(", declaration->function_name);
> + for (i = 0; i < declaration->len; i++) {
> + mock_format_param(stream,
> + declaration->type_names[i],
> + declaration->params[i]);
> + if (i < declaration->len - 1)
> + kunit_stream_add(stream, ", ");
> + }
> + kunit_stream_add(stream, ")\n");
> +}
> +
> +void kunit_mock_failed_match_format(struct kunit_mock_failed_match *match,
> + struct kunit_stream *stream)
> +{
> + struct kunit_matcher_result *result;
> + size_t i;
> +
> + kunit_stream_add(stream,
> + "Tried expectation: %s, but\n",
> + match->expectation_text);
> + for (i = 0; i < match->matcher_list_len; i++) {
> + result = &match->matcher_list[i];
> + kunit_stream_add(stream, "\t");
> + result->assert.format(&result->assert, stream);
> + kunit_stream_add(stream, "\n");
> + }
> +}
> +
> +void kunit_mock_no_match_format(struct kunit_assert *assert,
> + struct kunit_stream *stream)
> +{
> + struct kunit_mock_assert *mock_assert = container_of(
> + assert, struct kunit_mock_assert, assert);
> + struct kunit_mock_no_match *no_match = container_of(
> + mock_assert, struct kunit_mock_no_match, assert);
> + struct kunit_mock_failed_match *expectation;
> +
> + kunit_base_assert_format(assert, stream);
> + kunit_mock_declaration_format(&no_match->declaration, stream);
> +
> + list_for_each_entry(expectation, &no_match->failed_match_list, node)
> + kunit_mock_failed_match_format(expectation, stream);
> +}
> diff --git a/kunit/mock.c b/kunit/mock.c
> index ccb0abe111402..ab441a58a918c 100644
> --- a/kunit/mock.c
> +++ b/kunit/mock.c
> @@ -269,9 +269,9 @@ struct mock_param_formatter *mock_find_formatter(const char *type_name)
> return NULL;
> }
>
> -static void mock_format_param(struct kunit_stream *stream,
> - const char *type_name,
> - const void *param)
> +void mock_format_param(struct kunit_stream *stream,
> + const char *type_name,
> + const void *param)
> {
> struct mock_param_formatter *formatter;
>
> --
> 2.22.0.657.g960e92d24f-goog
>
^ permalink raw reply
* Re: [RFC PATCH] string.h: Add stracpy/stracpy_pad (was: Re: [PATCH] checkpatch: Added warnings in favor of strscpy().)
From: Joe Perches @ 2019-07-22 17:58 UTC (permalink / raw)
To: Kees Cook
Cc: Nitin Gote, akpm, corbet, apw, linux-doc, linux-kernel,
kernel-hardening, Rasmus Villemoes
In-Reply-To: <b9bb5550b264d4b29b2b20f7ff8b1b40d20def6a.camel@perches.com>
On Mon, 2019-07-22 at 10:43 -0700, Joe Perches wrote:
> On Mon, 2019-07-22 at 10:33 -0700, Kees Cook wrote:
> > On Thu, Jul 04, 2019 at 05:15:57PM -0700, Joe Perches wrote:
> > > On Thu, 2019-07-04 at 13:46 -0700, Joe Perches wrote:
> > > > On Thu, 2019-07-04 at 11:24 +0530, Nitin Gote wrote:
> > > > > Added warnings in checkpatch.pl script to :
> > > > >
> > > > > 1. Deprecate strcpy() in favor of strscpy().
> > > > > 2. Deprecate strlcpy() in favor of strscpy().
> > > > > 3. Deprecate strncpy() in favor of strscpy() or strscpy_pad().
> > > > >
> > > > > Updated strncpy() section in Documentation/process/deprecated.rst
> > > > > to cover strscpy_pad() case.
> > >
> > > []
> > >
> > > I sent a patch series for some strscpy/strlcpy misuses.
> > >
> > > How about adding a macro helper to avoid the misuses like:
> > > ---
> > > include/linux/string.h | 16 ++++++++++++++++
> > > 1 file changed, 16 insertions(+)
> > >
> > > diff --git a/include/linux/string.h b/include/linux/string.h
> > > index 4deb11f7976b..ef01bd6f19df 100644
> > > --- a/include/linux/string.h
> > > +++ b/include/linux/string.h
> > > @@ -35,6 +35,22 @@ ssize_t strscpy(char *, const char *, size_t);
> > > /* Wraps calls to strscpy()/memset(), no arch specific code required */
> > > ssize_t strscpy_pad(char *dest, const char *src, size_t count);
> > >
> > > +#define stracpy(to, from) \
> > > +({ \
> > > + size_t size = ARRAY_SIZE(to); \
> > > + BUILD_BUG_ON(!__same_type(typeof(*to), char)); \
> > > + \
> > > + strscpy(to, from, size); \
> > > +})
> > > +
> > > +#define stracpy_pad(to, from) \
> > > +({ \
> > > + size_t size = ARRAY_SIZE(to); \
> > > + BUILD_BUG_ON(!__same_type(typeof(*to), char)); \
> > > + \
> > > + strscpy_pad(to, from, size); \
> > > +})
> > > +
> > > #ifndef __HAVE_ARCH_STRCAT
> > > extern char * strcat(char *, const char *);
> > > #endif
> >
> > This seems like a reasonable addition, yes. I think Coccinelle might
> > actually be able to find all the existing strscpy(dst, src, sizeof(dst))
> > cases to jump-start this conversion.
>
> I did that. It works. It's a lot of conversions.
>
> $ cat str.cpy.cocci
> @@
> expression e1;
> expression e2;
> @@
>
> - strscpy(e1, e2, sizeof(e1))
> + stracpy(e1, e2)
>
> @@
> expression e1;
> expression e2;
> @@
>
> - strlcpy(e1, e2, sizeof(e1))
> + stracpy(e1, e2)
>
> > Devil's advocate: this adds yet more string handling functions... will
> > this cause even more confusion?
>
> Documentation is good.
> Actual in-kernel use and examples better.
btw: I just ran this again and it produces:
$ spatch --in-place -sp-file str.cpy.cocci .
$ git checkout tools/
$ git diff --shortstat
958 files changed, 2179 insertions(+), 2655 deletions(-)
^ permalink raw reply
* Re: [PATCH 17/22] docs: mips: add to the documentation body as ReST
From: Paul Burton @ 2019-07-22 17:51 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Jonathan Corbet, Ralf Baechle, James Hogan,
linux-doc@vger.kernel.org, linux-mips@vger.kernel.org
In-Reply-To: <d1b00534f167baba66b1f808e1aed3f7c888c468.1563792334.git.mchehab+samsung@kernel.org>
Hi Mauro,
On Mon, Jul 22, 2019 at 08:07:44AM -0300, Mauro Carvalho Chehab wrote:
> Manually convert the AU1xxx_IDE.README file to ReST and add
> to a MIPS book as part of the main documentation body.
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
> ---
> Documentation/index.rst | 1 +
> .../{AU1xxx_IDE.README => au1xxx_ide.rst} | 89 +++++++++++--------
> Documentation/mips/index.rst | 17 ++++
> 3 files changed, 70 insertions(+), 37 deletions(-)
> rename Documentation/mips/{AU1xxx_IDE.README => au1xxx_ide.rst} (67%)
> create mode 100644 Documentation/mips/index.rst
Acked-by: Paul Burton <paul.burton@mips.com>
Thanks,
Paul
^ permalink raw reply
* Re: [RFC PATCH] string.h: Add stracpy/stracpy_pad (was: Re: [PATCH] checkpatch: Added warnings in favor of strscpy().)
From: Joe Perches @ 2019-07-22 17:43 UTC (permalink / raw)
To: Kees Cook
Cc: Nitin Gote, akpm, corbet, apw, linux-doc, linux-kernel,
kernel-hardening, Rasmus Villemoes
In-Reply-To: <201907221031.8B87A9DE@keescook>
On Mon, 2019-07-22 at 10:33 -0700, Kees Cook wrote:
> On Thu, Jul 04, 2019 at 05:15:57PM -0700, Joe Perches wrote:
> > On Thu, 2019-07-04 at 13:46 -0700, Joe Perches wrote:
> > > On Thu, 2019-07-04 at 11:24 +0530, Nitin Gote wrote:
> > > > Added warnings in checkpatch.pl script to :
> > > >
> > > > 1. Deprecate strcpy() in favor of strscpy().
> > > > 2. Deprecate strlcpy() in favor of strscpy().
> > > > 3. Deprecate strncpy() in favor of strscpy() or strscpy_pad().
> > > >
> > > > Updated strncpy() section in Documentation/process/deprecated.rst
> > > > to cover strscpy_pad() case.
> >
> > []
> >
> > I sent a patch series for some strscpy/strlcpy misuses.
> >
> > How about adding a macro helper to avoid the misuses like:
> > ---
> > include/linux/string.h | 16 ++++++++++++++++
> > 1 file changed, 16 insertions(+)
> >
> > diff --git a/include/linux/string.h b/include/linux/string.h
> > index 4deb11f7976b..ef01bd6f19df 100644
> > --- a/include/linux/string.h
> > +++ b/include/linux/string.h
> > @@ -35,6 +35,22 @@ ssize_t strscpy(char *, const char *, size_t);
> > /* Wraps calls to strscpy()/memset(), no arch specific code required */
> > ssize_t strscpy_pad(char *dest, const char *src, size_t count);
> >
> > +#define stracpy(to, from) \
> > +({ \
> > + size_t size = ARRAY_SIZE(to); \
> > + BUILD_BUG_ON(!__same_type(typeof(*to), char)); \
> > + \
> > + strscpy(to, from, size); \
> > +})
> > +
> > +#define stracpy_pad(to, from) \
> > +({ \
> > + size_t size = ARRAY_SIZE(to); \
> > + BUILD_BUG_ON(!__same_type(typeof(*to), char)); \
> > + \
> > + strscpy_pad(to, from, size); \
> > +})
> > +
> > #ifndef __HAVE_ARCH_STRCAT
> > extern char * strcat(char *, const char *);
> > #endif
>
> This seems like a reasonable addition, yes. I think Coccinelle might
> actually be able to find all the existing strscpy(dst, src, sizeof(dst))
> cases to jump-start this conversion.
I did that. It works. It's a lot of conversions.
$ cat str.cpy.cocci
@@
expression e1;
expression e2;
@@
- strscpy(e1, e2, sizeof(e1))
+ stracpy(e1, e2)
@@
expression e1;
expression e2;
@@
- strlcpy(e1, e2, sizeof(e1))
+ stracpy(e1, e2)
> Devil's advocate: this adds yet more string handling functions... will
> this cause even more confusion?
Documentation is good.
Actual in-kernel use and examples better.
^ permalink raw reply
* Re: [PATCH v5] Documentation/checkpatch: Prefer strscpy/strscpy_pad over strcpy/strlcpy/strncpy
From: Joe Perches @ 2019-07-22 17:40 UTC (permalink / raw)
To: Kees Cook, NitinGote; +Cc: corbet, akpm, apw, linux-doc, kernel-hardening
In-Reply-To: <201907221029.B0CBED4F@keescook>
On Mon, 2019-07-22 at 10:30 -0700, Kees Cook wrote:
> On Wed, Jul 17, 2019 at 10:00:05AM +0530, NitinGote wrote:
> > From: Nitin Gote <nitin.r.gote@intel.com>
> >
> > Added check in checkpatch.pl to
> > 1. Deprecate strcpy() in favor of strscpy().
> > 2. Deprecate strlcpy() in favor of strscpy().
> > 3. Deprecate strncpy() in favor of strscpy() or strscpy_pad().
> >
> > Updated strncpy() section in Documentation/process/deprecated.rst
> > to cover strscpy_pad() case.
> >
> > Signed-off-by: Nitin Gote <nitin.r.gote@intel.com>
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
>
> Joe, does this address your checkpatch concerns?
Well, kinda.
strscpy_pad isn't used anywhere in the kernel.
And
+ "strncpy" => "strscpy, strscpy_pad or for non-NUL-terminated strings, strncpy() can still be used, but destinations should be marked with __nonstring",
is a bit verbose. This could be simply:
+ "strncpy" => "strscpy - for non-NUL-terminated uses, strncpy() dst should be __nonstring",
And I still prefer adding stracpy as it
reduces code verbosity and eliminates defects.
^ permalink raw reply
* Re: [RFC PATCH] string.h: Add stracpy/stracpy_pad (was: Re: [PATCH] checkpatch: Added warnings in favor of strscpy().)
From: Kees Cook @ 2019-07-22 17:33 UTC (permalink / raw)
To: Joe Perches
Cc: Nitin Gote, akpm, corbet, apw, linux-doc, linux-kernel,
kernel-hardening, Rasmus Villemoes
In-Reply-To: <d1524130f91d7cfd61bc736623409693d2895f57.camel@perches.com>
On Thu, Jul 04, 2019 at 05:15:57PM -0700, Joe Perches wrote:
> On Thu, 2019-07-04 at 13:46 -0700, Joe Perches wrote:
> > On Thu, 2019-07-04 at 11:24 +0530, Nitin Gote wrote:
> > > Added warnings in checkpatch.pl script to :
> > >
> > > 1. Deprecate strcpy() in favor of strscpy().
> > > 2. Deprecate strlcpy() in favor of strscpy().
> > > 3. Deprecate strncpy() in favor of strscpy() or strscpy_pad().
> > >
> > > Updated strncpy() section in Documentation/process/deprecated.rst
> > > to cover strscpy_pad() case.
>
> []
>
> I sent a patch series for some strscpy/strlcpy misuses.
>
> How about adding a macro helper to avoid the misuses like:
> ---
> include/linux/string.h | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/include/linux/string.h b/include/linux/string.h
> index 4deb11f7976b..ef01bd6f19df 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -35,6 +35,22 @@ ssize_t strscpy(char *, const char *, size_t);
> /* Wraps calls to strscpy()/memset(), no arch specific code required */
> ssize_t strscpy_pad(char *dest, const char *src, size_t count);
>
> +#define stracpy(to, from) \
> +({ \
> + size_t size = ARRAY_SIZE(to); \
> + BUILD_BUG_ON(!__same_type(typeof(*to), char)); \
> + \
> + strscpy(to, from, size); \
> +})
> +
> +#define stracpy_pad(to, from) \
> +({ \
> + size_t size = ARRAY_SIZE(to); \
> + BUILD_BUG_ON(!__same_type(typeof(*to), char)); \
> + \
> + strscpy_pad(to, from, size); \
> +})
> +
> #ifndef __HAVE_ARCH_STRCAT
> extern char * strcat(char *, const char *);
> #endif
This seems like a reasonable addition, yes. I think Coccinelle might
actually be able to find all the existing strscpy(dst, src, sizeof(dst))
cases to jump-start this conversion.
Devil's advocate: this adds yet more string handling functions... will
this cause even more confusion?
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v5] Documentation/checkpatch: Prefer strscpy/strscpy_pad over strcpy/strlcpy/strncpy
From: Kees Cook @ 2019-07-22 17:30 UTC (permalink / raw)
To: NitinGote; +Cc: joe, corbet, akpm, apw, linux-doc, kernel-hardening
In-Reply-To: <20190717043005.19627-1-nitin.r.gote@intel.com>
On Wed, Jul 17, 2019 at 10:00:05AM +0530, NitinGote wrote:
> From: Nitin Gote <nitin.r.gote@intel.com>
>
> Added check in checkpatch.pl to
> 1. Deprecate strcpy() in favor of strscpy().
> 2. Deprecate strlcpy() in favor of strscpy().
> 3. Deprecate strncpy() in favor of strscpy() or strscpy_pad().
>
> Updated strncpy() section in Documentation/process/deprecated.rst
> to cover strscpy_pad() case.
>
> Signed-off-by: Nitin Gote <nitin.r.gote@intel.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
Joe, does this address your checkpatch concerns?
-Kees
> ---
> Documentation/process/deprecated.rst | 6 +++---
> scripts/checkpatch.pl | 24 ++++++++++++++++++++++++
> 2 files changed, 27 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/process/deprecated.rst b/Documentation/process/deprecated.rst
> index 49e0f64a3427..c348ef9d44f5 100644
> --- a/Documentation/process/deprecated.rst
> +++ b/Documentation/process/deprecated.rst
> @@ -93,9 +93,9 @@ will be NUL terminated. This can lead to various linear read overflows
> and other misbehavior due to the missing termination. It also NUL-pads the
> destination buffer if the source contents are shorter than the destination
> buffer size, which may be a needless performance penalty for callers using
> -only NUL-terminated strings. The safe replacement is :c:func:`strscpy`.
> -(Users of :c:func:`strscpy` still needing NUL-padding will need an
> -explicit :c:func:`memset` added.)
> +only NUL-terminated strings. In this case, the safe replacement is
> +strscpy(). If, however, the destination buffer still needs NUL-padding,
> +the safe replacement is strscpy_pad().
>
> If a caller is using non-NUL-terminated strings, :c:func:`strncpy()` can
> still be used, but destinations should be marked with the `__nonstring
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index bb28b178d929..1bb12127115d 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -605,6 +605,20 @@ foreach my $entry (keys %deprecated_apis) {
> }
> $deprecated_apis_search = "(?:${deprecated_apis_search})";
>
> +our %deprecated_string_apis = (
> + "strcpy" => "strscpy",
> + "strlcpy" => "strscpy",
> + "strncpy" => "strscpy, strscpy_pad or for non-NUL-terminated strings, strncpy() can still be used, but destinations should be marked with __nonstring",
> +);
> +
> +#Create a search pattern for all these strings apis to speed up a loop below
> +our $deprecated_string_apis_search = "";
> +foreach my $entry (keys %deprecated_string_apis) {
> + $deprecated_string_apis_search .= '|' if ($deprecated_string_apis_search ne "");
> + $deprecated_string_apis_search .= $entry;
> +}
> +$deprecated_string_apis_search = "(?:${deprecated_string_apis_search})";
> +
> our $mode_perms_world_writable = qr{
> S_IWUGO |
> S_IWOTH |
> @@ -6446,6 +6460,16 @@ sub process {
> "Deprecated use of '$deprecated_api', prefer '$new_api' instead\n" . $herecurr);
> }
>
> +# check for string deprecated apis
> + if ($line =~ /\b($deprecated_string_apis_search)\b\s*\(/) {
> + my $deprecated_string_api = $1;
> + my $new_api = $deprecated_string_apis{$deprecated_string_api};
> + my $msg_level = \&WARN;
> + $msg_level = \&CHK if ($file);
> + &{$msg_level}("DEPRECATED_API",
> + "Deprecated use of '$deprecated_string_api', prefer '$new_api' instead\n" . $herecurr);
> + }
> +
> # check for various structs that are normally const (ops, kgdb, device_tree)
> # and avoid what seem like struct definitions 'struct foo {'
> if ($line !~ /\bconst\b/ &&
> --
> 2.17.1
>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH 04/22] docs: spi: convert to ReST and add it to the kABI bookset
From: Mauro Carvalho Chehab @ 2019-07-22 15:51 UTC (permalink / raw)
To: Mark Brown
Cc: Jonathan Corbet, Jonathan Cameron, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald-Stadler, linux-doc, linux-spi,
linux-iio, Jonathan Cameron
In-Reply-To: <20190722152110.GE4756@sirena.org.uk>
Em Mon, 22 Jul 2019 16:21:10 +0100
Mark Brown <broonie@kernel.org> escreveu:
> On Mon, Jul 22, 2019 at 10:10:35AM -0300, Mauro Carvalho Chehab wrote:
> > Mark Brown <broonie@kernel.org> escreveu:
>
> > > On Mon, Jul 22, 2019 at 08:07:31AM -0300, Mauro Carvalho Chehab wrote:
> > > > While there's one file there with briefily describes the uAPI,
> > > > the documentation was written just like most subsystems: focused
> > > > on kernel developers. So, add it together with driver-api books.
>
> > > Please use subject lines matching the style for the subsystem. This
> > > makes it easier for people to identify relevant patches.
>
> > Sure. Do you prefer this prefixed by:
>
> > spi: docs:
>
> > Or with something else?
>
> Anything starting with spi:
Ok.
>
> > > > Documentation/spi/{spidev => spidev.rst} | 30 +++--
> > >
> > > This is clearly a userspace focused document rather than a kernel
> > > internal one.
> >
> > True. What I've been doing so far is, for all drivers that I'm converting
> > with carries more than one documentation type (kABI, uABI and/or
> > admin-guide) is to keep the directory as-is, adding them under
> > this section at Documentation/index.rst:
...
> > Btw, if you look at spidev file, it contains stuff for both
> > userspace-api:
> >
> > "SPI devices have a limited userspace API, supporting basic half-duplex
> > read() and write() access to SPI slave devices. Using ioctl() requests,"
>
> > And for admin-guide:
>
> > "For a SPI device with chipselect C on bus B, you should see:
> >
> > /dev/spidevB.C ... character special device, major number 153 with
> > a dynamically chosen minor device number. "
>
> I think that split is higly artificial...
>
> > So, if we're willing to move it, the best is to do on a separate patch
> > with would split its contents into two files: admin-guide/spi-devices.rst and
> > userspace-api/spi-api.rst.
>
> ...
>
> > Ideally, we should split what's there at media/uapi into admin-guide
> > and userspace-api, but this would mean *a lot* of efforts. Not sure
> > if it is worth the effort.
>
> Is the admin/API stuff even sensible for things that are more embedded
> or desktop focused?
Yes. Btw, the plan is to add everything under Documentation/ABI at the
admin guide (parsed via some scripts).
> It feels very arbatrary and unhelpful for things
> like spidev where theuser is going to be writing a program.
I tend to agree with you. Doing such split may actually make things
worse for app developers, without providing much benefit for sysadmins.
I sent today an e-mail to the KS discussion ML about that, as, IMHO,
this is something that we should discuss at the Documentation track
there.
While the idea of having users/sysadmin-faced stuff at admin-guide
seems to be nice, doing it for driver-specific stuff could be overkill,
and will mean a lot of extra work.
Thanks,
Mauro
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox