* [PATCH v6 04/18] kunit: test: add kunit_stream a std::stream like logger
From: Brendan Higgins @ 2019-07-04 0:36 UTC (permalink / raw)
To: frowand.list, gregkh, jpoimboe, keescook, kieran.bingham, mcgrof,
peterz, robh, sboyd, shuah, tytso, yamada.masahiro
Cc: devicetree, dri-devel, kunit-dev, linux-doc, linux-fsdevel,
linux-kbuild, linux-kernel, linux-kselftest, linux-nvdimm,
linux-um, Alexander.Levin, Tim.Bird, amir73il, dan.carpenter,
daniel, jdike, joel, julia.lawall, khilman, knut.omang, logang,
mpe, pmladek, rdunlap, richard, rientjes, rostedt, wfg,
Brendan Higgins
In-Reply-To: <20190704003615.204860-1-brendanhiggins@google.com>
A lot of the expectation and assertion infrastructure prints out fairly
complicated test failure messages, so add a C++ style log library for
for logging test results.
Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
---
include/kunit/kunit-stream.h | 81 +++++++++++++++++++++++
include/kunit/test.h | 3 +
kunit/Makefile | 3 +-
kunit/kunit-stream.c | 123 +++++++++++++++++++++++++++++++++++
kunit/test.c | 6 ++
5 files changed, 215 insertions(+), 1 deletion(-)
create mode 100644 include/kunit/kunit-stream.h
create mode 100644 kunit/kunit-stream.c
diff --git a/include/kunit/kunit-stream.h b/include/kunit/kunit-stream.h
new file mode 100644
index 0000000000000..a7b53eabf6be4
--- /dev/null
+++ b/include/kunit/kunit-stream.h
@@ -0,0 +1,81 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * C++ stream style string formatter and printer used in KUnit for outputting
+ * KUnit messages.
+ *
+ * Copyright (C) 2019, Google LLC.
+ * Author: Brendan Higgins <brendanhiggins@google.com>
+ */
+
+#ifndef _KUNIT_KUNIT_STREAM_H
+#define _KUNIT_KUNIT_STREAM_H
+
+#include <linux/types.h>
+#include <kunit/string-stream.h>
+
+struct kunit;
+
+/**
+ * struct kunit_stream - a std::stream style string builder.
+ *
+ * A std::stream style string builder. Allows messages to be built up and
+ * printed all at once.
+ */
+struct kunit_stream {
+ /* private: internal use only. */
+ struct kunit *test;
+ const char *level;
+ struct string_stream *internal_stream;
+};
+
+/**
+ * alloc_kunit_stream() - constructs a new &struct kunit_stream.
+ * @test: The test context object.
+ * @level: The log level at which to print out the message.
+ *
+ * Constructs a new test managed &struct kunit_stream.
+ */
+struct kunit_stream *alloc_kunit_stream(struct kunit *test, const char *level);
+
+/**
+ * kunit_stream_add(): adds the formatted input to the internal buffer.
+ * @kstream: the stream being operated on.
+ * @fmt: printf style format string to append to stream.
+ *
+ * Appends the formatted string, @fmt, to the internal buffer.
+ */
+void __printf(2, 3) kunit_stream_add(struct kunit_stream *kstream,
+ const char *fmt, ...);
+
+/**
+ * kunit_stream_append(): appends the contents of @other to @kstream.
+ * @kstream: the stream to which @other is appended.
+ * @other: the stream whose contents are appended to @kstream.
+ *
+ * Appends the contents of @other to @kstream.
+ */
+void kunit_stream_append(struct kunit_stream *kstream,
+ struct kunit_stream *other);
+
+/**
+ * kunit_stream_commit(): prints out the internal buffer to the user.
+ * @kstream: the stream being operated on.
+ *
+ * Outputs the contents of the internal buffer as a kunit_printk formatted
+ * output. KUNIT_STREAM ONLY OUTPUTS ITS BUFFER TO THE USER IF COMMIT IS
+ * CALLED!!! The reason for this is that it allows us to construct a message
+ * before we know whether we want to print it out; this can be extremely handy
+ * if there is information you might need for a failure message that is easiest
+ * to collect in the steps leading up to the actual check.
+ */
+void kunit_stream_commit(struct kunit_stream *kstream);
+
+/**
+ * kunit_stream_clear(): clears the internal buffer.
+ * @kstream: the stream being operated on.
+ *
+ * Clears the contents of the internal buffer.
+ */
+void kunit_stream_clear(struct kunit_stream *kstream);
+
+#endif /* _KUNIT_KUNIT_STREAM_H */
diff --git a/include/kunit/test.h b/include/kunit/test.h
index d9973ee5d7f82..a96a166c4808c 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -11,6 +11,7 @@
#include <linux/types.h>
#include <linux/slab.h>
+#include <kunit/kunit-stream.h>
struct kunit_resource;
@@ -184,6 +185,8 @@ struct kunit {
void kunit_init_test(struct kunit *test, const char *name);
+void kunit_fail(struct kunit *test, struct kunit_stream *stream);
+
int kunit_run_tests(struct kunit_suite *suite);
/**
diff --git a/kunit/Makefile b/kunit/Makefile
index 275b565a0e81f..6ddc622ee6b1c 100644
--- a/kunit/Makefile
+++ b/kunit/Makefile
@@ -1,2 +1,3 @@
obj-$(CONFIG_KUNIT) += test.o \
- string-stream.o
+ string-stream.o \
+ kunit-stream.o
diff --git a/kunit/kunit-stream.c b/kunit/kunit-stream.c
new file mode 100644
index 0000000000000..8bea1f22eafb5
--- /dev/null
+++ b/kunit/kunit-stream.c
@@ -0,0 +1,123 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * C++ stream style string formatter and printer used in KUnit for outputting
+ * KUnit messages.
+ *
+ * Copyright (C) 2019, Google LLC.
+ * Author: Brendan Higgins <brendanhiggins@google.com>
+ */
+
+#include <kunit/test.h>
+#include <kunit/kunit-stream.h>
+#include <kunit/string-stream.h>
+
+void kunit_stream_add(struct kunit_stream *kstream, const char *fmt, ...)
+{
+ va_list args;
+ struct string_stream *stream = kstream->internal_stream;
+
+ va_start(args, fmt);
+
+ if (string_stream_vadd(stream, fmt, args) < 0)
+ kunit_err(kstream->test,
+ "Failed to allocate fragment: %s\n",
+ fmt);
+
+ va_end(args);
+}
+
+void kunit_stream_append(struct kunit_stream *kstream,
+ struct kunit_stream *other)
+{
+ struct string_stream *other_stream = other->internal_stream;
+ const char *other_content;
+
+ other_content = string_stream_get_string(other_stream);
+
+ if (!other_content) {
+ kunit_err(kstream->test,
+ "Failed to get string from second argument for appending\n");
+ return;
+ }
+
+ kunit_stream_add(kstream, other_content);
+}
+
+void kunit_stream_clear(struct kunit_stream *kstream)
+{
+ string_stream_clear(kstream->internal_stream);
+}
+
+void kunit_stream_commit(struct kunit_stream *kstream)
+{
+ struct string_stream *stream = kstream->internal_stream;
+ struct string_stream_fragment *fragment;
+ struct kunit *test = kstream->test;
+ char *buf;
+
+ buf = string_stream_get_string(stream);
+ if (!buf) {
+ kunit_err(test,
+ "Could not allocate buffer, dumping stream:\n");
+ list_for_each_entry(fragment, &stream->fragments, node) {
+ kunit_err(test, fragment->fragment);
+ }
+ kunit_err(test, "\n");
+ goto cleanup;
+ }
+
+ kunit_printk(kstream->level, test, buf);
+ kfree(buf);
+
+cleanup:
+ kunit_stream_clear(kstream);
+}
+
+static int kunit_stream_init(struct kunit_resource *res, void *context)
+{
+ struct kunit *test = context;
+ struct kunit_stream *stream;
+
+ stream = kzalloc(sizeof(*stream), GFP_KERNEL);
+ if (!stream)
+ return -ENOMEM;
+
+ res->allocation = stream;
+ stream->test = test;
+ stream->internal_stream = alloc_string_stream(test);
+
+ if (!stream->internal_stream)
+ return -ENOMEM;
+
+ return 0;
+}
+
+static void kunit_stream_free(struct kunit_resource *res)
+{
+ struct kunit_stream *stream = res->allocation;
+
+ if (!string_stream_is_empty(stream->internal_stream)) {
+ kunit_err(stream->test,
+ "End of test case reached with uncommitted stream entries\n");
+ kunit_stream_commit(stream);
+ }
+}
+
+struct kunit_stream *alloc_kunit_stream(struct kunit *test, const char *level)
+{
+ struct kunit_stream *kstream;
+ struct kunit_resource *res;
+
+ res = kunit_alloc_resource(test,
+ kunit_stream_init,
+ kunit_stream_free,
+ test);
+
+ if (!res)
+ return NULL;
+
+ kstream = res->allocation;
+ kstream->level = level;
+
+ return kstream;
+}
diff --git a/kunit/test.c b/kunit/test.c
index a70fbe449e922..c6a9e89ae3048 100644
--- a/kunit/test.c
+++ b/kunit/test.c
@@ -120,6 +120,12 @@ static void kunit_print_test_case_ok_not_ok(struct kunit_case *test_case,
test_case->name);
}
+void kunit_fail(struct kunit *test, struct kunit_stream *stream)
+{
+ kunit_set_failure(test);
+ kunit_stream_commit(stream);
+}
+
void kunit_init_test(struct kunit *test, const char *name)
{
mutex_init(&test->lock);
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCH v6 03/18] kunit: test: add string_stream a std::stream like string builder
From: Brendan Higgins @ 2019-07-04 0:36 UTC (permalink / raw)
To: frowand.list, gregkh, jpoimboe, keescook, kieran.bingham, mcgrof,
peterz, robh, sboyd, shuah, tytso, yamada.masahiro
Cc: devicetree, dri-devel, kunit-dev, linux-doc, linux-fsdevel,
linux-kbuild, linux-kernel, linux-kselftest, linux-nvdimm,
linux-um, Alexander.Levin, Tim.Bird, amir73il, dan.carpenter,
daniel, jdike, joel, julia.lawall, khilman, knut.omang, logang,
mpe, pmladek, rdunlap, richard, rientjes, rostedt, wfg,
Brendan Higgins
In-Reply-To: <20190704003615.204860-1-brendanhiggins@google.com>
A number of test features need to do pretty complicated string printing
where it may not be possible to rely on a single preallocated string
with parameters.
So provide a library for constructing the string as you go similar to
C++'s std::string.
Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
---
include/kunit/string-stream.h | 49 ++++++++++++
kunit/Makefile | 3 +-
kunit/string-stream.c | 147 ++++++++++++++++++++++++++++++++++
3 files changed, 198 insertions(+), 1 deletion(-)
create mode 100644 include/kunit/string-stream.h
create mode 100644 kunit/string-stream.c
diff --git a/include/kunit/string-stream.h b/include/kunit/string-stream.h
new file mode 100644
index 0000000000000..0552a05781afe
--- /dev/null
+++ b/include/kunit/string-stream.h
@@ -0,0 +1,49 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * C++ stream style string builder used in KUnit for building messages.
+ *
+ * Copyright (C) 2019, Google LLC.
+ * Author: Brendan Higgins <brendanhiggins@google.com>
+ */
+
+#ifndef _KUNIT_STRING_STREAM_H
+#define _KUNIT_STRING_STREAM_H
+
+#include <linux/types.h>
+#include <linux/spinlock.h>
+#include <linux/kref.h>
+#include <stdarg.h>
+
+struct string_stream_fragment {
+ struct list_head node;
+ char *fragment;
+};
+
+struct string_stream {
+ size_t length;
+ struct list_head fragments;
+ /* length and fragments are protected by this lock */
+ spinlock_t lock;
+};
+
+struct kunit;
+
+struct string_stream *alloc_string_stream(struct kunit *test);
+
+void string_stream_get(struct string_stream *stream);
+
+int string_stream_put(struct string_stream *stream);
+
+int string_stream_add(struct string_stream *stream, const char *fmt, ...);
+
+int string_stream_vadd(struct string_stream *stream,
+ const char *fmt,
+ va_list args);
+
+char *string_stream_get_string(struct string_stream *stream);
+
+void string_stream_clear(struct string_stream *stream);
+
+bool string_stream_is_empty(struct string_stream *stream);
+
+#endif /* _KUNIT_STRING_STREAM_H */
diff --git a/kunit/Makefile b/kunit/Makefile
index 5efdc4dea2c08..275b565a0e81f 100644
--- a/kunit/Makefile
+++ b/kunit/Makefile
@@ -1 +1,2 @@
-obj-$(CONFIG_KUNIT) += test.o
+obj-$(CONFIG_KUNIT) += test.o \
+ string-stream.o
diff --git a/kunit/string-stream.c b/kunit/string-stream.c
new file mode 100644
index 0000000000000..0463a92dad74b
--- /dev/null
+++ b/kunit/string-stream.c
@@ -0,0 +1,147 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * C++ stream style string builder used in KUnit for building messages.
+ *
+ * Copyright (C) 2019, Google LLC.
+ * Author: Brendan Higgins <brendanhiggins@google.com>
+ */
+
+#include <linux/list.h>
+#include <linux/slab.h>
+#include <kunit/string-stream.h>
+#include <kunit/test.h>
+
+int string_stream_vadd(struct string_stream *stream,
+ const char *fmt,
+ va_list args)
+{
+ struct string_stream_fragment *frag_container;
+ int len;
+ va_list args_for_counting;
+ unsigned long flags;
+
+ /* Make a copy because `vsnprintf` could change it */
+ va_copy(args_for_counting, args);
+
+ /* Need space for null byte. */
+ len = vsnprintf(NULL, 0, fmt, args_for_counting) + 1;
+
+ va_end(args_for_counting);
+
+ frag_container = kmalloc(sizeof(*frag_container), GFP_KERNEL);
+ if (!frag_container)
+ return -ENOMEM;
+
+ frag_container->fragment = kmalloc(len, GFP_KERNEL);
+ if (!frag_container->fragment) {
+ kfree(frag_container);
+ return -ENOMEM;
+ }
+
+ len = vsnprintf(frag_container->fragment, len, fmt, args);
+ spin_lock_irqsave(&stream->lock, flags);
+ stream->length += len;
+ list_add_tail(&frag_container->node, &stream->fragments);
+ spin_unlock_irqrestore(&stream->lock, flags);
+
+ return 0;
+}
+
+int string_stream_add(struct string_stream *stream, const char *fmt, ...)
+{
+ va_list args;
+ int result;
+
+ va_start(args, fmt);
+ result = string_stream_vadd(stream, fmt, args);
+ va_end(args);
+
+ return result;
+}
+
+void string_stream_clear(struct string_stream *stream)
+{
+ struct string_stream_fragment *frag_container, *frag_container_safe;
+ unsigned long flags;
+
+ spin_lock_irqsave(&stream->lock, flags);
+ list_for_each_entry_safe(frag_container,
+ frag_container_safe,
+ &stream->fragments,
+ node) {
+ list_del(&frag_container->node);
+ kfree(frag_container->fragment);
+ kfree(frag_container);
+ }
+ stream->length = 0;
+ spin_unlock_irqrestore(&stream->lock, flags);
+}
+
+char *string_stream_get_string(struct string_stream *stream)
+{
+ struct string_stream_fragment *frag_container;
+ size_t buf_len = stream->length + 1; /* +1 for null byte. */
+ char *buf;
+ unsigned long flags;
+
+ buf = kzalloc(buf_len, GFP_KERNEL);
+ if (!buf)
+ return NULL;
+
+ spin_lock_irqsave(&stream->lock, flags);
+ list_for_each_entry(frag_container, &stream->fragments, node)
+ strlcat(buf, frag_container->fragment, buf_len);
+ spin_unlock_irqrestore(&stream->lock, flags);
+
+ return buf;
+}
+
+bool string_stream_is_empty(struct string_stream *stream)
+{
+ bool is_empty;
+ unsigned long flags;
+
+ spin_lock_irqsave(&stream->lock, flags);
+ is_empty = list_empty(&stream->fragments);
+ spin_unlock_irqrestore(&stream->lock, flags);
+
+ return is_empty;
+}
+
+static int string_stream_init(struct kunit_resource *res, void *context)
+{
+ struct string_stream *stream;
+
+ stream = kzalloc(sizeof(*stream), GFP_KERNEL);
+ if (!stream)
+ return -ENOMEM;
+
+ res->allocation = stream;
+ INIT_LIST_HEAD(&stream->fragments);
+ spin_lock_init(&stream->lock);
+
+ return 0;
+}
+
+static void string_stream_free(struct kunit_resource *res)
+{
+ struct string_stream *stream = res->allocation;
+
+ string_stream_clear(stream);
+ kfree(stream);
+}
+
+struct string_stream *alloc_string_stream(struct kunit *test)
+{
+ struct kunit_resource *res;
+
+ res = kunit_alloc_resource(test,
+ string_stream_init,
+ string_stream_free,
+ NULL);
+
+ if (!res)
+ return NULL;
+
+ return res->allocation;
+}
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCH v6 02/18] kunit: test: add test resource management API
From: Brendan Higgins @ 2019-07-04 0:35 UTC (permalink / raw)
To: frowand.list, gregkh, jpoimboe, keescook, kieran.bingham, mcgrof,
peterz, robh, sboyd, shuah, tytso, yamada.masahiro
Cc: devicetree, dri-devel, kunit-dev, linux-doc, linux-fsdevel,
linux-kbuild, linux-kernel, linux-kselftest, linux-nvdimm,
linux-um, Alexander.Levin, Tim.Bird, amir73il, dan.carpenter,
daniel, jdike, joel, julia.lawall, khilman, knut.omang, logang,
mpe, pmladek, rdunlap, richard, rientjes, rostedt, wfg,
Brendan Higgins
In-Reply-To: <20190704003615.204860-1-brendanhiggins@google.com>
Create a common API for test managed resources like memory and test
objects. A lot of times a test will want to set up infrastructure to be
used in test cases; this could be anything from just wanting to allocate
some memory to setting up a driver stack; this defines facilities for
creating "test resources" which are managed by the test infrastructure
and are automatically cleaned up at the conclusion of the test.
Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
---
Changes Since Last Revision:
- Replaced spinlock with a mutex. - Suggested by Luis and Stephen
(Sorry guys, I still need some kind of locking for the resource list
and the death test field (introduced later) that gets written to and
read from multiple times).
---
include/kunit/test.h | 111 +++++++++++++++++++++++++++++++++++++++++++
kunit/test.c | 95 +++++++++++++++++++++++++++++++++++-
2 files changed, 205 insertions(+), 1 deletion(-)
diff --git a/include/kunit/test.h b/include/kunit/test.h
index b34d9f2ac6f9c..d9973ee5d7f82 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -10,6 +10,70 @@
#define _KUNIT_TEST_H
#include <linux/types.h>
+#include <linux/slab.h>
+
+struct kunit_resource;
+
+typedef int (*kunit_resource_init_t)(struct kunit_resource *, void *);
+typedef void (*kunit_resource_free_t)(struct kunit_resource *);
+
+/**
+ * struct kunit_resource - represents a *test managed resource*
+ * @allocation: for the user to store arbitrary data.
+ * @free: a user supplied function to free the resource. Populated by
+ * kunit_alloc_resource().
+ *
+ * Represents a *test managed resource*, a resource which will automatically be
+ * cleaned up at the end of a test case.
+ *
+ * Example:
+ *
+ * .. code-block:: c
+ *
+ * struct kunit_kmalloc_params {
+ * size_t size;
+ * gfp_t gfp;
+ * };
+ *
+ * static int kunit_kmalloc_init(struct kunit_resource *res, void *context)
+ * {
+ * struct kunit_kmalloc_params *params = context;
+ * res->allocation = kmalloc(params->size, params->gfp);
+ *
+ * if (!res->allocation)
+ * return -ENOMEM;
+ *
+ * return 0;
+ * }
+ *
+ * static void kunit_kmalloc_free(struct kunit_resource *res)
+ * {
+ * kfree(res->allocation);
+ * }
+ *
+ * void *kunit_kmalloc(struct kunit *test, size_t size, gfp_t gfp)
+ * {
+ * struct kunit_kmalloc_params params;
+ * struct kunit_resource *res;
+ *
+ * params.size = size;
+ * params.gfp = gfp;
+ *
+ * res = kunit_alloc_resource(test, kunit_kmalloc_init,
+ * kunit_kmalloc_free, ¶ms);
+ * if (res)
+ * return res->allocation;
+ *
+ * return NULL;
+ * }
+ */
+struct kunit_resource {
+ void *allocation;
+ kunit_resource_free_t free;
+
+ /* private: internal use only. */
+ struct list_head node;
+};
struct kunit;
@@ -114,6 +178,8 @@ struct kunit {
/* private: internal use only. */
const char *name; /* Read only after initialization! */
bool success; /* Read only after test_case finishes! */
+ struct mutex lock; /* Gaurds all mutable test state. */
+ struct list_head resources; /* Protected by lock. */
};
void kunit_init_test(struct kunit *test, const char *name);
@@ -144,6 +210,51 @@ int kunit_run_tests(struct kunit_suite *suite);
} \
late_initcall(kunit_suite_init##suite)
+/**
+ * kunit_alloc_resource() - Allocates a *test managed resource*.
+ * @test: The test context object.
+ * @init: a user supplied function to initialize the resource.
+ * @free: a user supplied function to free the resource.
+ * @context: for the user to pass in arbitrary data to the init function.
+ *
+ * Allocates a *test managed resource*, a resource which will automatically be
+ * cleaned up at the end of a test case. See &struct kunit_resource for an
+ * example.
+ */
+struct kunit_resource *kunit_alloc_resource(struct kunit *test,
+ kunit_resource_init_t init,
+ kunit_resource_free_t free,
+ void *context);
+
+void kunit_free_resource(struct kunit *test, struct kunit_resource *res);
+
+/**
+ * kunit_kmalloc() - Like kmalloc() except the allocation is *test managed*.
+ * @test: The test context object.
+ * @size: The size in bytes of the desired memory.
+ * @gfp: flags passed to underlying kmalloc().
+ *
+ * Just like `kmalloc(...)`, except the allocation is managed by the test case
+ * and is automatically cleaned up after the test case concludes. See &struct
+ * kunit_resource for more information.
+ */
+void *kunit_kmalloc(struct kunit *test, size_t size, gfp_t gfp);
+
+/**
+ * kunit_kzalloc() - Just like kunit_kmalloc(), but zeroes the allocation.
+ * @test: The test context object.
+ * @size: The size in bytes of the desired memory.
+ * @gfp: flags passed to underlying kmalloc().
+ *
+ * See kzalloc() and kunit_kmalloc() for more information.
+ */
+static inline void *kunit_kzalloc(struct kunit *test, size_t size, gfp_t gfp)
+{
+ return kunit_kmalloc(test, size, gfp | __GFP_ZERO);
+}
+
+void kunit_cleanup(struct kunit *test);
+
void __printf(3, 4) kunit_printk(const char *level,
const struct kunit *test,
const char *fmt, ...);
diff --git a/kunit/test.c b/kunit/test.c
index c030ba5a43e40..a70fbe449e922 100644
--- a/kunit/test.c
+++ b/kunit/test.c
@@ -122,7 +122,8 @@ static void kunit_print_test_case_ok_not_ok(struct kunit_case *test_case,
void kunit_init_test(struct kunit *test, const char *name)
{
- spin_lock_init(&test->lock);
+ mutex_init(&test->lock);
+ INIT_LIST_HEAD(&test->resources);
test->name = name;
test->success = true;
}
@@ -152,6 +153,8 @@ static void kunit_run_case(struct kunit_suite *suite,
if (suite->exit)
suite->exit(&test);
+ kunit_cleanup(&test);
+
test_case->success = test.success;
}
@@ -172,6 +175,96 @@ int kunit_run_tests(struct kunit_suite *suite)
return 0;
}
+struct kunit_resource *kunit_alloc_resource(struct kunit *test,
+ kunit_resource_init_t init,
+ kunit_resource_free_t free,
+ void *context)
+{
+ struct kunit_resource *res;
+ int ret;
+
+ res = kzalloc(sizeof(*res), GFP_KERNEL);
+ if (!res)
+ return NULL;
+
+ ret = init(res, context);
+ if (ret)
+ return NULL;
+
+ res->free = free;
+ mutex_lock(&test->lock);
+ list_add_tail(&res->node, &test->resources);
+ mutex_unlock(&test->lock);
+
+ return res;
+}
+
+void kunit_free_resource(struct kunit *test, struct kunit_resource *res)
+{
+ res->free(res);
+ list_del(&res->node);
+ kfree(res);
+}
+
+struct kunit_kmalloc_params {
+ size_t size;
+ gfp_t gfp;
+};
+
+static int kunit_kmalloc_init(struct kunit_resource *res, void *context)
+{
+ struct kunit_kmalloc_params *params = context;
+
+ res->allocation = kmalloc(params->size, params->gfp);
+ if (!res->allocation)
+ return -ENOMEM;
+
+ return 0;
+}
+
+static void kunit_kmalloc_free(struct kunit_resource *res)
+{
+ kfree(res->allocation);
+}
+
+void *kunit_kmalloc(struct kunit *test, size_t size, gfp_t gfp)
+{
+ struct kunit_kmalloc_params params;
+ struct kunit_resource *res;
+
+ params.size = size;
+ params.gfp = gfp;
+
+ res = kunit_alloc_resource(test,
+ kunit_kmalloc_init,
+ kunit_kmalloc_free,
+ ¶ms);
+
+ if (res)
+ return res->allocation;
+
+ return NULL;
+}
+
+void kunit_cleanup(struct kunit *test)
+{
+ struct kunit_resource *resource, *resource_safe;
+
+ mutex_lock(&test->lock);
+ /*
+ * test->resources is a stack - each allocation must be freed in the
+ * reverse order from which it was added since one resource may depend
+ * on another for its entire lifetime.
+ */
+ list_for_each_entry_safe_reverse(resource,
+ resource_safe,
+ &test->resources,
+ node) {
+ kunit_free_resource(test, resource);
+ }
+ mutex_unlock(&test->lock);
+}
+
void kunit_printk(const char *level,
const struct kunit *test,
const char *fmt, ...)
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCH v6 01/18] kunit: test: add KUnit test runner core
From: Brendan Higgins @ 2019-07-04 0:35 UTC (permalink / raw)
To: frowand.list, gregkh, jpoimboe, keescook, kieran.bingham, mcgrof,
peterz, robh, sboyd, shuah, tytso, yamada.masahiro
Cc: devicetree, dri-devel, kunit-dev, linux-doc, linux-fsdevel,
linux-kbuild, linux-kernel, linux-kselftest, linux-nvdimm,
linux-um, Alexander.Levin, Tim.Bird, amir73il, dan.carpenter,
daniel, jdike, joel, julia.lawall, khilman, knut.omang, logang,
mpe, pmladek, rdunlap, richard, rientjes, rostedt, wfg,
Brendan Higgins
In-Reply-To: <20190704003615.204860-1-brendanhiggins@google.com>
Add core facilities for defining unit tests; this provides a common way
to define test cases, functions that execute code which is under test
and determine whether the code under test behaves as expected; this also
provides a way to group together related test cases in test suites (here
we call them test_modules).
Just define test cases and how to execute them for now; setting
expectations on code will be defined later.
Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
---
Changes Since Last Revision:
- Removed spinlock. - Suggested by Luis and Stephen
- Renamed `struct kunit_module` to `struct kunit_suite`. - Inspired by
Luis.
- Made a number of minor cleanups. - Suggested by Luis and Stephen.
---
include/kunit/test.h | 182 +++++++++++++++++++++++++++++++++++++++++
kunit/Kconfig | 17 ++++
kunit/Makefile | 1 +
kunit/test.c | 190 +++++++++++++++++++++++++++++++++++++++++++
4 files changed, 390 insertions(+)
create mode 100644 include/kunit/test.h
create mode 100644 kunit/Kconfig
create mode 100644 kunit/Makefile
create mode 100644 kunit/test.c
diff --git a/include/kunit/test.h b/include/kunit/test.h
new file mode 100644
index 0000000000000..b34d9f2ac6f9c
--- /dev/null
+++ b/include/kunit/test.h
@@ -0,0 +1,182 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Base unit test (KUnit) API.
+ *
+ * Copyright (C) 2019, Google LLC.
+ * Author: Brendan Higgins <brendanhiggins@google.com>
+ */
+
+#ifndef _KUNIT_TEST_H
+#define _KUNIT_TEST_H
+
+#include <linux/types.h>
+
+struct kunit;
+
+/**
+ * struct kunit_case - represents an individual test case.
+ * @run_case: the function representing the actual test case.
+ * @name: the name of the test case.
+ *
+ * A test case is a function with the signature, ``void (*)(struct kunit *)``
+ * that makes expectations (see KUNIT_EXPECT_TRUE()) about code under test. Each
+ * test case is associated with a &struct kunit_suite and will be run after the
+ * suite's init function and followed by the suite's exit function.
+ *
+ * A test case should be static and should only be created with the KUNIT_CASE()
+ * macro; additionally, every array of test cases should be terminated with an
+ * empty test case.
+ *
+ * Example:
+ *
+ * .. code-block:: c
+ *
+ * void add_test_basic(struct kunit *test)
+ * {
+ * KUNIT_EXPECT_EQ(test, 1, add(1, 0));
+ * KUNIT_EXPECT_EQ(test, 2, add(1, 1));
+ * KUNIT_EXPECT_EQ(test, 0, add(-1, 1));
+ * KUNIT_EXPECT_EQ(test, INT_MAX, add(0, INT_MAX));
+ * KUNIT_EXPECT_EQ(test, -1, add(INT_MAX, INT_MIN));
+ * }
+ *
+ * static struct kunit_case example_test_cases[] = {
+ * KUNIT_CASE(add_test_basic),
+ * {}
+ * };
+ *
+ */
+struct kunit_case {
+ void (*run_case)(struct kunit *test);
+ const char *name;
+
+ /* private: internal use only. */
+ bool success;
+};
+
+/**
+ * KUNIT_CASE - A helper for creating a &struct kunit_case
+ * @test_name: a reference to a test case function.
+ *
+ * Takes a symbol for a function representing a test case and creates a
+ * &struct kunit_case object from it. See the documentation for
+ * &struct kunit_case for an example on how to use it.
+ */
+#define KUNIT_CASE(test_name) { .run_case = test_name, .name = #test_name }
+
+/**
+ * struct kunit_suite - describes a related collection of &struct kunit_case s.
+ * @name: the name of the test. Purely informational.
+ * @init: called before every test case.
+ * @exit: called after every test case.
+ * @test_cases: a null terminated array of test cases.
+ *
+ * A kunit_suite is a collection of related &struct kunit_case s, such that
+ * @init is called before every test case and @exit is called after every test
+ * case, similar to the notion of a *test fixture* or a *test class* in other
+ * unit testing frameworks like JUnit or Googletest.
+ *
+ * Every &struct kunit_case must be associated with a kunit_suite for KUnit to
+ * run it.
+ */
+struct kunit_suite {
+ const char name[256];
+ int (*init)(struct kunit *test);
+ void (*exit)(struct kunit *test);
+ struct kunit_case *test_cases;
+};
+
+/**
+ * struct kunit - represents a running instance of a test.
+ * @priv: for user to store arbitrary data. Commonly used to pass data created
+ * in the init function (see &struct kunit_suite).
+ *
+ * Used to store information about the current context under which the test is
+ * running. Most of this data is private and should only be accessed indirectly
+ * via public functions; the one exception is @priv which can be used by the
+ * test writer to store arbitrary data.
+ *
+ * A brief note on locking:
+ *
+ * First off, we need to lock because in certain cases a user may want to use an
+ * expectation in a thread other than the thread that the test case is running
+ * in.
+ *
+ * The next point is that the locking should be safe in an irq context.
+ * Technically speaking a user should use a mock/fake irqchip or some other fake
+ * method to trigger an irq handler under test; however, the irq handler should
+ * think that it is in a real irq context. Given that the code being run expects
+ * to be in an irq context, we should act as though it is.
+ */
+struct kunit {
+ void *priv;
+
+ /* private: internal use only. */
+ const char *name; /* Read only after initialization! */
+ bool success; /* Read only after test_case finishes! */
+};
+
+void kunit_init_test(struct kunit *test, const char *name);
+
+int kunit_run_tests(struct kunit_suite *suite);
+
+/**
+ * suite_test() - used to register a &struct kunit_suite with KUnit.
+ * @suite: a statically allocated &struct kunit_suite.
+ *
+ * Registers @suite with the test framework. See &struct kunit_suite for more
+ * information.
+ *
+ * NOTE: Currently KUnit tests are all run as late_initcalls; this means that
+ * they cannot test anything where tests must run at a different init phase. One
+ * significant restriction resulting from this is that KUnit cannot reliably
+ * test anything that is initialize in the late_init phase.
+ *
+ * TODO(brendanhiggins@google.com): Don't run all KUnit tests as late_initcalls.
+ * I have some future work planned to dispatch all KUnit tests from the same
+ * place, and at the very least to do so after everything else is definitely
+ * initialized.
+ */
+#define kunit_test_suite(suite) \
+ static int kunit_suite_init##suite(void) \
+ { \
+ return kunit_run_tests(&suite); \
+ } \
+ late_initcall(kunit_suite_init##suite)
+
+void __printf(3, 4) kunit_printk(const char *level,
+ const struct kunit *test,
+ const char *fmt, ...);
+
+/**
+ * kunit_info() - Prints an INFO level message associated with the current test.
+ * @test: The test context object.
+ * @fmt: A printk() style format string.
+ *
+ * Prints an info level message associated with the test suite being run. Takes
+ * a variable number of format parameters just like printk().
+ */
+#define kunit_info(test, fmt, ...) \
+ kunit_printk(KERN_INFO, test, fmt, ##__VA_ARGS__)
+
+/**
+ * kunit_warn() - Prints a WARN level message associated with the current test.
+ * @test: The test context object.
+ * @fmt: A printk() style format string.
+ *
+ * Prints a warning level message.
+ */
+#define kunit_warn(test, fmt, ...) \
+ kunit_printk(KERN_WARNING, test, fmt, ##__VA_ARGS__)
+
+/**
+ * kunit_err() - Prints an ERROR level message associated with the current test.
+ * @test: The test context object.
+ * @fmt: A printk() style format string.
+ *
+ * Prints an error level message.
+ */
+#define kunit_err(test, fmt, ...) \
+ kunit_printk(KERN_ERR, test, fmt, ##__VA_ARGS__)
+
+#endif /* _KUNIT_TEST_H */
diff --git a/kunit/Kconfig b/kunit/Kconfig
new file mode 100644
index 0000000000000..330ae83527c23
--- /dev/null
+++ b/kunit/Kconfig
@@ -0,0 +1,17 @@
+#
+# KUnit base configuration
+#
+
+menu "KUnit support"
+
+config KUNIT
+ bool "Enable support for unit tests (KUnit)"
+ help
+ Enables support for kernel unit tests (KUnit), a lightweight unit
+ testing and mocking framework for the Linux kernel. These tests are
+ able to be run locally on a developer's workstation without a VM or
+ special hardware when using UML. Can also be used on most other
+ architectures. For more information, please see
+ Documentation/dev-tools/kunit/.
+
+endmenu
diff --git a/kunit/Makefile b/kunit/Makefile
new file mode 100644
index 0000000000000..5efdc4dea2c08
--- /dev/null
+++ b/kunit/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_KUNIT) += test.o
diff --git a/kunit/test.c b/kunit/test.c
new file mode 100644
index 0000000000000..c030ba5a43e40
--- /dev/null
+++ b/kunit/test.c
@@ -0,0 +1,190 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Base unit test (KUnit) API.
+ *
+ * Copyright (C) 2019, Google LLC.
+ * Author: Brendan Higgins <brendanhiggins@google.com>
+ */
+
+#include <linux/sched/debug.h>
+#include <kunit/test.h>
+
+static void kunit_set_failure(struct kunit *test)
+{
+ WRITE_ONCE(test->success, false);
+}
+
+static int kunit_vprintk_emit(int level, const char *fmt, va_list args)
+{
+ return vprintk_emit(0, level, NULL, 0, fmt, args);
+}
+
+static int kunit_printk_emit(int level, const char *fmt, ...)
+{
+ va_list args;
+ int ret;
+
+ va_start(args, fmt);
+ ret = kunit_vprintk_emit(level, fmt, args);
+ va_end(args);
+
+ return ret;
+}
+
+static void kunit_vprintk(const struct kunit *test,
+ const char *level,
+ struct va_format *vaf)
+{
+ kunit_printk_emit(level[1] - '0', "\t# %s: %pV", test->name, vaf);
+}
+
+static void kunit_print_tap_version(void)
+{
+ static bool kunit_has_printed_tap_version;
+
+ if (!kunit_has_printed_tap_version) {
+ kunit_printk_emit(LOGLEVEL_INFO, "TAP version 14\n");
+ kunit_has_printed_tap_version = true;
+ }
+}
+
+static size_t kunit_test_cases_len(struct kunit_case *test_cases)
+{
+ struct kunit_case *test_case;
+ size_t len = 0;
+
+ for (test_case = test_cases; test_case->run_case; test_case++)
+ len++;
+
+ return len;
+}
+
+static void kunit_print_subtest_start(struct kunit_suite *suite)
+{
+ kunit_print_tap_version();
+ kunit_printk_emit(LOGLEVEL_INFO, "\t# Subtest: %s\n", suite->name);
+ kunit_printk_emit(LOGLEVEL_INFO,
+ "\t1..%zd\n",
+ kunit_test_cases_len(suite->test_cases));
+}
+
+static void kunit_print_ok_not_ok(bool should_indent,
+ bool is_ok,
+ size_t test_number,
+ const char *description)
+{
+ const char *indent, *ok_not_ok;
+
+ if (should_indent)
+ indent = "\t";
+ else
+ indent = "";
+
+ if (is_ok)
+ ok_not_ok = "ok";
+ else
+ ok_not_ok = "not ok";
+
+ kunit_printk_emit(LOGLEVEL_INFO,
+ "%s%s %zd - %s\n",
+ indent, ok_not_ok, test_number, description);
+}
+
+static bool kunit_suite_has_succeeded(struct kunit_suite *suite)
+{
+ const struct kunit_case *test_case;
+
+ for (test_case = suite->test_cases; test_case->run_case; test_case++)
+ if (!test_case->success)
+ return false;
+
+ return true;
+}
+
+static void kunit_print_subtest_end(struct kunit_suite *suite)
+{
+ static size_t kunit_suite_counter = 1;
+
+ kunit_print_ok_not_ok(false,
+ kunit_suite_has_succeeded(suite),
+ kunit_suite_counter++,
+ suite->name);
+}
+
+static void kunit_print_test_case_ok_not_ok(struct kunit_case *test_case,
+ size_t test_number)
+{
+ kunit_print_ok_not_ok(true,
+ test_case->success,
+ test_number,
+ test_case->name);
+}
+
+void kunit_init_test(struct kunit *test, const char *name)
+{
+ spin_lock_init(&test->lock);
+ test->name = name;
+ test->success = true;
+}
+
+/*
+ * Performs all logic to run a test case.
+ */
+static void kunit_run_case(struct kunit_suite *suite,
+ struct kunit_case *test_case)
+{
+ struct kunit test;
+ int ret = 0;
+
+ kunit_init_test(&test, test_case->name);
+
+ if (suite->init) {
+ ret = suite->init(&test);
+ if (ret) {
+ kunit_err(&test, "failed to initialize: %d\n", ret);
+ kunit_set_failure(&test);
+ return;
+ }
+ }
+
+ test_case->run_case(&test);
+
+ if (suite->exit)
+ suite->exit(&test);
+
+ test_case->success = test.success;
+}
+
+int kunit_run_tests(struct kunit_suite *suite)
+{
+ struct kunit_case *test_case;
+ size_t test_case_count = 1;
+
+ kunit_print_subtest_start(suite);
+
+ for (test_case = suite->test_cases; test_case->run_case; test_case++) {
+ kunit_run_case(suite, test_case);
+ kunit_print_test_case_ok_not_ok(test_case, test_case_count++);
+ }
+
+ kunit_print_subtest_end(suite);
+
+ return 0;
+}
+
+void kunit_printk(const char *level,
+ const struct kunit *test,
+ const char *fmt, ...)
+{
+ struct va_format vaf;
+ va_list args;
+
+ va_start(args, fmt);
+
+ vaf.fmt = fmt;
+ vaf.va = &args;
+
+ kunit_vprintk(test, level, &vaf);
+
+ va_end(args);
+}
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCH v6 00/18] kunit: introduce KUnit, the Linux kernel unit testing framework
From: Brendan Higgins @ 2019-07-04 0:35 UTC (permalink / raw)
To: frowand.list, gregkh, jpoimboe, keescook, kieran.bingham, mcgrof,
peterz, robh, sboyd, shuah, tytso, yamada.masahiro
Cc: devicetree, dri-devel, kunit-dev, linux-doc, linux-fsdevel,
linux-kbuild, linux-kernel, linux-kselftest, linux-nvdimm,
linux-um, Alexander.Levin, Tim.Bird, amir73il, dan.carpenter,
daniel, jdike, joel, julia.lawall, khilman, knut.omang, logang,
mpe, pmladek, rdunlap, richard, rientjes, rostedt, wfg,
Brendan Higgins
## TL;DR
This is a pretty straightforward follow-up to Stephen and Luis' comments
on PATCH v5: There is nothing that really changes any functionality or
usage with the minor exception that I renamed `struct kunit_module` to
`struct kunit_suite`. Nevertheless, a good deal of clean ups and fixes.
See "Changes Since Last Version" section below.
As for our current status, right now we got Reviewed-bys on all patches
except:
- [PATCH v6 08/18] objtool: add kunit_try_catch_throw to the noreturn
list
However, it would probably be good to get reviews/acks from the
subsystem maintainers on:
- [PATCH v6 06/18] kbuild: enable building KUnit
- [PATCH v6 08/18] objtool: add kunit_try_catch_throw to the noreturn
list
- [PATCH v6 15/18] Documentation: kunit: add documentation for KUnit
- [PATCH v6 17/18] kernel/sysctl-test: Add null pointer test for
sysctl.c:proc_dointvec()
Other than that, I think we should be good to go.
## Background
This patch set proposes KUnit, a lightweight unit testing and mocking
framework for the Linux kernel.
Unlike Autotest and kselftest, KUnit is a true unit testing framework;
it does not require installing the kernel on a test machine or in a VM
(however, KUnit still allows you to run tests on test machines or in VMs
if you want[1]) and does not require tests to be written in userspace
running on a host kernel. Additionally, KUnit is fast: From invocation
to completion KUnit can run several dozen tests in about a second.
Currently, the entire KUnit test suite for KUnit runs in under a second
from the initial invocation (build time excluded).
KUnit is heavily inspired by JUnit, Python's unittest.mock, and
Googletest/Googlemock for C++. KUnit provides facilities for defining
unit test cases, grouping related test cases into test suites, providing
common infrastructure for running tests, mocking, spying, and much more.
### What's so special about unit testing?
A unit test is supposed to test a single unit of code in isolation,
hence the name. There should be no dependencies outside the control of
the test; this means no external dependencies, which makes tests orders
of magnitudes faster. Likewise, since there are no external dependencies,
there are no hoops to jump through to run the tests. Additionally, this
makes unit tests deterministic: a failing unit test always indicates a
problem. Finally, because unit tests necessarily have finer granularity,
they are able to test all code paths easily solving the classic problem
of difficulty in exercising error handling code.
### Is KUnit trying to replace other testing frameworks for the kernel?
No. Most existing tests for the Linux kernel are end-to-end tests, which
have their place. A well tested system has lots of unit tests, a
reasonable number of integration tests, and some end-to-end tests. KUnit
is just trying to address the unit test space which is currently not
being addressed.
### More information on KUnit
There is a bunch of documentation near the end of this patch set that
describes how to use KUnit and best practices for writing unit tests.
For convenience I am hosting the compiled docs here[2].
Additionally for convenience, I have applied these patches to a
branch[3]. The repo may be cloned with:
git clone https://kunit.googlesource.com/linux
This patchset is on the kunit/rfc/v5.2-rc7/v6 branch.
## Changes Since Last Version
Aside from renaming `struct kunit_module` to `struct kunit_suite`, there
isn't really anything in here that changes any functionality:
- Rebased on v5.2-rc7
- Got rid of spinlocks.
- Now update success field using WRITE_ONCE. - Suggested by Stephen
- Other instances replaced by mutex. - Suggested by Stephen and Luis
- Renamed `struct kunit_module` to `struct kunit_suite`. - Inspired by
Luis.
- Fixed a broken example of how to use kunit_tool. - Pointed out by Ted
- Added descriptions to unit tests. - Suggested by Luis
- Labeled unit tests which tested the API. - Suggested by Luis
- Made a number of minor cleanups. - Suggested by Luis and Stephen.
[1] https://google.github.io/kunit-docs/third_party/kernel/docs/usage.html#kunit-on-non-uml-architectures
[2] https://google.github.io/kunit-docs/third_party/kernel/docs/
[3] https://kunit.googlesource.com/linux/+/kunit/rfc/v5.2-rc7/v6
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply
* Re: [PATCH 04/15] fpga: dfl: fme: support 512bit data width PR
From: Wu Hao @ 2019-07-03 23:44 UTC (permalink / raw)
To: Greg KH
Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-doc,
Ananda Ravuri, Xu Yilun, Alan Tull
In-Reply-To: <20190703175601.GA14034@kroah.com>
On Wed, Jul 03, 2019 at 07:56:01PM +0200, Greg KH wrote:
> On Thu, Jun 27, 2019 at 05:49:40PM -0700, Moritz Fischer wrote:
> > From: Wu Hao <hao.wu@intel.com>
> >
> > In early partial reconfiguration private feature, it only
> > supports 32bit data width when writing data to hardware for
> > PR. 512bit data width PR support is an important optimization
> > for some specific solutions (e.g. XEON with FPGA integrated),
> > it allows driver to use AVX512 instruction to improve the
> > performance of partial reconfiguration. e.g. programming one
> > 100MB bitstream image via this 512bit data width PR hardware
> > only takes ~300ms, but 32bit revision requires ~3s per test
> > result.
> >
> > Please note now this optimization is only done on revision 2
> > of this PR private feature which is only used in integrated
> > solution that AVX512 is always supported. This revision 2
> > hardware doesn't support 32bit PR.
> >
> > Signed-off-by: Ananda Ravuri <ananda.ravuri@intel.com>
> > Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > Signed-off-by: Wu Hao <hao.wu@intel.com>
> > Acked-by: Alan Tull <atull@kernel.org>
> > Signed-off-by: Moritz Fischer <mdf@kernel.org>
> > ---
> > drivers/fpga/dfl-fme-main.c | 3 +
> > drivers/fpga/dfl-fme-mgr.c | 113 +++++++++++++++++++++++++++++++-----
> > drivers/fpga/dfl-fme-pr.c | 43 +++++++++-----
> > drivers/fpga/dfl-fme.h | 2 +
> > drivers/fpga/dfl.h | 5 ++
> > 5 files changed, 135 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/fpga/dfl-fme-main.c b/drivers/fpga/dfl-fme-main.c
> > index 086ad2420ade..076d74f6416d 100644
> > --- a/drivers/fpga/dfl-fme-main.c
> > +++ b/drivers/fpga/dfl-fme-main.c
> > @@ -21,6 +21,8 @@
> > #include "dfl.h"
> > #include "dfl-fme.h"
> >
> > +#define DRV_VERSION "0.8"
> > +
> > static ssize_t ports_num_show(struct device *dev,
> > struct device_attribute *attr, char *buf)
> > {
> > @@ -277,3 +279,4 @@ MODULE_DESCRIPTION("FPGA Management Engine driver");
> > MODULE_AUTHOR("Intel Corporation");
> > MODULE_LICENSE("GPL v2");
> > MODULE_ALIAS("platform:dfl-fme");
> > +MODULE_VERSION(DRV_VERSION);
>
> No, we ripped out these useless "driver version" things all over the
> place, please do not add them back in again. They mean nothing and
> confuse people to no end.
>
> I'll not take this patch, sorry.
Let me remove them from these patches, and generate a new version quickly.
Thanks for the review and comments.
Hao
>
> greg k-h
^ permalink raw reply
* Re: [PATCH 05/15] Documentation: fpga: dfl: add descriptions for virtualization and new interfaces.
From: Wu Hao @ 2019-07-03 23:38 UTC (permalink / raw)
To: Greg KH
Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-doc, Xu Yilun,
Alan Tull
In-Reply-To: <20190703175926.GA14649@kroah.com>
On Wed, Jul 03, 2019 at 07:59:26PM +0200, Greg KH wrote:
> On Thu, Jun 27, 2019 at 05:49:41PM -0700, Moritz Fischer wrote:
> > From: Wu Hao <hao.wu@intel.com>
> >
> > This patch adds virtualization support description for DFL based
> > FPGA devices (based on PCIe SRIOV), and introductions to new
> > interfaces added by new dfl private feature drivers.
> >
> > [mdf@kernel.org: Fixed up to make it work with new reStructuredText docs]
> > Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > Signed-off-by: Wu Hao <hao.wu@intel.com>
> > Acked-by: Alan Tull <atull@kernel.org>
> > Signed-off-by: Moritz Fischer <mdf@kernel.org>
> > ---
> > Documentation/fpga/dfl.rst | 100 +++++++++++++++++++++++++++++++++++++
> > 1 file changed, 100 insertions(+)
>
> This doesn't apply to my tree, where is this file created?
Hi Greg,
From the cover-letter, Moritz mentioned, dfl.txt has been converted to .rst
in linux-next. I think this patch is created on top of that by Moritz.
"Note: I've seen that Mauro touched Documentation/fpga/dfl.rst in linux-next
commit c220a1fae6c5d ("docs: fpga: convert docs to ReST and rename to *.rst")"
Thanks
Hao
>
> thanks,
>
> greg k-h
^ permalink raw reply
* Re: [PATCH 06/15] fpga: dfl: fme: add DFL_FPGA_FME_PORT_RELEASE/ASSIGN ioctl support.
From: Wu Hao @ 2019-07-03 23:30 UTC (permalink / raw)
To: Greg KH
Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-doc, Zhang Yi Z,
Xu Yilun, Alan Tull
In-Reply-To: <20190703180753.GA24723@kroah.com>
On Wed, Jul 03, 2019 at 08:07:53PM +0200, Greg KH wrote:
> On Thu, Jun 27, 2019 at 05:49:42PM -0700, Moritz Fischer wrote:
> > From: Wu Hao <hao.wu@intel.com>
> >
> > In order to support virtualization usage via PCIe SRIOV, this patch
> > adds two ioctls under FPGA Management Engine (FME) to release and
> > assign back the port device. In order to safely turn Port from PF
> > into VF and enable PCIe SRIOV, it requires user to invoke this
> > PORT_RELEASE ioctl to release port firstly to remove userspace
> > interfaces, and then configure the PF/VF access register in FME.
> > After disable SRIOV, it requires user to invoke this PORT_ASSIGN
> > ioctl to attach the port back to PF.
> >
> > Ioctl interfaces:
> > * DFL_FPGA_FME_PORT_RELEASE
> > Release platform device of given port, it deletes port platform
> > device to remove related userspace interfaces on PF, then
> > configures PF/VF access mode to VF.
> >
> > * DFL_FPGA_FME_PORT_ASSIGN
> > Assign platform device of given port back to PF, it configures
> > PF/VF access mode to PF, then adds port platform device back to
> > re-enable related userspace interfaces on PF.
>
> Why are you not using the "generic" bind/unbind facility that userspace
> already has for this with binding drivers to devices? Why a special
> ioctl?
Hi Greg,
Actually we think it should be safer that making the device invisble than
just unbinding its driver. Looks like user can try to rebind it at any
time and we don't have any method to stop them.
>
> > --- a/include/uapi/linux/fpga-dfl.h
> > +++ b/include/uapi/linux/fpga-dfl.h
> > @@ -176,4 +176,36 @@ struct dfl_fpga_fme_port_pr {
> >
> > #define DFL_FPGA_FME_PORT_PR _IO(DFL_FPGA_MAGIC, DFL_FME_BASE + 0)
> >
> > +/**
> > + * DFL_FPGA_FME_PORT_RELEASE - _IOW(DFL_FPGA_MAGIC, DFL_FME_BASE + 1,
> > + * struct dfl_fpga_fme_port_release)
> > + *
> > + * Driver releases the port per Port ID provided by caller.
> > + * Return: 0 on success, -errno on failure.
> > + */
> > +struct dfl_fpga_fme_port_release {
> > + /* Input */
> > + __u32 argsz; /* Structure length */
> > + __u32 flags; /* Zero for now */
> > + __u32 port_id;
> > +};
>
> meta-comment, why do all of your structures for ioctls have argsz? You
> "know" the size of the structure already, it's part of the ioctl
> definition. You shouldn't need to also set it again, right? Otherwise
> ALL Linux ioctls would need something crazy like this.
Actually we followed the same method as vfio. The major purpose should be
extendibility, as we really need this to be sth long term maintainable.
It really helps, if we add some new members for extentions/enhancement
under the same ioctl. I don't think everybody needs this, but my
consideration here is if newer generations of hardware/specs come with some
extentions, I still hope we can resue these IOCTLs as much as we could,
instead of creating more new ones.
Thanks
Hao
>
> thanks,
>
> greg k-h
^ permalink raw reply
* Re: [PATCH v5 00/18] kunit: introduce KUnit, the Linux kernel unit testing framework
From: Brendan Higgins @ 2019-07-03 23:40 UTC (permalink / raw)
To: shuah
Cc: Theodore Ts'o, Frank Rowand, Greg KH, Josh Poimboeuf,
Kees Cook, Kieran Bingham, Luis Chamberlain, Peter Zijlstra,
Rob Herring, Stephen Boyd, 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: <CAFd5g46W1u+6JKLW0WX9uicK5utvJe9tvq4YBsCkghuo0rCmng@mail.gmail.com>
On Fri, Jun 21, 2019 at 5:54 PM Brendan Higgins
<brendanhiggins@google.com> wrote:
>
> On Fri, Jun 21, 2019 at 12:21 PM shuah <shuah@kernel.org> wrote:
> >
> > On 6/21/19 12:13 PM, Theodore Ts'o wrote:
> > > On Fri, Jun 21, 2019 at 08:59:48AM -0600, shuah wrote:
> > >>>> ### But wait! Doesn't kselftest support in kernel testing?!
> > >>>>
> > >>>> ....
> > >>
> > >> I think I commented on this before. I agree with the statement that
> > >> there is no overlap between Kselftest and KUnit. I would like see this
> > >> removed. Kselftest module support supports use-cases KUnit won't be able
> > >> to. I can build an kernel with Kselftest test modules and use it in the
> > >> filed to load and run tests if I need to debug a problem and get data
> > >> from a system. I can't do that with KUnit.
>
> Sure, I think this point has been brought up a number of times before.
> Maybe I didn't write this section well because, like Frank said, it
> comes across as being critical of the Kselftest module support; that
> wasn't my intention. I was speaking from the perspective that
> Kselftest module support is just a feature of Kselftest, and not a
> full framework like KUnit is (obviously Kselftest itself *is* a
> framework, but a very small part of it is not).
>
> It was obvious to me what Kselftest module support was intended for,
> and it is not intended to cover the use case that KUnit is targeting.
>
> > >> In my mind, I am not viewing this as which is better. Kselftest and
> > >> KUnit both have their place in the kernel development process. It isn't
> > >> productive and/or necessary to comparing Kselftest and KUnit without a
> > >> good understanding of the problem spaces for each of these.
>
> Again, I didn't mean to draw a comparison of which is better than the
> other. I was just trying to point out that Kselftest module support
> doesn't make sense as a stand alone unit testing framework, or really
> a framework of any kind, despite how it might actually be used.
>
> > >> I would strongly recommend not making reference to Kselftest and talk
> > >> about what KUnit offers.
>
> I can see your point. It seems that both you and Frank seem to think
> that I drew a comparison between Kselftest and KUnit, which was
> unintended. I probably should have spent more time editing this
> section, but I can see the point of drawing the comparison itself
> might invite this confusion.
>
> > > Shuah,
> > >
> > > Just to recall the history, this section of the FAQ was added to rebut
> > > the ***very*** strong statements that Frank made that there was
> > > overlap between Kselftest and Kunit, and that having too many ways for
> > > kernel developers to do the identical thing was harmful (he said it
> > > was too much of a burden on a kernel developer) --- and this was an
> > > argument for not including Kunit in the upstream kernel.
>
> I don't think he was actually advocating that we don't include KUnit,
> maybe playing devil's advocate; nevertheless, at the end, Frank seemed
> to agree that there were valuable things that KUnit offered. I thought
> he just wanted to make the point that I hadn't made the distinction
> sufficiently clear in the cover letter, and other reviewers might get
> confused in the future as well.
>
> Additionally, it does look like people were trying to use Kselftest
> module support to cover some things which really were trying to be
> unit tests. I know this isn't really intended - everything looks like
> a nail when you only have a hammer, which I think Frank was pointing
> out furthers the above confusion.
>
> In anycase, it sounds like I have, if anything, only made the
> discussion even more confusing by adding this section; sorry about
> that.
>
> > > If we're past that objection, then perhaps this section can be
> > > dropped, but there's a very good reason why it was there. I wouldn't
> > > Brendan to be accused of ignoring feedback from those who reviewed his
> > > patches. :-)
> > >
> >
> > Agreed. I understand that this FAQ probably was needed at one time and
> > Brendan added it to address the concerns.
>
> I don't want to speak for Frank, but I don't think it was an objection
> to KUnit itself, but rather an objection to not sufficiently
> addressing the point about how they differ.
>
> > I think at some point we do need to have a document that outlines when
> > to KUnit and when to use Kselftest modules. I think one concern people
> > have is that if KUnit is perceived as a replacement for Ksefltest
> > module, Kselftest module will be ignored leaving users without the
> > ability to build and run with Kselftest modules and load them on a need
> > basis to gather data on a systems that aren't dedicated strictly for
> > testing.
>
> I absolutely agree! I posed a suggestion here[1], which after I just
> now searched for a link, I realize for some reason it didn't seem like
> it reached a number of the mailing lists that I sent it to, so I
> should probably resend it.
>
> Anyway, a summary of what I suggested: We should start off by better
> organizing Documentation/dev-tools/ and create a landing page that
> groups the dev-tools by function according to what person is likely to
> use them and for what. Eventually and specifically for Kselftest and
> KUnit, I would like to have a testing guide for the kernel that
> explains what testing procedure should look like and what to use and
> when.
>
> > I am trying to move the conversation forward from KUnit vs. Kselftest
> > modules discussion to which problem areas each one addresses keeping
> > in mind that it is not about which is better. Kselftest and KUnit both
> > have their place in the kernel development process. We just have to be
> > clear on usage as we write tests for each.
>
> I think that is the right long term approach. I think a good place to
> start, like I suggested above, is cleaning up
> Documentation/dev-tools/, but I think that belongs in a (probably
> several) follow-up patchset.
>
> Frank, I believe your objection was mostly related to how KUnit is
> presented specifically in the cover letter, and doesn't necessarily
> deal with the intended use case. So I don't think that doing this,
> especially doing this later, really addresses your concern. I don't
> want to belabor the issue, but I would also rather not put words in
> your mouth, what are your thoughts on the above?
>
> I think my main concern moving forward on this point is that I am not
> sure that I can address the debate that this section covers in a way
> that is both sufficiently concise for a cover letter, but also doesn't
> invite more potential confusion. My inclination at this point is to
> drop it since I think the set of reviewers for this patchset has at
> this point become fixed, and it seems that it will likely cause more
> confusion rather than reduce it; also, I don't really think this will
Since no one has objected to dropping the "### But wait! Doesn't
kselftest support in kernel testing?!" section in the past almost two
weeks, I am just going to continue on without it.
Cheers
> be an issue for end users, especially once we have proper
> documentation in place. Alternatively, I guess I could maybe address
> the point elsewhere and refer to it in the cover letter? Or maybe just
> put it at the end of the cover letter?
>
> [1] https://www.mail-archive.com/kgdb-bugreport@lists.sourceforge.net/msg05059.html
^ permalink raw reply
* Re: [PATCH 35/39] docs: infiniband: add it to the driver-api bookset
From: Jason Gunthorpe @ 2019-07-03 18:08 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Linux Doc Mailing List, Mauro Carvalho Chehab, linux-kernel,
Jonathan Corbet, Doug Ledford, linux-rdma
In-Reply-To: <12743088687a9b0b305c05b62a5093056a4190b8.1561724493.git.mchehab+samsung@kernel.org>
On Fri, Jun 28, 2019 at 09:30:28AM -0300, Mauro Carvalho Chehab wrote:
> While this contains some uAPI stuff, it was intended to be
> read by a kernel doc. So, let's not move it to a different
> dir, but, instead, just add it to the driver-api bookset.
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
> Documentation/index.rst | 1 +
> Documentation/infiniband/index.rst | 2 +-
> 2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/index.rst b/Documentation/index.rst
> index ea33cbbccd9d..e69d2fde7735 100644
> +++ b/Documentation/index.rst
> @@ -96,6 +96,7 @@ needed).
> block/index
> hid/index
> iio/index
> + infiniband/index
> leds/index
> media/index
> networking/index
> diff --git a/Documentation/infiniband/index.rst b/Documentation/infiniband/index.rst
> index 22eea64de722..9cd7615438b9 100644
> +++ b/Documentation/infiniband/index.rst
> @@ -1,4 +1,4 @@
> -:orphan:
> +.. SPDX-License-Identifier: GPL-2.0
>
> ==========
> InfiniBand
Should this one go to the rdma.git as well? It looks like yes
Thanks,
Jason
^ permalink raw reply
* Re: [PATCH 06/15] fpga: dfl: fme: add DFL_FPGA_FME_PORT_RELEASE/ASSIGN ioctl support.
From: Greg KH @ 2019-07-03 18:07 UTC (permalink / raw)
To: Moritz Fischer
Cc: linux-fpga, linux-kernel, linux-doc, Wu Hao, Zhang Yi Z, Xu Yilun,
Alan Tull
In-Reply-To: <20190628004951.6202-7-mdf@kernel.org>
On Thu, Jun 27, 2019 at 05:49:42PM -0700, Moritz Fischer wrote:
> From: Wu Hao <hao.wu@intel.com>
>
> In order to support virtualization usage via PCIe SRIOV, this patch
> adds two ioctls under FPGA Management Engine (FME) to release and
> assign back the port device. In order to safely turn Port from PF
> into VF and enable PCIe SRIOV, it requires user to invoke this
> PORT_RELEASE ioctl to release port firstly to remove userspace
> interfaces, and then configure the PF/VF access register in FME.
> After disable SRIOV, it requires user to invoke this PORT_ASSIGN
> ioctl to attach the port back to PF.
>
> Ioctl interfaces:
> * DFL_FPGA_FME_PORT_RELEASE
> Release platform device of given port, it deletes port platform
> device to remove related userspace interfaces on PF, then
> configures PF/VF access mode to VF.
>
> * DFL_FPGA_FME_PORT_ASSIGN
> Assign platform device of given port back to PF, it configures
> PF/VF access mode to PF, then adds port platform device back to
> re-enable related userspace interfaces on PF.
Why are you not using the "generic" bind/unbind facility that userspace
already has for this with binding drivers to devices? Why a special
ioctl?
> --- a/include/uapi/linux/fpga-dfl.h
> +++ b/include/uapi/linux/fpga-dfl.h
> @@ -176,4 +176,36 @@ struct dfl_fpga_fme_port_pr {
>
> #define DFL_FPGA_FME_PORT_PR _IO(DFL_FPGA_MAGIC, DFL_FME_BASE + 0)
>
> +/**
> + * DFL_FPGA_FME_PORT_RELEASE - _IOW(DFL_FPGA_MAGIC, DFL_FME_BASE + 1,
> + * struct dfl_fpga_fme_port_release)
> + *
> + * Driver releases the port per Port ID provided by caller.
> + * Return: 0 on success, -errno on failure.
> + */
> +struct dfl_fpga_fme_port_release {
> + /* Input */
> + __u32 argsz; /* Structure length */
> + __u32 flags; /* Zero for now */
> + __u32 port_id;
> +};
meta-comment, why do all of your structures for ioctls have argsz? You
"know" the size of the structure already, it's part of the ioctl
definition. You shouldn't need to also set it again, right? Otherwise
ALL Linux ioctls would need something crazy like this.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 01/43] docs: infiniband: convert docs to ReST and rename to *.rst
From: Jason Gunthorpe @ 2019-07-03 18:06 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Linux Doc Mailing List, Mauro Carvalho Chehab, linux-kernel,
Jonathan Corbet, Doug Ledford, linux-rdma
In-Reply-To: <4d843d0361e245861f7051e2c736a18dfaae7601.1561723980.git.mchehab+samsung@kernel.org>
On Fri, Jun 28, 2019 at 09:19:57AM -0300, Mauro Carvalho Chehab wrote:
> The InfiniBand docs are plain text with no markups.
> So, all we needed to do were to add the title markups and
> some markup sequences in order to properly parse tables,
> lists and literal blocks.
>
> At its new index.rst, let's add a :orphan: while this is not linked to
> the main index.rst file, in order to avoid build warnings.
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
> ---
> .../{core_locking.txt => core_locking.rst} | 64 ++++++-----
> Documentation/infiniband/index.rst | 23 ++++
> .../infiniband/{ipoib.txt => ipoib.rst} | 24 ++--
> .../infiniband/{opa_vnic.txt => opa_vnic.rst} | 108 +++++++++---------
> .../infiniband/{sysfs.txt => sysfs.rst} | 4 +-
> .../{tag_matching.txt => tag_matching.rst} | 5 +
> .../infiniband/{user_mad.txt => user_mad.rst} | 33 ++++--
> .../{user_verbs.txt => user_verbs.rst} | 12 +-
> drivers/infiniband/core/user_mad.c | 2 +-
> drivers/infiniband/ulp/ipoib/Kconfig | 2 +-
> 10 files changed, 174 insertions(+), 103 deletions(-)
> rename Documentation/infiniband/{core_locking.txt => core_locking.rst} (78%)
> create mode 100644 Documentation/infiniband/index.rst
> rename Documentation/infiniband/{ipoib.txt => ipoib.rst} (90%)
> rename Documentation/infiniband/{opa_vnic.txt => opa_vnic.rst} (63%)
> rename Documentation/infiniband/{sysfs.txt => sysfs.rst} (69%)
> rename Documentation/infiniband/{tag_matching.txt => tag_matching.rst} (98%)
> rename Documentation/infiniband/{user_mad.txt => user_mad.rst} (90%)
> rename Documentation/infiniband/{user_verbs.txt => user_verbs.rst} (93%)
I'm not sure anymore if I sent a note or not, but this patch was
already applied to the rdma.git:
commit 97162a1ee8a1735fc7a7159fe08de966d88354ce
Author: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
Date: Sat Jun 8 23:27:03 2019 -0300
docs: infiniband: convert docs to ReST and rename to *.rst
The InfiniBand docs are plain text with no markups. So, all we needed to
do were to add the title markups and some markup sequences in order to
properly parse tables, lists and literal blocks.
At its new index.rst, let's add a :orphan: while this is not linked to the
main index.rst file, in order to avoid build warnings.
Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
Thanks,
Jason
^ permalink raw reply
* Re: [PATCH 05/15] Documentation: fpga: dfl: add descriptions for virtualization and new interfaces.
From: Greg KH @ 2019-07-03 17:59 UTC (permalink / raw)
To: Moritz Fischer
Cc: linux-fpga, linux-kernel, linux-doc, Wu Hao, Xu Yilun, Alan Tull
In-Reply-To: <20190628004951.6202-6-mdf@kernel.org>
On Thu, Jun 27, 2019 at 05:49:41PM -0700, Moritz Fischer wrote:
> From: Wu Hao <hao.wu@intel.com>
>
> This patch adds virtualization support description for DFL based
> FPGA devices (based on PCIe SRIOV), and introductions to new
> interfaces added by new dfl private feature drivers.
>
> [mdf@kernel.org: Fixed up to make it work with new reStructuredText docs]
> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> Signed-off-by: Wu Hao <hao.wu@intel.com>
> Acked-by: Alan Tull <atull@kernel.org>
> Signed-off-by: Moritz Fischer <mdf@kernel.org>
> ---
> Documentation/fpga/dfl.rst | 100 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 100 insertions(+)
This doesn't apply to my tree, where is this file created?
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 04/15] fpga: dfl: fme: support 512bit data width PR
From: Greg KH @ 2019-07-03 17:56 UTC (permalink / raw)
To: Moritz Fischer
Cc: linux-fpga, linux-kernel, linux-doc, Wu Hao, Ananda Ravuri,
Xu Yilun, Alan Tull
In-Reply-To: <20190628004951.6202-5-mdf@kernel.org>
On Thu, Jun 27, 2019 at 05:49:40PM -0700, Moritz Fischer wrote:
> From: Wu Hao <hao.wu@intel.com>
>
> In early partial reconfiguration private feature, it only
> supports 32bit data width when writing data to hardware for
> PR. 512bit data width PR support is an important optimization
> for some specific solutions (e.g. XEON with FPGA integrated),
> it allows driver to use AVX512 instruction to improve the
> performance of partial reconfiguration. e.g. programming one
> 100MB bitstream image via this 512bit data width PR hardware
> only takes ~300ms, but 32bit revision requires ~3s per test
> result.
>
> Please note now this optimization is only done on revision 2
> of this PR private feature which is only used in integrated
> solution that AVX512 is always supported. This revision 2
> hardware doesn't support 32bit PR.
>
> Signed-off-by: Ananda Ravuri <ananda.ravuri@intel.com>
> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> Signed-off-by: Wu Hao <hao.wu@intel.com>
> Acked-by: Alan Tull <atull@kernel.org>
> Signed-off-by: Moritz Fischer <mdf@kernel.org>
> ---
> drivers/fpga/dfl-fme-main.c | 3 +
> drivers/fpga/dfl-fme-mgr.c | 113 +++++++++++++++++++++++++++++++-----
> drivers/fpga/dfl-fme-pr.c | 43 +++++++++-----
> drivers/fpga/dfl-fme.h | 2 +
> drivers/fpga/dfl.h | 5 ++
> 5 files changed, 135 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/fpga/dfl-fme-main.c b/drivers/fpga/dfl-fme-main.c
> index 086ad2420ade..076d74f6416d 100644
> --- a/drivers/fpga/dfl-fme-main.c
> +++ b/drivers/fpga/dfl-fme-main.c
> @@ -21,6 +21,8 @@
> #include "dfl.h"
> #include "dfl-fme.h"
>
> +#define DRV_VERSION "0.8"
> +
> static ssize_t ports_num_show(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> @@ -277,3 +279,4 @@ MODULE_DESCRIPTION("FPGA Management Engine driver");
> MODULE_AUTHOR("Intel Corporation");
> MODULE_LICENSE("GPL v2");
> MODULE_ALIAS("platform:dfl-fme");
> +MODULE_VERSION(DRV_VERSION);
No, we ripped out these useless "driver version" things all over the
place, please do not add them back in again. They mean nothing and
confuse people to no end.
I'll not take this patch, sorry.
greg k-h
^ permalink raw reply
* Re: [PATCH] tpm: Document UEFI event log quirks
From: Jordan Hand @ 2019-07-03 17:08 UTC (permalink / raw)
To: Jarkko Sakkinen, linux-kernel, linux-integrity, linux-doc
Cc: tweek, matthewgarrett, Jonathan Corbet
In-Reply-To: <20190703161109.22935-1-jarkko.sakkinen@linux.intel.com>
On 7/3/19 9:11 AM, Jarkko Sakkinen wrote:
> There are some weird quirks when it comes to UEFI event log. Provide a
> brief introduction to TPM event log mechanism and describe the quirks
> and how they can be sorted out.
>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
> Documentation/security/tpm/tpm-eventlog.rst | 53 +++++++++++++++++++++
> 1 file changed, 53 insertions(+)
> create mode 100644 Documentation/security/tpm/tpm-eventlog.rst
>
> diff --git a/Documentation/security/tpm/tpm-eventlog.rst b/Documentation/security/tpm/tpm-eventlog.rst
> new file mode 100644
> index 000000000000..2ca8042bdb17
> --- /dev/null
> +++ b/Documentation/security/tpm/tpm-eventlog.rst
> @@ -0,0 +1,53 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +=============
> +TPM Event Log
> +=============
> +
> +| Authors:
> +| Stefan Berger <stefanb@linux.vnet.ibm.com>
> +
> +This document briefly describes what TPM log is and how it is handed
> +over from the preboot firmware to the operating system.
> +
> +Introduction
> +============
> +
> +The preboot firmware maintains an event log that gets new entries every
> +time something gets hashed by it to any of the PCR registers. The events
> +are segregated by their type and contain the value of the hashed PCR
> +register. Typically, the preboot firmware will hash the components to
> +who execution is to be handed over or actions relevant to the boot
> +process.
> +
> +The main application for this is remote attestation and the reason why
> +it is useful is nicely put in the very first section of [1]:
> +
> +"Attestation is used to provide information about the platform’s state
> +to a challenger. However, PCR contents are difficult to interpret;
> +therefore, attestation is typically more useful when the PCR contents
> +are accompanied by a measurement log. While not trusted on their own,
> +the measurement log contains a richer set of information than do the PCR
> +contents. The PCR contents are used to provide the validation of the
> +measurement log."
> +
> +UEFI event log
> +==============
> +
> +UEFI provided event log has a few somewhat weird quirks.
> +
> +Before calling ExitBootServices() Linux EFI stub copies the event log to
> +a custom configuration table defined by the stub itself. Unfortanely,
> +the events generated by ExitBootServices() do end up to the table.
do not
> +
> +The firmware provides so called final events configuration table to sort
> +out this issue. Events gets mirrored to this table after the first time
> +EFI_TCG2_PROTOCOL.GetEventLog() gets called.
> +
> +This introduces another problem: nothing guarantees that it is not
> +called before the stub gets to run. Thus, it needs to copy the final
> +events table preboot size to the custom configuration table so that
> +kernel offset it later on.
This doesn't really explain what the size will be used for. Matthew's
patch description for "tpm: Don't duplicate events from the final event
log in the TCG2 log" outlines this well. You could maybe word it
differently but I think the information is necessary:
"We can avoid this problem by looking at the size of the Final Event Log
just before we call ExitBootServices() and exporting this to the main
kernel. The kernel can then skip over all events that occured before
ExitBootServices() and only append events that were not also logged to
the main log."
> +
> +[1] https://trustedcomputinggroup.org/resource/pc-client-specific-platform-firmware-profile-specification/
> +[2] The final concatenation is done in drivers/char/tpm/eventlog/efi.c
>
Thanks,
Jordan
^ permalink raw reply
* Re: [PATCH] tpm: Document UEFI event log quirks
From: Randy Dunlap @ 2019-07-03 16:45 UTC (permalink / raw)
To: Jarkko Sakkinen, linux-kernel, linux-integrity, linux-doc
Cc: tweek, matthewgarrett, Jonathan Corbet
In-Reply-To: <20190703161109.22935-1-jarkko.sakkinen@linux.intel.com>
On 7/3/19 9:11 AM, Jarkko Sakkinen wrote:
> There are some weird quirks when it comes to UEFI event log. Provide a
> brief introduction to TPM event log mechanism and describe the quirks
> and how they can be sorted out.
>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
> Documentation/security/tpm/tpm-eventlog.rst | 53 +++++++++++++++++++++
> 1 file changed, 53 insertions(+)
> create mode 100644 Documentation/security/tpm/tpm-eventlog.rst
>
> diff --git a/Documentation/security/tpm/tpm-eventlog.rst b/Documentation/security/tpm/tpm-eventlog.rst
> new file mode 100644
> index 000000000000..2ca8042bdb17
> --- /dev/null
> +++ b/Documentation/security/tpm/tpm-eventlog.rst
> @@ -0,0 +1,53 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +=============
> +TPM Event Log
> +=============
> +
> +| Authors:
> +| Stefan Berger <stefanb@linux.vnet.ibm.com>
> +
> +This document briefly describes what TPM log is and how it is handed
> +over from the preboot firmware to the operating system.
> +
> +Introduction
> +============
> +
> +The preboot firmware maintains an event log that gets new entries every
> +time something gets hashed by it to any of the PCR registers. The events
> +are segregated by their type and contain the value of the hashed PCR
> +register. Typically, the preboot firmware will hash the components to
> +who execution is to be handed over or actions relevant to the boot
> +process.
> +
> +The main application for this is remote attestation and the reason why
> +it is useful is nicely put in the very first section of [1]:
> +
> +"Attestation is used to provide information about the platform’s state
> +to a challenger. However, PCR contents are difficult to interpret;
> +therefore, attestation is typically more useful when the PCR contents
> +are accompanied by a measurement log. While not trusted on their own,
> +the measurement log contains a richer set of information than do the PCR
> +contents. The PCR contents are used to provide the validation of the
> +measurement log."
> +
> +UEFI event log
> +==============
> +
> +UEFI provided event log has a few somewhat weird quirks.
> +
> +Before calling ExitBootServices() Linux EFI stub copies the event log to
> +a custom configuration table defined by the stub itself. Unfortanely,
Unfortunately,
> +the events generated by ExitBootServices() do end up to the table.
> +
> +The firmware provides so called final events configuration table to sort
> +out this issue. Events gets mirrored to this table after the first time
> +EFI_TCG2_PROTOCOL.GetEventLog() gets called.
> +
> +This introduces another problem: nothing guarantees that it is not
> +called before the stub gets to run. Thus, it needs to copy the final
> +events table preboot size to the custom configuration table so that
> +kernel offset it later on.
? kernel can offset it later on.
> +
> +[1] https://trustedcomputinggroup.org/resource/pc-client-specific-platform-firmware-profile-specification/
> +[2] The final concatenation is done in drivers/char/tpm/eventlog/efi.c
>
--
~Randy
^ permalink raw reply
* Re: [PATCH] mm, slab: Extend slab/shrink to shrink all the memcg caches
From: Waiman Long @ 2019-07-03 16:16 UTC (permalink / raw)
To: Michal Hocko
Cc: Andrew Morton, Christoph Lameter, Pekka Enberg, David Rientjes,
Joonsoo Kim, Alexander Viro, Jonathan Corbet, Luis Chamberlain,
Kees Cook, Johannes Weiner, Vladimir Davydov, linux-mm, linux-doc,
linux-fsdevel, cgroups, linux-kernel, Roman Gushchin,
Shakeel Butt, Andrea Arcangeli
In-Reply-To: <20190703155314.GT978@dhcp22.suse.cz>
On 7/3/19 11:53 AM, Michal Hocko wrote:
> On Wed 03-07-19 11:21:16, Waiman Long wrote:
>> On 7/2/19 5:33 PM, Andrew Morton wrote:
>>> On Tue, 2 Jul 2019 16:44:24 -0400 Waiman Long <longman@redhat.com> wrote:
>>>
>>>> On 7/2/19 4:03 PM, Andrew Morton wrote:
>>>>> On Tue, 2 Jul 2019 14:37:30 -0400 Waiman Long <longman@redhat.com> wrote:
>>>>>
>>>>>> Currently, a value of '1" is written to /sys/kernel/slab/<slab>/shrink
>>>>>> file to shrink the slab by flushing all the per-cpu slabs and free
>>>>>> slabs in partial lists. This applies only to the root caches, though.
>>>>>>
>>>>>> Extends this capability by shrinking all the child memcg caches and
>>>>>> the root cache when a value of '2' is written to the shrink sysfs file.
>>>>> Why?
>>>>>
>>>>> Please fully describe the value of the proposed feature to or users.
>>>>> Always.
>>>> Sure. Essentially, the sysfs shrink interface is not complete. It allows
>>>> the root cache to be shrunk, but not any of the memcg caches.
>>> But that doesn't describe anything of value. Who wants to use this,
>>> and why? How will it be used? What are the use-cases?
>>>
>> For me, the primary motivation of posting this patch is to have a way to
>> make the number of active objects reported in /proc/slabinfo more
>> accurately reflect the number of objects that are actually being used by
>> the kernel.
> I believe we have been through that. If the number is inexact due to
> caching then lets fix slabinfo rather than trick around it and teach
> people to do a magic write to some file that will "solve" a problem.
> This is exactly what drop_caches turned out to be in fact. People just
> got used to drop caches because they were told so by $random web page.
> So really, think about the underlying problem and try to fix it.
>
> It is true that you could argue that this patch is actually fixing the
> existing interface because it doesn't really do what it is documented to
> do and on those grounds I would agree with the change.
I do think that we should correct the shrink file to do what it is
designed to do to include the memcg caches as well.
> But do not teach
> people that they have to write to some file to get proper numbers.
> Because that is just a bad idea and it will kick back the same way
> drop_caches.
The /proc/slabinfo file is a well-known file that is probably used
relatively extensively. Making it to scan through all the per-cpu
structures will probably cause performance issues as the slab_mutex has
to be taken during the whole duration of the scan. That could have
undesirable side effect.
Instead, I am thinking about extending the slab/objects sysfs file to
also show the number of objects hold up by the per-cpu structures and
thus we can get an accurate count by subtracting it from the reported
active objects. That will have a more limited performance impact as it
is just one kmem cache instead of all the kmem caches in the system.
Also the sysfs files are not as commonly used as slabinfo. That will be
another patch in the near future.
Cheers,
Longman
^ permalink raw reply
* Re: [PATCH] mm, slab: Extend slab/shrink to shrink all the memcg caches
From: Waiman Long @ 2019-07-03 16:13 UTC (permalink / raw)
To: Christopher Lameter
Cc: Michal Hocko, Pekka Enberg, David Rientjes, Joonsoo Kim,
Andrew Morton, Alexander Viro, Jonathan Corbet, Luis Chamberlain,
Kees Cook, Johannes Weiner, Vladimir Davydov, linux-mm, linux-doc,
linux-fsdevel, cgroups, linux-kernel, Roman Gushchin,
Shakeel Butt, Andrea Arcangeli
In-Reply-To: <0100016bb89a0a6e-99d54043-4934-420f-9de0-1f71a8f943a3-000000@email.amazonses.com>
On 7/3/19 12:10 PM, Christopher Lameter wrote:
> On Wed, 3 Jul 2019, Waiman Long wrote:
>
>> On 7/3/19 2:56 AM, Michal Hocko wrote:
>>> On Tue 02-07-19 14:37:30, Waiman Long wrote:
>>>> Currently, a value of '1" is written to /sys/kernel/slab/<slab>/shrink
>>>> file to shrink the slab by flushing all the per-cpu slabs and free
>>>> slabs in partial lists. This applies only to the root caches, though.
>>>>
>>>> Extends this capability by shrinking all the child memcg caches and
>>>> the root cache when a value of '2' is written to the shrink sysfs file.
>>> Why do we need a new value for this functionality? I would tend to think
>>> that skipping memcg caches is a bug/incomplete implementation. Or is it
>>> a deliberate decision to cover root caches only?
>> It is just that I don't want to change the existing behavior of the
>> current code. It will definitely take longer to shrink both the root
>> cache and the memcg caches. If we all agree that the only sensible
>> operation is to shrink root cache and the memcg caches together. I am
>> fine just adding memcg shrink without changing the sysfs interface
>> definition and be done with it.
> I think its best and consistent behavior to shrink all memcg caches
> with the root cache. This looks like an oversight and thus a bugfix.
>
Yes, that is what I am now planning to do for the next version of the patch.
Cheers,
Longman
^ permalink raw reply
* [PATCH] tpm: Document UEFI event log quirks
From: Jarkko Sakkinen @ 2019-07-03 16:11 UTC (permalink / raw)
To: linux-kernel, linux-integrity, linux-doc
Cc: tweek, matthewgarrett, Jarkko Sakkinen, Jonathan Corbet
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 2843 bytes --]
There are some weird quirks when it comes to UEFI event log. Provide a
brief introduction to TPM event log mechanism and describe the quirks
and how they can be sorted out.
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
Documentation/security/tpm/tpm-eventlog.rst | 53 +++++++++++++++++++++
1 file changed, 53 insertions(+)
create mode 100644 Documentation/security/tpm/tpm-eventlog.rst
diff --git a/Documentation/security/tpm/tpm-eventlog.rst b/Documentation/security/tpm/tpm-eventlog.rst
new file mode 100644
index 000000000000..2ca8042bdb17
--- /dev/null
+++ b/Documentation/security/tpm/tpm-eventlog.rst
@@ -0,0 +1,53 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+=============
+TPM Event Log
+=============
+
+| Authors:
+| Stefan Berger <stefanb@linux.vnet.ibm.com>
+
+This document briefly describes what TPM log is and how it is handed
+over from the preboot firmware to the operating system.
+
+Introduction
+============
+
+The preboot firmware maintains an event log that gets new entries every
+time something gets hashed by it to any of the PCR registers. The events
+are segregated by their type and contain the value of the hashed PCR
+register. Typically, the preboot firmware will hash the components to
+who execution is to be handed over or actions relevant to the boot
+process.
+
+The main application for this is remote attestation and the reason why
+it is useful is nicely put in the very first section of [1]:
+
+"Attestation is used to provide information about the platform’s state
+to a challenger. However, PCR contents are difficult to interpret;
+therefore, attestation is typically more useful when the PCR contents
+are accompanied by a measurement log. While not trusted on their own,
+the measurement log contains a richer set of information than do the PCR
+contents. The PCR contents are used to provide the validation of the
+measurement log."
+
+UEFI event log
+==============
+
+UEFI provided event log has a few somewhat weird quirks.
+
+Before calling ExitBootServices() Linux EFI stub copies the event log to
+a custom configuration table defined by the stub itself. Unfortanely,
+the events generated by ExitBootServices() do end up to the table.
+
+The firmware provides so called final events configuration table to sort
+out this issue. Events gets mirrored to this table after the first time
+EFI_TCG2_PROTOCOL.GetEventLog() gets called.
+
+This introduces another problem: nothing guarantees that it is not
+called before the stub gets to run. Thus, it needs to copy the final
+events table preboot size to the custom configuration table so that
+kernel offset it later on.
+
+[1] https://trustedcomputinggroup.org/resource/pc-client-specific-platform-firmware-profile-specification/
+[2] The final concatenation is done in drivers/char/tpm/eventlog/efi.c
--
2.20.1
^ permalink raw reply related
* Re: [PATCH] mm, slab: Extend slab/shrink to shrink all the memcg caches
From: Christopher Lameter @ 2019-07-03 16:10 UTC (permalink / raw)
To: Waiman Long
Cc: Michal Hocko, Pekka Enberg, David Rientjes, Joonsoo Kim,
Andrew Morton, Alexander Viro, Jonathan Corbet, Luis Chamberlain,
Kees Cook, Johannes Weiner, Vladimir Davydov, linux-mm, linux-doc,
linux-fsdevel, cgroups, linux-kernel, Roman Gushchin,
Shakeel Butt, Andrea Arcangeli
In-Reply-To: <9ade5859-b937-c1ac-9881-2289d734441d@redhat.com>
On Wed, 3 Jul 2019, Waiman Long wrote:
> On 7/3/19 2:56 AM, Michal Hocko wrote:
> > On Tue 02-07-19 14:37:30, Waiman Long wrote:
> >> Currently, a value of '1" is written to /sys/kernel/slab/<slab>/shrink
> >> file to shrink the slab by flushing all the per-cpu slabs and free
> >> slabs in partial lists. This applies only to the root caches, though.
> >>
> >> Extends this capability by shrinking all the child memcg caches and
> >> the root cache when a value of '2' is written to the shrink sysfs file.
> > Why do we need a new value for this functionality? I would tend to think
> > that skipping memcg caches is a bug/incomplete implementation. Or is it
> > a deliberate decision to cover root caches only?
>
> It is just that I don't want to change the existing behavior of the
> current code. It will definitely take longer to shrink both the root
> cache and the memcg caches. If we all agree that the only sensible
> operation is to shrink root cache and the memcg caches together. I am
> fine just adding memcg shrink without changing the sysfs interface
> definition and be done with it.
I think its best and consistent behavior to shrink all memcg caches
with the root cache. This looks like an oversight and thus a bugfix.
^ permalink raw reply
* Re: [PATCH] mm, slab: Extend slab/shrink to shrink all the memcg caches
From: Michal Hocko @ 2019-07-03 15:53 UTC (permalink / raw)
To: Waiman Long
Cc: Andrew Morton, Christoph Lameter, Pekka Enberg, David Rientjes,
Joonsoo Kim, Alexander Viro, Jonathan Corbet, Luis Chamberlain,
Kees Cook, Johannes Weiner, Vladimir Davydov, linux-mm, linux-doc,
linux-fsdevel, cgroups, linux-kernel, Roman Gushchin,
Shakeel Butt, Andrea Arcangeli
In-Reply-To: <c29ff725-95ba-db4d-944f-d33f5f766cd3@redhat.com>
On Wed 03-07-19 11:21:16, Waiman Long wrote:
> On 7/2/19 5:33 PM, Andrew Morton wrote:
> > On Tue, 2 Jul 2019 16:44:24 -0400 Waiman Long <longman@redhat.com> wrote:
> >
> >> On 7/2/19 4:03 PM, Andrew Morton wrote:
> >>> On Tue, 2 Jul 2019 14:37:30 -0400 Waiman Long <longman@redhat.com> wrote:
> >>>
> >>>> Currently, a value of '1" is written to /sys/kernel/slab/<slab>/shrink
> >>>> file to shrink the slab by flushing all the per-cpu slabs and free
> >>>> slabs in partial lists. This applies only to the root caches, though.
> >>>>
> >>>> Extends this capability by shrinking all the child memcg caches and
> >>>> the root cache when a value of '2' is written to the shrink sysfs file.
> >>> Why?
> >>>
> >>> Please fully describe the value of the proposed feature to or users.
> >>> Always.
> >> Sure. Essentially, the sysfs shrink interface is not complete. It allows
> >> the root cache to be shrunk, but not any of the memcg caches.
> > But that doesn't describe anything of value. Who wants to use this,
> > and why? How will it be used? What are the use-cases?
> >
> For me, the primary motivation of posting this patch is to have a way to
> make the number of active objects reported in /proc/slabinfo more
> accurately reflect the number of objects that are actually being used by
> the kernel.
I believe we have been through that. If the number is inexact due to
caching then lets fix slabinfo rather than trick around it and teach
people to do a magic write to some file that will "solve" a problem.
This is exactly what drop_caches turned out to be in fact. People just
got used to drop caches because they were told so by $random web page.
So really, think about the underlying problem and try to fix it.
It is true that you could argue that this patch is actually fixing the
existing interface because it doesn't really do what it is documented to
do and on those grounds I would agree with the change. But do not teach
people that they have to write to some file to get proper numbers.
Because that is just a bad idea and it will kick back the same way
drop_caches.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH] mm, slab: Extend slab/shrink to shrink all the memcg caches
From: Waiman Long @ 2019-07-03 15:21 UTC (permalink / raw)
To: Andrew Morton
Cc: Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim,
Alexander Viro, Jonathan Corbet, Luis Chamberlain, Kees Cook,
Johannes Weiner, Michal Hocko, Vladimir Davydov, linux-mm,
linux-doc, linux-fsdevel, cgroups, linux-kernel, Roman Gushchin,
Shakeel Butt, Andrea Arcangeli
In-Reply-To: <20190702143340.715f771192721f60de1699d7@linux-foundation.org>
On 7/2/19 5:33 PM, Andrew Morton wrote:
> On Tue, 2 Jul 2019 16:44:24 -0400 Waiman Long <longman@redhat.com> wrote:
>
>> On 7/2/19 4:03 PM, Andrew Morton wrote:
>>> On Tue, 2 Jul 2019 14:37:30 -0400 Waiman Long <longman@redhat.com> wrote:
>>>
>>>> Currently, a value of '1" is written to /sys/kernel/slab/<slab>/shrink
>>>> file to shrink the slab by flushing all the per-cpu slabs and free
>>>> slabs in partial lists. This applies only to the root caches, though.
>>>>
>>>> Extends this capability by shrinking all the child memcg caches and
>>>> the root cache when a value of '2' is written to the shrink sysfs file.
>>> Why?
>>>
>>> Please fully describe the value of the proposed feature to or users.
>>> Always.
>> Sure. Essentially, the sysfs shrink interface is not complete. It allows
>> the root cache to be shrunk, but not any of the memcg caches.
> But that doesn't describe anything of value. Who wants to use this,
> and why? How will it be used? What are the use-cases?
>
For me, the primary motivation of posting this patch is to have a way to
make the number of active objects reported in /proc/slabinfo more
accurately reflect the number of objects that are actually being used by
the kernel. When measuring changes in slab objects consumption between
successive run of a certain workload, I can more easily see the amount
of increase. Without that, the data will have much more noise and it
will be harder to see a pattern.
Cheers,
Longman
^ permalink raw reply
* Re: [PATCH] mm, slab: Extend slab/shrink to shrink all the memcg caches
From: Waiman Long @ 2019-07-03 15:14 UTC (permalink / raw)
To: Michal Hocko
Cc: Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim,
Andrew Morton, Alexander Viro, Jonathan Corbet, Luis Chamberlain,
Kees Cook, Johannes Weiner, Vladimir Davydov, linux-mm, linux-doc,
linux-fsdevel, cgroups, linux-kernel, Roman Gushchin,
Shakeel Butt, Andrea Arcangeli
In-Reply-To: <20190703143701.GR978@dhcp22.suse.cz>
On 7/3/19 10:37 AM, Michal Hocko wrote:
> On Wed 03-07-19 09:12:13, Waiman Long wrote:
>> On 7/3/19 2:56 AM, Michal Hocko wrote:
>>> On Tue 02-07-19 14:37:30, Waiman Long wrote:
>>>> Currently, a value of '1" is written to /sys/kernel/slab/<slab>/shrink
>>>> file to shrink the slab by flushing all the per-cpu slabs and free
>>>> slabs in partial lists. This applies only to the root caches, though.
>>>>
>>>> Extends this capability by shrinking all the child memcg caches and
>>>> the root cache when a value of '2' is written to the shrink sysfs file.
>>> Why do we need a new value for this functionality? I would tend to think
>>> that skipping memcg caches is a bug/incomplete implementation. Or is it
>>> a deliberate decision to cover root caches only?
>> It is just that I don't want to change the existing behavior of the
>> current code. It will definitely take longer to shrink both the root
>> cache and the memcg caches.
> Does that matter? To whom and why? I do not expect this interface to be
> used heavily.
The only concern that I can see is the fact that I need to take the
slab_mutex when iterating the memcg list to prevent concurrent
modification. That may have some impact on other applications running in
the system. However, I can put a precaution statement on the user-doc to
discuss the potential performance impact.
>> If we all agree that the only sensible
>> operation is to shrink root cache and the memcg caches together. I am
>> fine just adding memcg shrink without changing the sysfs interface
>> definition and be done with it.
> The existing documentation is really modest on the actual semantic:
> Description:
> The shrink file is written when memory should be reclaimed from
> a cache. Empty partial slabs are freed and the partial list is
> sorted so the slabs with the fewest available objects are used
> first.
>
> which to me sounds like all slabs are free and nobody should be really
> thinking of memcgs. This is simply drop_caches kinda thing. We surely do
> not want to drop caches only for the root memcg for /proc/sys/vm/drop_caches
> right?
>
I am planning to reword the document to make the effect of using this
sysfs file more explicit.
Cheers,
Longman
^ permalink raw reply
* Re: [PATCH] mm, slab: Extend slab/shrink to shrink all the memcg caches
From: Michal Hocko @ 2019-07-03 14:37 UTC (permalink / raw)
To: Waiman Long
Cc: Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim,
Andrew Morton, Alexander Viro, Jonathan Corbet, Luis Chamberlain,
Kees Cook, Johannes Weiner, Vladimir Davydov, linux-mm, linux-doc,
linux-fsdevel, cgroups, linux-kernel, Roman Gushchin,
Shakeel Butt, Andrea Arcangeli
In-Reply-To: <9ade5859-b937-c1ac-9881-2289d734441d@redhat.com>
On Wed 03-07-19 09:12:13, Waiman Long wrote:
> On 7/3/19 2:56 AM, Michal Hocko wrote:
> > On Tue 02-07-19 14:37:30, Waiman Long wrote:
> >> Currently, a value of '1" is written to /sys/kernel/slab/<slab>/shrink
> >> file to shrink the slab by flushing all the per-cpu slabs and free
> >> slabs in partial lists. This applies only to the root caches, though.
> >>
> >> Extends this capability by shrinking all the child memcg caches and
> >> the root cache when a value of '2' is written to the shrink sysfs file.
> > Why do we need a new value for this functionality? I would tend to think
> > that skipping memcg caches is a bug/incomplete implementation. Or is it
> > a deliberate decision to cover root caches only?
>
> It is just that I don't want to change the existing behavior of the
> current code. It will definitely take longer to shrink both the root
> cache and the memcg caches.
Does that matter? To whom and why? I do not expect this interface to be
used heavily.
> If we all agree that the only sensible
> operation is to shrink root cache and the memcg caches together. I am
> fine just adding memcg shrink without changing the sysfs interface
> definition and be done with it.
The existing documentation is really modest on the actual semantic:
Description:
The shrink file is written when memory should be reclaimed from
a cache. Empty partial slabs are freed and the partial list is
sorted so the slabs with the fewest available objects are used
first.
which to me sounds like all slabs are free and nobody should be really
thinking of memcgs. This is simply drop_caches kinda thing. We surely do
not want to drop caches only for the root memcg for /proc/sys/vm/drop_caches
right?
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH v2 0/2] Clean up crypto documentation
From: Herbert Xu @ 2019-07-03 14:28 UTC (permalink / raw)
To: Hook, Gary
Cc: corbet@lwn.net, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-crypto@vger.kernel.org,
davem@davemloft.net
In-Reply-To: <156150616764.22527.16524544899486041609.stgit@taos>
On Tue, Jun 25, 2019 at 11:43:36PM +0000, Hook, Gary wrote:
> Tidy up the crypto documentation by filling in some variable
> descriptions, make some grammatical corrections, and enhance
> formatting.
>
> Changes since v1:
> - Remove patch with superfluous change to index (patch 2)
> - Remove unnecessary markup on function names in patch 3
> - Un-add extraneous white space (patch 3)
>
> ---
>
> Gary R Hook (2):
> crypto: doc - Add parameter documentation
> crypto: doc - Fix formatting of new crypto engine content
>
>
> Documentation/crypto/api-skcipher.rst | 2 -
> Documentation/crypto/crypto_engine.rst | 111 +++++++++++++++++++++-----------
> include/linux/crypto.h | 11 +++
> 3 files changed, 85 insertions(+), 39 deletions(-)
All applied. Thanks.
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ 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