date:20190812

Quoting Brendan Higgins (2019-08-12 11:24:16)
> From: Felix Guo 
> 
> The ultimate goal is to create minimal isolated test binaries; in the
> meantime we are using UML to provide the infrastructure to run tests, so
> define an abstract way to configure and run tests that allow us to
> change the context in which tests are built without affecting the user.
> This also makes pretty and dynamic error reporting, and a lot of other
> nice features easier.
> 
> kunit_config.py:
>   - parse .config and Kconfig files.
> 
> kunit_kernel.py: provides helper functions to:
>   - configure the kernel using kunitconfig.
>   - build the kernel with the appropriate configuration.
>   - provide function to invoke the kernel and stream the output back.
> 
> kunit_parser.py: parses raw logs returned out by kunit_kernel and
> displays them in a user friendly way.
> 
> test_data/*: samples of test data for testing kunit.py, kunit_config.py,
> etc.
> 
> Signed-off-by: Felix Guo 
> Signed-off-by: Brendan Higgins 
> Reviewed-by: Greg Kroah-Hartman 
> Reviewed-by: Logan Gunthorpe 
> ---

Reviewed-by: Stephen Boyd 

I look forward to the single binary solution, but until then, we have
this.

___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v12 10/18] kunit: test: add tests for kunit test abort

Quoting Brendan Higgins (2019-08-12 22:06:04)
> On Mon, Aug 12, 2019 at 9:24 PM Stephen Boyd  wrote:
> >
> > Quoting Brendan Higgins (2019-08-12 11:24:13)
> > > +
> > > +static int kunit_try_catch_test_init(struct kunit *test)
> > > +{
> > > +   struct kunit_try_catch_test_context *ctx;
> > > +
> > > +   ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
> >
> > Can this fail? Should return -ENOMEM in that case?
> 
> Yes, I should do that.

Looks like it's asserted to not be an error. If it's pushed into the API
then there's nothing to do here, and you can have my reviewed-by on this
patch.

Reviewed-by: Stephen Boyd 

___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v12 09/18] kunit: test: add support for test abort

Quoting Brendan Higgins (2019-08-12 21:57:55)
> On Mon, Aug 12, 2019 at 9:22 PM Stephen Boyd  wrote:
> >
> > Quoting Brendan Higgins (2019-08-12 11:24:12)
> > > diff --git a/include/kunit/test.h b/include/kunit/test.h
> > > index 2625bcfeb19ac..93381f841e09f 100644
> > > --- a/include/kunit/test.h
> > > +++ b/include/kunit/test.h
> > > @@ -176,6 +178,11 @@ struct kunit {
> > >  */
> > > bool success; /* Read only after test_case finishes! */
> > > spinlock_t lock; /* Gaurds all mutable test state. */
> > > +   /*
> > > +* death_test may be both set and unset from multiple threads in 
> > > a test
> > > +* case.
> > > +*/
> > > +   bool death_test; /* Protected by lock. */
> > > /*
> > >  * Because resources is a list that may be updated multiple times 
> > > (with
> > >  * new resources) from any thread associated with a test case, we 
> > > must
> > > @@ -184,6 +191,13 @@ struct kunit {
> > > struct list_head resources; /* Protected by lock. */
> > >  };
> > >
> > > +static inline void kunit_set_death_test(struct kunit *test, bool 
> > > death_test)
> > > +{
> > > +   spin_lock(&test->lock);
> > > +   test->death_test = death_test;
> > > +   spin_unlock(&test->lock);
> > > +}
> >
> > These getters and setters are using spinlocks again. It doesn't make any
> > sense. It probably needs a rework like was done for the other bool
> > member, success.
> 
> No, this is intentional. death_test can transition from false to true
> and then back to false within the same test. Maybe that deserves a
> comment?

Yes. How does it transition from true to false again?

Either way, having a spinlock around a read/write API doesn't make sense
because it just makes sure that two writes don't overlap, but otherwise
does nothing to keep things synchronized. For example a set to true
after a set to false when the two calls to set true or false aren't
synchronized means they can happen in any order. So I don't see how it
needs a spinlock. The lock needs to be one level higher.

> 
> > > +
> > >  void kunit_init_test(struct kunit *test, const char *name);
> > >
> > >  int kunit_run_tests(struct kunit_suite *suite);
> > > diff --git a/include/kunit/try-catch.h b/include/kunit/try-catch.h
> > > new file mode 100644
> > > index 0..8a414a9af0b64
> > > --- /dev/null
> > > +++ b/include/kunit/try-catch.h
[...]
> > > +
> > > +/*
> > > + * struct kunit_try_catch - provides a generic way to run code which 
> > > might fail.
> > > + * @context: used to pass user data to the try and catch functions.
> > > + *
> > > + * kunit_try_catch provides a generic, architecture independent way to 
> > > execute
> > > + * an arbitrary function of type kunit_try_catch_func_t which may bail 
> > > out by
> > > + * calling kunit_try_catch_throw(). If kunit_try_catch_throw() is 
> > > called, @try
> > > + * is stopped at the site of invocation and @catch is catch is called.
> > > + *
> > > + * struct kunit_try_catch provides a generic interface for the 
> > > functionality
> > > + * needed to implement kunit->abort() which in turn is needed for 
> > > implementing
> > > + * assertions. Assertions allow stating a precondition for a test 
> > > simplifying
> > > + * how test cases are written and presented.
> > > + *
> > > + * Assertions are like expectations, except they abort (call
> > > + * kunit_try_catch_throw()) when the specified condition is not met. 
> > > This is
> > > + * useful when you look at a test case as a logical statement about some 
> > > piece
> > > + * of code, where assertions are the premises for the test case, and the
> > > + * conclusion is a set of predicates, rather expectations, that must all 
> > > be
> > > + * true. If your premises are violated, it does not makes sense to 
> > > continue.
> > > + */
> > > +struct kunit_try_catch {
> > > +   /* private: internal use only. */
> > > +   struct kunit *test;
> > > +   struct completion *try_completion;
> > > +   int try_result;
> > > +   kunit_try_catch_func_t try;
> > > +   kunit_try_catch_func_t catch;
> >
> > Can these other variables be documented in the kernel doc? And should
> > context be marked as 'public'?
> 
> Sure, I can document them.
> 
> But I don't think context should be public; it should only be accessed
> by kunit_try_catch_* functions. context should only be populated by
> *_init, and will be passed into *try and *catch when they are called
> internally.

Ok. Then I guess just document them all but keep them all marked as
private.

> 
> > > + */
> > > +void kunit_generic_try_catch_init(struct kunit_try_catch *try_catch);
> > > +
> > > +#endif /* _KUNIT_TRY_CATCH_H */
> > > diff --git a/kunit/test.c b/kunit/test.c
> > > index e5080a2c6b29c..995cb53fe4ee9 100644
> > > --- a/kunit/test.c
> > > +++ b/kunit/test.c
> > > @@ -158,6 +171,21 @@ static void kunit_fail(struct kunit *test, struct 
> > > kunit_assert *assert)
> > > ku

Re: [PATCH v12 03/18] kunit: test: add string_stream a std::stream like string builder

Quoting Brendan Higgins (2019-08-12 22:02:59)
> On Mon, Aug 12, 2019 at 9:56 PM Stephen Boyd  wrote:
> >
> > Quoting Brendan Higgins (2019-08-12 17:41:05)
> > > On Mon, Aug 12, 2019 at 4:59 PM Stephen Boyd  wrote:
> > > >
> > > > > kunit_resource_destroy (respective equivalents to devm_kfree, and
> > > > > devres_destroy) and use kunit_kfree here?
> > > > >
> > > >
> > > > Yes, or drop the API entirely? Does anything need this functionality?
> > >
> > > Drop the kunit_resource API? I would strongly prefer not to.
> >
> > No. I mean this API, string_stream_clear(). Does anything use it?
> 
> Oh, right. No.
> 
> However, now that I added the kunit_resource_destroy, I thought it
> might be good to free the string_stream after I use it in each call to
> kunit_assert->format(...) in which case I will be using this logic.
> 
> So I think the right thing to do is to expose string_stream_destroy so
> kunit_do_assert can clean up when it's done, and then demote
> string_stream_clear to static. Sound good?

Ok, sure. I don't really see how clearing it explicitly when the
assertion prints vs. never allocating it to begin with is really any
different. Maybe I've missed something though.

___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v12 16/18] MAINTAINERS: add entry for KUnit the unit testing framework

Quoting Brendan Higgins (2019-08-12 11:24:19)
> Add myself as maintainer of KUnit, the Linux kernel's unit testing
> framework.
> 
> Signed-off-by: Brendan Higgins 
> Reviewed-by: Greg Kroah-Hartman 
> Reviewed-by: Logan Gunthorpe 
> ---

Reviewed-by: Stephen Boyd 

___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v12 11/18] kunit: test: add the concept of assertions

On Mon, Aug 12, 2019 at 9:55 PM Stephen Boyd  wrote:
>
> Quoting Brendan Higgins (2019-08-12 11:24:14)
> > Add support for assertions which are like expectations except the test
> > terminates if the assertion is not satisfied.
> >
> > The idea with assertions is that you use them to state all the
> > preconditions for your test. Logically speaking, these are the premises
> > of the test case, so if a premise isn't true, there is no point in
> > continuing the test case because there are no conclusions that can be
> > drawn without the premises. Whereas, the expectation is the thing you
> > are trying to prove. It is not used universally in x-unit style test
> > frameworks, but I really like it as a convention.  You could still
> > express the idea of a premise using the above idiom, but I think
> > KUNIT_ASSERT_* states the intended idea perfectly.
> >
> > Signed-off-by: Brendan Higgins 
> > Reviewed-by: Greg Kroah-Hartman 
> > Reviewed-by: Logan Gunthorpe 
>
> Reviewed-by: Stephen Boyd 
>
> > + * Sets an expectation that the values that @left and @right evaluate to 
> > are
> > + * not equal. This is semantically equivalent to
> > + * KUNIT_ASSERT_TRUE(@test, strcmp((@left), (@right))). See 
> > KUNIT_ASSERT_TRUE()
> > + * for more information.
> > + */
> > +#define KUNIT_ASSERT_STRNEQ(test, left, right) 
> >\
> > +   KUNIT_BINARY_STR_NE_ASSERTION(test, 
> >\
> > + KUNIT_ASSERTION,  
> >\
> > + left, 
> >\
> > + right)
> > +
> > +#define KUNIT_ASSERT_STRNEQ_MSG(test, left, right, fmt, ...)   
> >\
> > +   KUNIT_BINARY_STR_NE_MSG_ASSERTION(test, 
> >\
> > + KUNIT_ASSERTION,  
> >\
> > + left, 
> >\
> > + right,
> >\
> > + fmt,  
> >\
>
> Same question about tabbing too.

Yep. WIll fix.

> > diff --git a/kunit/test-test.c b/kunit/test-test.c
> > index 88f4cdf03db2a..058f3fb37458a 100644
> > --- a/kunit/test-test.c
> > +++ b/kunit/test-test.c
> > @@ -78,11 +78,13 @@ static int kunit_try_catch_test_init(struct kunit *test)
> > struct kunit_try_catch_test_context *ctx;
> >
> > ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
> > +   KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
>
> Ah ok. Question still stands if kunit_kzalloc() should just have the
> assertion on failure.

Right. In the previous patch KUNIT_ASSERT_* doesn't exist yet, so I
can't use it. And rather than fall back to return -ENOMEM like I
should have, I evidently forgot to do that.

> > test->priv = ctx;
> >
> > ctx->try_catch = kunit_kmalloc(test,
> >sizeof(*ctx->try_catch),
> >GFP_KERNEL);
> > +   KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx->try_catch);
> >
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v12 10/18] kunit: test: add tests for kunit test abort

On Mon, Aug 12, 2019 at 9:24 PM Stephen Boyd  wrote:
>
> Quoting Brendan Higgins (2019-08-12 11:24:13)
> > +
> > +static int kunit_try_catch_test_init(struct kunit *test)
> > +{
> > +   struct kunit_try_catch_test_context *ctx;
> > +
> > +   ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
>
> Can this fail? Should return -ENOMEM in that case?

Yes, I should do that.

> > +   test->priv = ctx;
> > +
> > +   ctx->try_catch = kunit_kmalloc(test,
> > +  sizeof(*ctx->try_catch),
> > +  GFP_KERNEL);
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v12 05/18] kunit: test: add the concept of expectations

On Mon, Aug 12, 2019 at 10:02 PM Stephen Boyd  wrote:
>
> Quoting Brendan Higgins (2019-08-12 17:33:52)
> > On Mon, Aug 12, 2019 at 04:57:00PM -0700, Stephen Boyd wrote:
> > > Quoting Brendan Higgins (2019-08-12 11:24:08)
> > > > + */
> > > > +#define KUNIT_EXPECT_TRUE(test, condition) \
> > > > +   KUNIT_TRUE_ASSERTION(test, KUNIT_EXPECTATION, condition)
> > >
> > > A lot of these macros seem double indented.
> >
> > In a case you pointed out in the preceding patch, I was just keeping the
> > arguments column aligned.
> >
> > In this case I am just indenting two tabs for a line continuation. I
> > thought I found other instances in the kernel that did this early on
> > (and that's also what the Linux kernel vim plugin wanted me to do).
> > After a couple of spot checks, it seems like one tab for this kind of
> > line continuation seems more common. I personally don't feel strongly
> > about any particular version. I just want to know now what the correct
> > indentation is for macros before I go through and change them all.
> >
> > I think there are three cases:
> >
> > #define macro0(param0, param1) \
> > a_really_long_macro(...)
> >
> > In this first case, I use two tabs for the first indent, I think you are
> > telling me this should be one tab.
>
> Yes. Should be one.
>
> >
> > #define macro1(param0, param1) {
> >\
> > statement_in_a_block0;  
> >\
> > statement_in_a_block1;  
> >\
> > ... 
> >\
> > }
> >
> > In this case, every line is in a block and is indented as it would be in
> > a function body. I think you are okay with this, and now that I am
> > thinking about it, what I think you are proposing for macro0 will make
> > these two cases more consistent.
> >
> > #define macro2(param0,  
> >\
> >param1,  
> >\
> >param2,  
> >\
> >param3,  
> >\
> >..., 
> >\
> >paramn) ...  
> >\
> >
> > In this last case, the body would be indented as in macro0, or macro1,
> > but the parameters passed into the macro are column aligned, consistent
> > with one of the acceptable ways of formatting function parameters that
> > don't fit on a single line.
> >
> > In all cases, I put 1 space in between the closing parameter paren and
> > the line continuation `\`, if only one `\` is needed. Otherwise, I align
> > all the `\s` to the 80th column. Is this okay, or would you prefer that
> > I align them all to the 80th column, or something else?
> >
>
> This all sounds fine and I'm not nitpicking this style. Just the double
> tabs making lines longer than required.

Sounds good. Will do.
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v12 04/18] kunit: test: add assertion printing library

On Mon, Aug 12, 2019 at 9:57 PM Stephen Boyd  wrote:
>
> Quoting Brendan Higgins (2019-08-12 21:27:05)
> > On Mon, Aug 12, 2019 at 4:56 PM Brendan Higgins
> >  wrote:
> > >
> > > On Mon, Aug 12, 2019 at 4:46 PM Stephen Boyd  wrote:
> > > >
> > > > Quoting Brendan Higgins (2019-08-12 11:24:07)
> > > > > +#define KUNIT_INIT_FAIL_ASSERT_STRUCT(test, type) {  
> > > > >  \
> > > > > +   .assert = KUNIT_INIT_ASSERT_STRUCT(test,  
> > > > >  \
> > > > > +  type,  
> > > > >  \
> > > > > +  
> > > > > kunit_fail_assert_format)   \
> > > >
> > > > This one got indented one too many times?
> > >
> > > Not unless I have been using the wrong formatting for multiline
> > > macros. You can see this commit applied here:
> > > https://kunit.googlesource.com/linux/+/870964da2990920030990dd1ffb647ef408e52df/include/kunit/assert.h#59
> > >
> > > I have test, type, and kunit_fail_assert_format all column aligned (it
> > > just doesn't render nicely in the patch format).
> >
> > Disregard that last comment. I just looked at the line immediately
> > above your comment and thought it looked correct. Sorry about that
> > (you were pointing out that the .assert line looked wrong, correct?).
>
> Yes. .assert is double tabbed?

Yes it is. Sorry about the confusion. Will fix.
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v12 03/18] kunit: test: add string_stream a std::stream like string builder

On Mon, Aug 12, 2019 at 9:56 PM Stephen Boyd  wrote:
>
> Quoting Brendan Higgins (2019-08-12 17:41:05)
> > On Mon, Aug 12, 2019 at 4:59 PM Stephen Boyd  wrote:
> > >
> > > > kunit_resource_destroy (respective equivalents to devm_kfree, and
> > > > devres_destroy) and use kunit_kfree here?
> > > >
> > >
> > > Yes, or drop the API entirely? Does anything need this functionality?
> >
> > Drop the kunit_resource API? I would strongly prefer not to.
>
> No. I mean this API, string_stream_clear(). Does anything use it?

Oh, right. No.

However, now that I added the kunit_resource_destroy, I thought it
might be good to free the string_stream after I use it in each call to
kunit_assert->format(...) in which case I will be using this logic.

So I think the right thing to do is to expose string_stream_destroy so
kunit_do_assert can clean up when it's done, and then demote
string_stream_clear to static. Sound good?
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v12 05/18] kunit: test: add the concept of expectations

Quoting Brendan Higgins (2019-08-12 17:33:52)
> On Mon, Aug 12, 2019 at 04:57:00PM -0700, Stephen Boyd wrote:
> > Quoting Brendan Higgins (2019-08-12 11:24:08)
> > > + */
> > > +#define KUNIT_EXPECT_TRUE(test, condition) \
> > > +   KUNIT_TRUE_ASSERTION(test, KUNIT_EXPECTATION, condition)
> > 
> > A lot of these macros seem double indented.
> 
> In a case you pointed out in the preceding patch, I was just keeping the
> arguments column aligned.
> 
> In this case I am just indenting two tabs for a line continuation. I
> thought I found other instances in the kernel that did this early on
> (and that's also what the Linux kernel vim plugin wanted me to do).
> After a couple of spot checks, it seems like one tab for this kind of
> line continuation seems more common. I personally don't feel strongly
> about any particular version. I just want to know now what the correct
> indentation is for macros before I go through and change them all.
> 
> I think there are three cases:
> 
> #define macro0(param0, param1) \
> a_really_long_macro(...)
> 
> In this first case, I use two tabs for the first indent, I think you are
> telling me this should be one tab.

Yes. Should be one.

> 
> #define macro1(param0, param1) {  
>  \
> statement_in_a_block0;
>  \
> statement_in_a_block1;
>  \
> ...   
>  \
> }
> 
> In this case, every line is in a block and is indented as it would be in
> a function body. I think you are okay with this, and now that I am
> thinking about it, what I think you are proposing for macro0 will make
> these two cases more consistent.
> 
> #define macro2(param0,
>  \
>param1,
>  \
>param2,
>  \
>param3,
>  \
>...,   
>  \
>paramn) ...
>  \
> 
> In this last case, the body would be indented as in macro0, or macro1,
> but the parameters passed into the macro are column aligned, consistent
> with one of the acceptable ways of formatting function parameters that
> don't fit on a single line.
> 
> In all cases, I put 1 space in between the closing parameter paren and
> the line continuation `\`, if only one `\` is needed. Otherwise, I align
> all the `\s` to the 80th column. Is this okay, or would you prefer that
> I align them all to the 80th column, or something else?
> 

This all sounds fine and I'm not nitpicking this style. Just the double
tabs making lines longer than required.

___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v12 09/18] kunit: test: add support for test abort

On Mon, Aug 12, 2019 at 9:22 PM Stephen Boyd  wrote:
>
> Quoting Brendan Higgins (2019-08-12 11:24:12)
> > diff --git a/include/kunit/test.h b/include/kunit/test.h
> > index 2625bcfeb19ac..93381f841e09f 100644
> > --- a/include/kunit/test.h
> > +++ b/include/kunit/test.h
> > @@ -13,6 +13,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >
> >  struct kunit_resource;
> >
> > @@ -167,6 +168,7 @@ struct kunit {
> >
> > /* private: internal use only. */
> > const char *name; /* Read only after initialization! */
> > +   struct kunit_try_catch try_catch;
> > /*
> >  * success starts as true, and may only be set to false during a 
> > test
> >  * case; thus, it is safe to update this across multiple threads 
> > using
> > @@ -176,6 +178,11 @@ struct kunit {
> >  */
> > bool success; /* Read only after test_case finishes! */
> > spinlock_t lock; /* Gaurds all mutable test state. */
> > +   /*
> > +* death_test may be both set and unset from multiple threads in a 
> > test
> > +* case.
> > +*/
> > +   bool death_test; /* Protected by lock. */
> > /*
> >  * Because resources is a list that may be updated multiple times 
> > (with
> >  * new resources) from any thread associated with a test case, we 
> > must
> > @@ -184,6 +191,13 @@ struct kunit {
> > struct list_head resources; /* Protected by lock. */
> >  };
> >
> > +static inline void kunit_set_death_test(struct kunit *test, bool 
> > death_test)
> > +{
> > +   spin_lock(&test->lock);
> > +   test->death_test = death_test;
> > +   spin_unlock(&test->lock);
> > +}
>
> These getters and setters are using spinlocks again. It doesn't make any
> sense. It probably needs a rework like was done for the other bool
> member, success.

No, this is intentional. death_test can transition from false to true
and then back to false within the same test. Maybe that deserves a
comment?

> > +
> >  void kunit_init_test(struct kunit *test, const char *name);
> >
> >  int kunit_run_tests(struct kunit_suite *suite);
> > diff --git a/include/kunit/try-catch.h b/include/kunit/try-catch.h
> > new file mode 100644
> > index 0..8a414a9af0b64
> > --- /dev/null
> > +++ b/include/kunit/try-catch.h
> > @@ -0,0 +1,69 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * An API to allow a function, that may fail, to be executed, and recover 
> > in a
> > + * controlled manner.
> > + *
> > + * Copyright (C) 2019, Google LLC.
> > + * Author: Brendan Higgins 
> > + */
> > +
> > +#ifndef _KUNIT_TRY_CATCH_H
> > +#define _KUNIT_TRY_CATCH_H
> > +
> > +#include 
> > +
> > +typedef void (*kunit_try_catch_func_t)(void *);
> > +
> > +struct kunit;
>
> Forward declare struct completion?

Sure. Will do.

> > +
> > +/*
> > + * struct kunit_try_catch - provides a generic way to run code which might 
> > fail.
> > + * @context: used to pass user data to the try and catch functions.
> > + *
> > + * kunit_try_catch provides a generic, architecture independent way to 
> > execute
> > + * an arbitrary function of type kunit_try_catch_func_t which may bail out 
> > by
> > + * calling kunit_try_catch_throw(). If kunit_try_catch_throw() is called, 
> > @try
> > + * is stopped at the site of invocation and @catch is catch is called.
> > + *
> > + * struct kunit_try_catch provides a generic interface for the 
> > functionality
> > + * needed to implement kunit->abort() which in turn is needed for 
> > implementing
> > + * assertions. Assertions allow stating a precondition for a test 
> > simplifying
> > + * how test cases are written and presented.
> > + *
> > + * Assertions are like expectations, except they abort (call
> > + * kunit_try_catch_throw()) when the specified condition is not met. This 
> > is
> > + * useful when you look at a test case as a logical statement about some 
> > piece
> > + * of code, where assertions are the premises for the test case, and the
> > + * conclusion is a set of predicates, rather expectations, that must all be
> > + * true. If your premises are violated, it does not makes sense to 
> > continue.
> > + */
> > +struct kunit_try_catch {
> > +   /* private: internal use only. */
> > +   struct kunit *test;
> > +   struct completion *try_completion;
> > +   int try_result;
> > +   kunit_try_catch_func_t try;
> > +   kunit_try_catch_func_t catch;
>
> Can these other variables be documented in the kernel doc? And should
> context be marked as 'public'?

Sure, I can document them.

But I don't think context should be public; it should only be accessed
by kunit_try_catch_* functions. context should only be populated by
*_init, and will be passed into *try and *catch when they are called
internally.

> > +   void *context;
> > +};
> > +
> > +void kunit_try_catch_init(struct kunit_try_catch *try_catch,
> > + struct kunit *test,
> > +

Re: [PATCH v12 04/18] kunit: test: add assertion printing library

Quoting Brendan Higgins (2019-08-12 21:27:05)
> On Mon, Aug 12, 2019 at 4:56 PM Brendan Higgins
>  wrote:
> >
> > On Mon, Aug 12, 2019 at 4:46 PM Stephen Boyd  wrote:
> > >
> > > Quoting Brendan Higgins (2019-08-12 11:24:07)
> > > > +#define KUNIT_INIT_FAIL_ASSERT_STRUCT(test, type) {
> > > >\
> > > > +   .assert = KUNIT_INIT_ASSERT_STRUCT(test,
> > > >\
> > > > +  type,
> > > >\
> > > > +  
> > > > kunit_fail_assert_format)   \
> > >
> > > This one got indented one too many times?
> >
> > Not unless I have been using the wrong formatting for multiline
> > macros. You can see this commit applied here:
> > https://kunit.googlesource.com/linux/+/870964da2990920030990dd1ffb647ef408e52df/include/kunit/assert.h#59
> >
> > I have test, type, and kunit_fail_assert_format all column aligned (it
> > just doesn't render nicely in the patch format).
> 
> Disregard that last comment. I just looked at the line immediately
> above your comment and thought it looked correct. Sorry about that
> (you were pointing out that the .assert line looked wrong, correct?).

Yes. .assert is double tabbed?

___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v12 03/18] kunit: test: add string_stream a std::stream like string builder

Quoting Brendan Higgins (2019-08-12 17:41:05)
> On Mon, Aug 12, 2019 at 4:59 PM Stephen Boyd  wrote:
> >
> > > kunit_resource_destroy (respective equivalents to devm_kfree, and
> > > devres_destroy) and use kunit_kfree here?
> > >
> >
> > Yes, or drop the API entirely? Does anything need this functionality?
> 
> Drop the kunit_resource API? I would strongly prefer not to.

No. I mean this API, string_stream_clear(). Does anything use it?

___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 5/5] memremap: provide a not device managed memremap_pages

2019-08-12 Thread Bharata B Rao

On Mon, Aug 12, 2019 at 05:00:12PM +0200, Christoph Hellwig wrote:
> On Mon, Aug 12, 2019 at 08:20:58PM +0530, Bharata B Rao wrote:
> > On Sun, Aug 11, 2019 at 10:12:47AM +0200, Christoph Hellwig wrote:
> > > The kvmppc ultravisor code wants a device private memory pool that is
> > > system wide and not attached to a device.  Instead of faking up one
> > > provide a low-level memremap_pages for it.  Note that this function is
> > > not exported, and doesn't have a cleanup routine associated with it to
> > > discourage use from more driver like users.
> > 
> > The kvmppc secure pages management code will be part of kvm-hv which
> > can be built as module too. So it would require memremap_pages() to be
> > exported.
> > 
> > Additionally, non-dev version of the cleanup routine
> > devm_memremap_pages_release() or equivalent would also be requried.
> > With device being present, put_device() used to take care of this
> > cleanup.
> 
> Oh well.  We can add them fairly easily if we really need to, but I
> tried to avoid that.  Can you try to see if this works non-modular
> for you for now until we hear more feedback from Dan?

Yes, this patchset works non-modular and with kvm-hv as module, it
works with devm_memremap_pages_release() and release_mem_region() in the
cleanup path. The cleanup path will be required in the non-modular
case too for proper recovery from failures.

Regards,
Bharata.

___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] libnvdimm: change disk name of virtio pmem disk

2019-08-12 Thread Pankaj Gupta



Ping.

> 
> This patch adds prefix 'v' in disk name for virtio pmem.
> This differentiates virtio-pmem disks from the pmem disks.
> 
> Signed-off-by: Pankaj Gupta 
> ---
>  drivers/nvdimm/namespace_devs.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvdimm/namespace_devs.c
> b/drivers/nvdimm/namespace_devs.c
> index a16e52251a30..8e5d29266fb0 100644
> --- a/drivers/nvdimm/namespace_devs.c
> +++ b/drivers/nvdimm/namespace_devs.c
> @@ -182,8 +182,12 @@ const char *nvdimm_namespace_disk_name(struct
> nd_namespace_common *ndns,
>   char *name)
>  {
>   struct nd_region *nd_region = to_nd_region(ndns->dev.parent);
> + const char *prefix = "";
>   const char *suffix = NULL;
>  
> + if (!is_nvdimm_sync(nd_region))
> + prefix = "v";
> +
>   if (ndns->claim && is_nd_btt(ndns->claim))
>   suffix = "s";
>  
> @@ -201,7 +205,7 @@ const char *nvdimm_namespace_disk_name(struct
> nd_namespace_common *ndns,
>   sprintf(name, "pmem%d.%d%s", nd_region->id, nsidx,
>   suffix ? suffix : "");
>   else
> - sprintf(name, "pmem%d%s", nd_region->id,
> + sprintf(name, "%spmem%d%s", prefix, nd_region->id,
>   suffix ? suffix : "");
>   } else if (is_namespace_blk(&ndns->dev)) {
>   struct nd_namespace_blk *nsblk;
> --
> 2.20.1
> 
> 
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v12 11/18] kunit: test: add the concept of assertions

Quoting Brendan Higgins (2019-08-12 11:24:14)
> Add support for assertions which are like expectations except the test
> terminates if the assertion is not satisfied.
> 
> The idea with assertions is that you use them to state all the
> preconditions for your test. Logically speaking, these are the premises
> of the test case, so if a premise isn't true, there is no point in
> continuing the test case because there are no conclusions that can be
> drawn without the premises. Whereas, the expectation is the thing you
> are trying to prove. It is not used universally in x-unit style test
> frameworks, but I really like it as a convention.  You could still
> express the idea of a premise using the above idiom, but I think
> KUNIT_ASSERT_* states the intended idea perfectly.
> 
> Signed-off-by: Brendan Higgins 
> Reviewed-by: Greg Kroah-Hartman 
> Reviewed-by: Logan Gunthorpe 

Reviewed-by: Stephen Boyd 

> + * Sets an expectation that the values that @left and @right evaluate to are
> + * not equal. This is semantically equivalent to
> + * KUNIT_ASSERT_TRUE(@test, strcmp((@left), (@right))). See 
> KUNIT_ASSERT_TRUE()
> + * for more information.
> + */
> +#define KUNIT_ASSERT_STRNEQ(test, left, right)   
>  \
> +   KUNIT_BINARY_STR_NE_ASSERTION(test,   
>  \
> + KUNIT_ASSERTION,
>  \
> + left,   
>  \
> + right)
> +
> +#define KUNIT_ASSERT_STRNEQ_MSG(test, left, right, fmt, ...) 
>  \
> +   KUNIT_BINARY_STR_NE_MSG_ASSERTION(test,   
>  \
> + KUNIT_ASSERTION,
>  \
> + left,   
>  \
> + right,  
>  \
> + fmt,
>  \

Same question about tabbing too.

> diff --git a/kunit/test-test.c b/kunit/test-test.c
> index 88f4cdf03db2a..058f3fb37458a 100644
> --- a/kunit/test-test.c
> +++ b/kunit/test-test.c
> @@ -78,11 +78,13 @@ static int kunit_try_catch_test_init(struct kunit *test)
> struct kunit_try_catch_test_context *ctx;
>  
> ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
> +   KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);

Ah ok. Question still stands if kunit_kzalloc() should just have the
assertion on failure.

> test->priv = ctx;
>  
> ctx->try_catch = kunit_kmalloc(test,
>sizeof(*ctx->try_catch),
>GFP_KERNEL);
> +   KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx->try_catch);
>  
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v12 17/18] kernel/sysctl-test: Add null pointer test for sysctl.c:proc_dointvec()

Quoting Brendan Higgins (2019-08-12 11:24:20)
> From: Iurii Zaikin 
> 
> KUnit tests for initialized data behavior of proc_dointvec that is
> explicitly checked in the code. Includes basic parsing tests including
> int min/max overflow.
> 
> Signed-off-by: Iurii Zaikin 
> Signed-off-by: Brendan Higgins 
> Reviewed-by: Greg Kroah-Hartman 
> Reviewed-by: Logan Gunthorpe 
> Acked-by: Luis Chamberlain 
> ---

Reviewed-by: Stephen Boyd 

___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v12 15/18] Documentation: kunit: add documentation for KUnit

Quoting Brendan Higgins (2019-08-12 11:24:18)
> Add documentation for KUnit, the Linux kernel unit testing framework.
> - Add intro and usage guide for KUnit
> - Add API reference
> 
> Signed-off-by: Felix Guo 
> Signed-off-by: Brendan Higgins 
> Cc: Jonathan Corbet 
> Reviewed-by: Greg Kroah-Hartman 
> Reviewed-by: Logan Gunthorpe 
> ---

Reviewed-by: Stephen Boyd 

___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v12 14/18] kunit: defconfig: add defconfigs for building KUnit tests

Quoting Brendan Higgins (2019-08-12 11:24:17)
> diff --git a/arch/um/configs/kunit_defconfig b/arch/um/configs/kunit_defconfig
> new file mode 100644
> index 0..bfe49689038f1
> --- /dev/null
> +++ b/arch/um/configs/kunit_defconfig
> @@ -0,0 +1,8 @@
> +CONFIG_OF=y
> +CONFIG_OF_UNITTEST=y
> +CONFIG_OF_OVERLAY=y
> +CONFIG_I2C=y
> +CONFIG_I2C_MUX=y
> +CONFIG_KUNIT=y
> +CONFIG_KUNIT_TEST=y
> +CONFIG_KUNIT_EXAMPLE_TEST=y
> diff --git a/tools/testing/kunit/configs/all_tests.config 
> b/tools/testing/kunit/configs/all_tests.config
> new file mode 100644
> index 0..bfe49689038f1
> --- /dev/null
> +++ b/tools/testing/kunit/configs/all_tests.config
> @@ -0,0 +1,8 @@
> +CONFIG_OF=y
> +CONFIG_OF_UNITTEST=y
> +CONFIG_OF_OVERLAY=y
> +CONFIG_I2C=y
> +CONFIG_I2C_MUX=y

Are these above config options necessary? I don't think they're part of
the patch series anymore so it looks odd to enable the OF unittests and
i2c configs.

> +CONFIG_KUNIT=y
> +CONFIG_KUNIT_TEST=y
> +CONFIG_KUNIT_EXAMPLE_TEST=y
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v12 12/18] kunit: test: add tests for KUnit managed resources

Quoting Brendan Higgins (2019-08-12 11:24:15)
> +
> +static int kunit_resource_test_init(struct kunit *test)
> +{
> +   struct kunit_test_resource_context *ctx =
> +   kzalloc(sizeof(*ctx), GFP_KERNEL);
> +
> +   if (!ctx)
> +   return -ENOMEM;

Should this use the test assertion logic to make sure that it's not
failing allocations during init? BTW, maybe kunit allocation APIs should
fail the test if they fail to allocate in general. Unless we're unit
testing failure to allocate problems.

> +
> +   test->priv = ctx;
> +
> +   kunit_init_test(&ctx->test, "test_test_context");
> +
> +   return 0;
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v12 04/18] kunit: test: add assertion printing library

On Mon, Aug 12, 2019 at 4:56 PM Brendan Higgins
 wrote:
>
> On Mon, Aug 12, 2019 at 4:46 PM Stephen Boyd  wrote:
> >
> > Quoting Brendan Higgins (2019-08-12 11:24:07)
> > > Add `struct kunit_assert` and friends which provide a structured way to
> > > capture data from an expectation or an assertion (introduced later in
> > > the series) so that it may be printed out in the event of a failure.
> > >
> > > Signed-off-by: Brendan Higgins 
> > > ---
> >
> > Reviewed-by: Stephen Boyd 
> >
> > Just some minor nits below
> >
> > > diff --git a/include/kunit/assert.h b/include/kunit/assert.h
> > > new file mode 100644
> > > index 0..55f1b88b0cb4d
> > > --- /dev/null
> > > +++ b/include/kunit/assert.h
> > > @@ -0,0 +1,183 @@
> > [...]
> > > +   struct string_stream *stream);
> > > +
> > > +struct kunit_fail_assert {
> > > +   struct kunit_assert assert;
> > > +};
> > > +
> > > +void kunit_fail_assert_format(const struct kunit_assert *assert,
> > > + struct string_stream *stream);
> > > +
> > > +#define KUNIT_INIT_FAIL_ASSERT_STRUCT(test, type) {  
> > >  \
> > > +   .assert = KUNIT_INIT_ASSERT_STRUCT(test,  
> > >  \
> > > +  type,  
> > >  \
> > > +  
> > > kunit_fail_assert_format)   \
> >
> > This one got indented one too many times?
>
> Not unless I have been using the wrong formatting for multiline
> macros. You can see this commit applied here:
> https://kunit.googlesource.com/linux/+/870964da2990920030990dd1ffb647ef408e52df/include/kunit/assert.h#59
>
> I have test, type, and kunit_fail_assert_format all column aligned (it
> just doesn't render nicely in the patch format).

Disregard that last comment. I just looked at the line immediately
above your comment and thought it looked correct. Sorry about that
(you were pointing out that the .assert line looked wrong, correct?).

> > > +}
> > > +
> > > +struct kunit_unary_assert {
> > > +   struct kunit_assert assert;
> > > +   const char *condition;
> > > +   bool expected_true;
> > > +};
> > > +
> > > +void kunit_unary_assert_format(const struct kunit_assert *assert,
> > > +  struct string_stream *stream);
> > > +
> > [...]
> > > +#define KUNIT_INIT_BINARY_STR_ASSERT_STRUCT(test,
> > >  \
> > > +   type, 
> > >  \
> > > +   op_str,   
> > >  \
> > > +   left_str, 
> > >  \
> > > +   left_val, 
> > >  \
> > > +   right_str,
> > >  \
> > > +   right_val) {  
> > >  \
> > > +   .assert = KUNIT_INIT_ASSERT_STRUCT(test,  
> > >  \
> > > +  type,  
> > >  \
> > > +  
> > > kunit_binary_str_assert_format),\
> > > +   .operation = op_str,  
> > >  \
> > > +   .left_text = left_str,
> > >  \
> > > +   .left_value = left_val,   
> > >  \
> > > +   .right_text = right_str,  
> > >  \
> > > +   .right_value = right_val  
> > >  \
> > > +}
> >
> > It would be nice to have kernel doc on these macros so we know how to
> > use them.
>
> Sounds good. Will fix.
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v12 10/18] kunit: test: add tests for kunit test abort

Quoting Brendan Higgins (2019-08-12 11:24:13)
> +
> +static int kunit_try_catch_test_init(struct kunit *test)
> +{
> +   struct kunit_try_catch_test_context *ctx;
> +
> +   ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);

Can this fail? Should return -ENOMEM in that case?

> +   test->priv = ctx;
> +
> +   ctx->try_catch = kunit_kmalloc(test,
> +  sizeof(*ctx->try_catch),
> +  GFP_KERNEL);
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v12 09/18] kunit: test: add support for test abort

Quoting Brendan Higgins (2019-08-12 11:24:12)
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index 2625bcfeb19ac..93381f841e09f 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -13,6 +13,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  struct kunit_resource;
>  
> @@ -167,6 +168,7 @@ struct kunit {
>  
> /* private: internal use only. */
> const char *name; /* Read only after initialization! */
> +   struct kunit_try_catch try_catch;
> /*
>  * success starts as true, and may only be set to false during a test
>  * case; thus, it is safe to update this across multiple threads using
> @@ -176,6 +178,11 @@ struct kunit {
>  */
> bool success; /* Read only after test_case finishes! */
> spinlock_t lock; /* Gaurds all mutable test state. */
> +   /*
> +* death_test may be both set and unset from multiple threads in a 
> test
> +* case.
> +*/
> +   bool death_test; /* Protected by lock. */
> /*
>  * Because resources is a list that may be updated multiple times 
> (with
>  * new resources) from any thread associated with a test case, we must
> @@ -184,6 +191,13 @@ struct kunit {
> struct list_head resources; /* Protected by lock. */
>  };
>  
> +static inline void kunit_set_death_test(struct kunit *test, bool death_test)
> +{
> +   spin_lock(&test->lock);
> +   test->death_test = death_test;
> +   spin_unlock(&test->lock);
> +}

These getters and setters are using spinlocks again. It doesn't make any
sense. It probably needs a rework like was done for the other bool
member, success.

> +
>  void kunit_init_test(struct kunit *test, const char *name);
>  
>  int kunit_run_tests(struct kunit_suite *suite);
> diff --git a/include/kunit/try-catch.h b/include/kunit/try-catch.h
> new file mode 100644
> index 0..8a414a9af0b64
> --- /dev/null
> +++ b/include/kunit/try-catch.h
> @@ -0,0 +1,69 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * An API to allow a function, that may fail, to be executed, and recover in 
> a
> + * controlled manner.
> + *
> + * Copyright (C) 2019, Google LLC.
> + * Author: Brendan Higgins 
> + */
> +
> +#ifndef _KUNIT_TRY_CATCH_H
> +#define _KUNIT_TRY_CATCH_H
> +
> +#include 
> +
> +typedef void (*kunit_try_catch_func_t)(void *);
> +
> +struct kunit;

Forward declare struct completion?

> +
> +/*
> + * struct kunit_try_catch - provides a generic way to run code which might 
> fail.
> + * @context: used to pass user data to the try and catch functions.
> + *
> + * kunit_try_catch provides a generic, architecture independent way to 
> execute
> + * an arbitrary function of type kunit_try_catch_func_t which may bail out by
> + * calling kunit_try_catch_throw(). If kunit_try_catch_throw() is called, 
> @try
> + * is stopped at the site of invocation and @catch is catch is called.
> + *
> + * struct kunit_try_catch provides a generic interface for the functionality
> + * needed to implement kunit->abort() which in turn is needed for 
> implementing
> + * assertions. Assertions allow stating a precondition for a test simplifying
> + * how test cases are written and presented.
> + *
> + * Assertions are like expectations, except they abort (call
> + * kunit_try_catch_throw()) when the specified condition is not met. This is
> + * useful when you look at a test case as a logical statement about some 
> piece
> + * of code, where assertions are the premises for the test case, and the
> + * conclusion is a set of predicates, rather expectations, that must all be
> + * true. If your premises are violated, it does not makes sense to continue.
> + */
> +struct kunit_try_catch {
> +   /* private: internal use only. */
> +   struct kunit *test;
> +   struct completion *try_completion;
> +   int try_result;
> +   kunit_try_catch_func_t try;
> +   kunit_try_catch_func_t catch;

Can these other variables be documented in the kernel doc? And should
context be marked as 'public'?

> +   void *context;
> +};
> +
> +void kunit_try_catch_init(struct kunit_try_catch *try_catch,
> + struct kunit *test,
> + kunit_try_catch_func_t try,
> + kunit_try_catch_func_t catch);
> +
> +void kunit_try_catch_run(struct kunit_try_catch *try_catch, void *context);
> +
> +void __noreturn kunit_try_catch_throw(struct kunit_try_catch *try_catch);
> +
> +static inline int kunit_try_catch_get_result(struct kunit_try_catch 
> *try_catch)
> +{
> +   return try_catch->try_result;
> +}
> +
> +/*
> + * Exposed for testing only.

Ugh that's sad. I hope we don't expose more functions just for testing
in other cases.

> + */
> +void kunit_generic_try_catch_init(struct kunit_try_catch *try_catch);
> +
> +#endif /* _KUNIT_TRY_CATCH_H */
> diff --git a/kunit/test.c b/kunit/test.c
> index e5080a2c6b29c..995cb53fe4ee9 100644
> --- a/kunit/t

Re: [PATCH v12 03/18] kunit: test: add string_stream a std::stream like string builder

On Mon, Aug 12, 2019 at 4:59 PM Stephen Boyd  wrote:
>
> Quoting Brendan Higgins (2019-08-12 16:33:36)
> > On Mon, Aug 12, 2019 at 03:55:19PM -0700, Stephen Boyd wrote:
> > > Quoting Brendan Higgins (2019-08-12 11:24:06)
> > > > +void string_stream_clear(struct string_stream *stream)
> > > > +{
> > > > +   struct string_stream_fragment *frag_container, 
> > > > *frag_container_safe;
> > > > +
> > > > +   spin_lock(&stream->lock);
> > > > +   list_for_each_entry_safe(frag_container,
> > > > +frag_container_safe,
> > > > +&stream->fragments,
> > > > +node) {
> > > > +   list_del(&frag_container->node);
> > >
> > > Shouldn't we free the allocation here? Otherwise, if some test is going
> > > to add, add, clear, add, it's going to leak until the test is over?
> >
> > So basically this means I should add a kunit_kfree and
> > kunit_resource_destroy (respective equivalents to devm_kfree, and
> > devres_destroy) and use kunit_kfree here?
> >
>
> Yes, or drop the API entirely? Does anything need this functionality?

Drop the kunit_resource API? I would strongly prefer not to.
string_stream uses it; the expectation stuff uses it via string
stream; some of the tests in this patchset allocate memory as part of
the test setup that uses it. The intention is that we would provide a
kunit_res_* version of many (hopefully eventually most) common
resources required by tests and it would be used in the same way that
the devm_* stuff is.

Nevertheless, I am fine adding the kunit_resource_destroy, etc. I just
wanted to make sure I understood what you were asking.
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v12 05/18] kunit: test: add the concept of expectations

On Mon, Aug 12, 2019 at 04:57:00PM -0700, Stephen Boyd wrote:
> Quoting Brendan Higgins (2019-08-12 11:24:08)
> > Add support for expectations, which allow properties to be specified and
> > then verified in tests.
> > 
> > Signed-off-by: Brendan Higgins 
> > Reviewed-by: Greg Kroah-Hartman 
> > Reviewed-by: Logan Gunthorpe 
> 
> Reviewed-by: Stephen Boyd 
> 
> Just some minor nits again.
> 
> > diff --git a/include/kunit/test.h b/include/kunit/test.h
> > index d0bf112910caf..2625bcfeb19ac 100644
> > --- a/include/kunit/test.h
> > +++ b/include/kunit/test.h
> > @@ -9,8 +9,10 @@
> >  #ifndef _KUNIT_TEST_H
> >  #define _KUNIT_TEST_H
> >  
> > +#include 
> >  #include 
> >  #include 
> > +#include 
> 
> Can you alphabet sort these?

Sure. Will fix.

> >  
> >  struct kunit_resource;
> >  
> > @@ -319,4 +321,845 @@ void __printf(3, 4) kunit_printk(const char *level,
> >  #define kunit_err(test, fmt, ...) \
> > kunit_printk(KERN_ERR, test, fmt, ##__VA_ARGS__)
> >  
> > +/*
> > + * Generates a compile-time warning in case of comparing incompatible 
> > types.
> > + */
> > +#define __kunit_typecheck(lhs, rhs) \
> > +   ((void) __typecheck(lhs, rhs))
> 
> Is there a reason why this can't be inlined and the __kunit_typecheck()
> macro can't be removed?

No real reason anymore. I used it in multiple places before and we
weren't sure if we wanted to stick with the warnings that __typecheck
produces long term, but now that it is only used in one place, I guess
that doesn't make sense anymore. Will fix.

> > +
> > +/**
> > + * KUNIT_SUCCEED() - A no-op expectation. Only exists for code clarity.
> > + * @test: The test context object.
> [...]
> > + * @condition: an arbitrary boolean expression. The test fails when this 
> > does
> > + * not evaluate to true.
> > + *
> > + * This and expectations of the form `KUNIT_EXPECT_*` will cause the test 
> > case
> > + * to fail when the specified condition is not met; however, it will not 
> > prevent
> > + * the test case from continuing to run; this is otherwise known as an
> > + * *expectation failure*.
> > + */
> > +#define KUNIT_EXPECT_TRUE(test, condition) \
> > +   KUNIT_TRUE_ASSERTION(test, KUNIT_EXPECTATION, condition)
> 
> A lot of these macros seem double indented.

In a case you pointed out in the preceding patch, I was just keeping the
arguments column aligned.

In this case I am just indenting two tabs for a line continuation. I
thought I found other instances in the kernel that did this early on
(and that's also what the Linux kernel vim plugin wanted me to do).
After a couple of spot checks, it seems like one tab for this kind of
line continuation seems more common. I personally don't feel strongly
about any particular version. I just want to know now what the correct
indentation is for macros before I go through and change them all.

I think there are three cases:

#define macro0(param0, param1) \
a_really_long_macro(...)

In this first case, I use two tabs for the first indent, I think you are
telling me this should be one tab.

#define macro1(param0, param1) {   \
statement_in_a_block0; \
statement_in_a_block1; \
...\
}

In this case, every line is in a block and is indented as it would be in
a function body. I think you are okay with this, and now that I am
thinking about it, what I think you are proposing for macro0 will make
these two cases more consistent.

#define macro2(param0, \
   param1, \
   param2, \
   param3, \
   ...,\
   paramn) ... \

In this last case, the body would be indented as in macro0, or macro1,
but the parameters passed into the macro are column aligned, consistent
with one of the acceptable ways of formatting function parameters that
don't fit on a single line.

In all cases, I put 1 space in between the closing parameter paren and
the line continuation `\`, if only one `\` is needed. Otherwise, I align
all the `\s` to the 80th column. Is this okay, or would you prefer that
I align them all to the 80th column, or something else?

> > +
> > +#define KUNIT_EXPECT_TRUE_MSG(test, condition, fmt, ...)   
> >\
> > +   KUNIT_TRUE_MSG_ASSERTION(test,  
> >\
> > +KUNIT_EXPECTATION, 
> >\
> > +condition,

Re: [PATCH v12 04/18] kunit: test: add assertion printing library

On Mon, Aug 12, 2019 at 4:46 PM Stephen Boyd  wrote:
>
> Quoting Brendan Higgins (2019-08-12 11:24:07)
> > Add `struct kunit_assert` and friends which provide a structured way to
> > capture data from an expectation or an assertion (introduced later in
> > the series) so that it may be printed out in the event of a failure.
> >
> > Signed-off-by: Brendan Higgins 
> > ---
>
> Reviewed-by: Stephen Boyd 
>
> Just some minor nits below
>
> > diff --git a/include/kunit/assert.h b/include/kunit/assert.h
> > new file mode 100644
> > index 0..55f1b88b0cb4d
> > --- /dev/null
> > +++ b/include/kunit/assert.h
> > @@ -0,0 +1,183 @@
> [...]
> > +   struct string_stream *stream);
> > +
> > +struct kunit_fail_assert {
> > +   struct kunit_assert assert;
> > +};
> > +
> > +void kunit_fail_assert_format(const struct kunit_assert *assert,
> > + struct string_stream *stream);
> > +
> > +#define KUNIT_INIT_FAIL_ASSERT_STRUCT(test, type) {
> >\
> > +   .assert = KUNIT_INIT_ASSERT_STRUCT(test,
> >\
> > +  type,
> >\
> > +  
> > kunit_fail_assert_format)   \
>
> This one got indented one too many times?

Not unless I have been using the wrong formatting for multiline
macros. You can see this commit applied here:
https://kunit.googlesource.com/linux/+/870964da2990920030990dd1ffb647ef408e52df/include/kunit/assert.h#59

I have test, type, and kunit_fail_assert_format all column aligned (it
just doesn't render nicely in the patch format).

> > +}
> > +
> > +struct kunit_unary_assert {
> > +   struct kunit_assert assert;
> > +   const char *condition;
> > +   bool expected_true;
> > +};
> > +
> > +void kunit_unary_assert_format(const struct kunit_assert *assert,
> > +  struct string_stream *stream);
> > +
> [...]
> > +#define KUNIT_INIT_BINARY_STR_ASSERT_STRUCT(test,  
> >\
> > +   type,   
> >\
> > +   op_str, 
> >\
> > +   left_str,   
> >\
> > +   left_val,   
> >\
> > +   right_str,  
> >\
> > +   right_val) {
> >\
> > +   .assert = KUNIT_INIT_ASSERT_STRUCT(test,
> >\
> > +  type,
> >\
> > +  kunit_binary_str_assert_format), 
> >\
> > +   .operation = op_str,
> >\
> > +   .left_text = left_str,  
> >\
> > +   .left_value = left_val, 
> >\
> > +   .right_text = right_str,
> >\
> > +   .right_value = right_val
> >\
> > +}
>
> It would be nice to have kernel doc on these macros so we know how to
> use them.

Sounds good. Will fix.
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v12 07/18] kunit: test: add initial tests

Quoting Brendan Higgins (2019-08-12 11:24:10)
> Add a test for string stream along with a simpler example.
> 
> Signed-off-by: Brendan Higgins 
> Reviewed-by: Greg Kroah-Hartman 
> Reviewed-by: Logan Gunthorpe 
> ---

Reviewed-by: Stephen Boyd 

___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v12 05/18] kunit: test: add the concept of expectations

Quoting Brendan Higgins (2019-08-12 11:24:08)
> Add support for expectations, which allow properties to be specified and
> then verified in tests.
> 
> Signed-off-by: Brendan Higgins 
> Reviewed-by: Greg Kroah-Hartman 
> Reviewed-by: Logan Gunthorpe 

Reviewed-by: Stephen Boyd 

Just some minor nits again.

> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index d0bf112910caf..2625bcfeb19ac 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -9,8 +9,10 @@
>  #ifndef _KUNIT_TEST_H
>  #define _KUNIT_TEST_H
>  
> +#include 
>  #include 
>  #include 
> +#include 

Can you alphabet sort these?

>  
>  struct kunit_resource;
>  
> @@ -319,4 +321,845 @@ void __printf(3, 4) kunit_printk(const char *level,
>  #define kunit_err(test, fmt, ...) \
> kunit_printk(KERN_ERR, test, fmt, ##__VA_ARGS__)
>  
> +/*
> + * Generates a compile-time warning in case of comparing incompatible types.
> + */
> +#define __kunit_typecheck(lhs, rhs) \
> +   ((void) __typecheck(lhs, rhs))

Is there a reason why this can't be inlined and the __kunit_typecheck()
macro can't be removed?

> +
> +/**
> + * KUNIT_SUCCEED() - A no-op expectation. Only exists for code clarity.
> + * @test: The test context object.
[...]
> + * @condition: an arbitrary boolean expression. The test fails when this does
> + * not evaluate to true.
> + *
> + * This and expectations of the form `KUNIT_EXPECT_*` will cause the test 
> case
> + * to fail when the specified condition is not met; however, it will not 
> prevent
> + * the test case from continuing to run; this is otherwise known as an
> + * *expectation failure*.
> + */
> +#define KUNIT_EXPECT_TRUE(test, condition) \
> +   KUNIT_TRUE_ASSERTION(test, KUNIT_EXPECTATION, condition)

A lot of these macros seem double indented.

> +
> +#define KUNIT_EXPECT_TRUE_MSG(test, condition, fmt, ...) 
>  \
> +   KUNIT_TRUE_MSG_ASSERTION(test,
>  \
> +KUNIT_EXPECTATION,   
>  \
> +condition,   
>  \
> +fmt, 
>  \
> +##__VA_ARGS__)
> +
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v12 03/18] kunit: test: add string_stream a std::stream like string builder

Quoting Brendan Higgins (2019-08-12 16:33:36)
> On Mon, Aug 12, 2019 at 03:55:19PM -0700, Stephen Boyd wrote:
> > Quoting Brendan Higgins (2019-08-12 11:24:06)
> > > +void string_stream_clear(struct string_stream *stream)
> > > +{
> > > +   struct string_stream_fragment *frag_container, 
> > > *frag_container_safe;
> > > +
> > > +   spin_lock(&stream->lock);
> > > +   list_for_each_entry_safe(frag_container,
> > > +frag_container_safe,
> > > +&stream->fragments,
> > > +node) {
> > > +   list_del(&frag_container->node);
> > 
> > Shouldn't we free the allocation here? Otherwise, if some test is going
> > to add, add, clear, add, it's going to leak until the test is over?
> 
> So basically this means I should add a kunit_kfree and
> kunit_resource_destroy (respective equivalents to devm_kfree, and
> devres_destroy) and use kunit_kfree here?
> 

Yes, or drop the API entirely? Does anything need this functionality?

___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v12 04/18] kunit: test: add assertion printing library

Quoting Brendan Higgins (2019-08-12 11:24:07)
> Add `struct kunit_assert` and friends which provide a structured way to
> capture data from an expectation or an assertion (introduced later in
> the series) so that it may be printed out in the event of a failure.
> 
> Signed-off-by: Brendan Higgins 
> ---

Reviewed-by: Stephen Boyd 

Just some minor nits below

> diff --git a/include/kunit/assert.h b/include/kunit/assert.h
> new file mode 100644
> index 0..55f1b88b0cb4d
> --- /dev/null
> +++ b/include/kunit/assert.h
> @@ -0,0 +1,183 @@
[...]
> +   struct string_stream *stream);
> +
> +struct kunit_fail_assert {
> +   struct kunit_assert assert;
> +};
> +
> +void kunit_fail_assert_format(const struct kunit_assert *assert,
> + struct string_stream *stream);
> +
> +#define KUNIT_INIT_FAIL_ASSERT_STRUCT(test, type) {  
>  \
> +   .assert = KUNIT_INIT_ASSERT_STRUCT(test,  
>  \
> +  type,  
>  \
> +  kunit_fail_assert_format)  
>  \

This one got indented one too many times?

> +}
> +
> +struct kunit_unary_assert {
> +   struct kunit_assert assert;
> +   const char *condition;
> +   bool expected_true;
> +};
> +
> +void kunit_unary_assert_format(const struct kunit_assert *assert,
> +  struct string_stream *stream);
> +
[...]
> +#define KUNIT_INIT_BINARY_STR_ASSERT_STRUCT(test,
>  \
> +   type, 
>  \
> +   op_str,   
>  \
> +   left_str, 
>  \
> +   left_val, 
>  \
> +   right_str,
>  \
> +   right_val) {  
>  \
> +   .assert = KUNIT_INIT_ASSERT_STRUCT(test,  
>  \
> +  type,  
>  \
> +  kunit_binary_str_assert_format),   
>  \
> +   .operation = op_str,  
>  \
> +   .left_text = left_str,
>  \
> +   .left_value = left_val,   
>  \
> +   .right_text = right_str,  
>  \
> +   .right_value = right_val  
>  \
> +}

It would be nice to have kernel doc on these macros so we know how to
use them.

___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v12 03/18] kunit: test: add string_stream a std::stream like string builder

On Mon, Aug 12, 2019 at 03:55:19PM -0700, Stephen Boyd wrote:
> Quoting Brendan Higgins (2019-08-12 11:24:06)
> > +void string_stream_clear(struct string_stream *stream)
> > +{
> > +   struct string_stream_fragment *frag_container, *frag_container_safe;
> > +
> > +   spin_lock(&stream->lock);
> > +   list_for_each_entry_safe(frag_container,
> > +frag_container_safe,
> > +&stream->fragments,
> > +node) {
> > +   list_del(&frag_container->node);
> 
> Shouldn't we free the allocation here? Otherwise, if some test is going
> to add, add, clear, add, it's going to leak until the test is over?

So basically this means I should add a kunit_kfree and
kunit_resource_destroy (respective equivalents to devm_kfree, and
devres_destroy) and use kunit_kfree here?

> > +   }
> > +   stream->length = 0;
> > +   spin_unlock(&stream->lock);
> > +}
> > +
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v12 03/18] kunit: test: add string_stream a std::stream like string builder

Quoting Brendan Higgins (2019-08-12 11:24:06)
> +void string_stream_clear(struct string_stream *stream)
> +{
> +   struct string_stream_fragment *frag_container, *frag_container_safe;
> +
> +   spin_lock(&stream->lock);
> +   list_for_each_entry_safe(frag_container,
> +frag_container_safe,
> +&stream->fragments,
> +node) {
> +   list_del(&frag_container->node);

Shouldn't we free the allocation here? Otherwise, if some test is going
to add, add, clear, add, it's going to leak until the test is over?

> +   }
> +   stream->length = 0;
> +   spin_unlock(&stream->lock);
> +}
> +
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v12 02/18] kunit: test: add test resource management API

Quoting Brendan Higgins (2019-08-12 11:24:05)
> 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 
> Reviewed-by: Greg Kroah-Hartman 
> Reviewed-by: Logan Gunthorpe 
> ---

Reviewed-by: Stephen Boyd 

___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [RFC PATCH v2 15/19] mm/gup: Introduce vaddr_pin_pages()

On Mon, Aug 12, 2019 at 09:28:14AM -0300, Jason Gunthorpe wrote:
> On Fri, Aug 09, 2019 at 03:58:29PM -0700, ira.we...@intel.com wrote:
> > From: Ira Weiny 
> > 
> > The addition of FOLL_LONGTERM has taken on additional meaning for CMA
> > pages.
> > 
> > In addition subsystems such as RDMA require new information to be passed
> > to the GUP interface to track file owning information.  As such a simple
> > FOLL_LONGTERM flag is no longer sufficient for these users to pin pages.
> > 
> > Introduce a new GUP like call which takes the newly introduced vaddr_pin
> > information.  Failure to pass the vaddr_pin object back to a vaddr_put*
> > call will result in a failure if pins were created on files during the
> > pin operation.
> 
> Is this a 'vaddr' in the traditional sense, ie does it work with
> something returned by valloc?

...or malloc in user space, yes.  I think the idea is that it is a user virtual
address.

> 
> Maybe another name would be better?

Maybe, the name I had was way worse...  So I'm not even going to admit to it...

;-)

So I'm open to suggestions.  Jan gave me this one, so I figured it was safer to
suggest it...

:-D

> 
> I also wish GUP like functions took in a 'void __user *' instead of
> the unsigned long to make this clear :\

Not a bad idea.  But I only see a couple of call sites who actually use a 'void
__user *' to pass into GUP...  :-/

For RDMA the address is _never_ a 'void __user *' AFAICS.

For the new API, it may be tractable to force users to cast to 'void __user *'
but it is not going to provide any type safety.

But it is easy to change in this series.

What do others think?

Ira

> 
> Jason
> 
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v3] ndctl, check: Ensure mmap of BTT sections work with 64K page-sizes

Vaibhav Jain  writes:

> On PPC64 which uses a 64K page-size, ndtl-check command fails on a BTT
> namespace with following error:
>
> $ sudo ndctl check-namespace namespace0.0 -vv
>   namespace0.0: namespace_check: checking namespace0.0
>   namespace0.0: btt_discover_arenas: found 1 BTT arena
>   namespace0.0: btt_create_mappings: mmap arena[0].info [sz = 0x1000, off = 
> 0x1000] failed: Invalid argument
>   error checking namespaces: Invalid argument
>   checked 0 namespaces
>
> An error happens when btt_create_mappings() tries to mmap the sections
> of the BTT device which are usually 4K offset aligned. However the
> mmap() syscall expects the 'offset' argument to be in multiples of
> page-size, hence it returns EINVAL causing the btt_create_mappings()
> to error out.
>
> As a fix for the issue this patch proposes addition of two new
> functions to 'check.c' namely btt_mmap/btt_unmap that can be used to
> map/unmap parts of BTT device to ndctl process address-space. The
> functions tweaks the requested 'offset' argument to mmap() ensuring
> that its page-size aligned and then fix-ups the returned pointer such
> that it points to the requested offset within mmapped region.
>
> With these newly introduced functions the patch updates the call-sites
> in 'check.c' to use these functions instead of mmap/unmap syscalls.
> Also since btt_mmap returns NULL if mmap operation fails, the
> error check call-sites can be made against NULL instead of MAP_FAILED.
>
> Reported-by: Harish Sriram 
> Signed-off-by: Vaibhav Jain 
> ---
> Changelog:
> v3:
> * Fixed a log string that was splitted across multiple lines [Vishal]
>
> v2:
> * Updated patch description to include canonical email address of bug
>   reporter. [Vishal]
> * Improved the comment describing function btt_mmap() in 'check.c'
>   [Vishal]
> * Simplified the argument list for btt_mmap() to just accept bttc,
>   length and offset. Other arguments for mmap() are derived from these
>   args. [Vishal]
> * Renamed 'shift' variable in btt_mmap()/unmap() to 'page_offset'
>   [Vishal]
> * Improved the dbg() messages logged inside
>   btt_mmap()/unmap(). [Vishal]
> * Return NULL from btt_mmap() in case of an error. [Vishal]
> * Changed the all sites to btt_mmap() to perform error check against
>   NULL return value instead of MAP_FAILED. [Vishal]
> ---
>  ndctl/check.c | 93 +--
>  1 file changed, 67 insertions(+), 26 deletions(-)
>
> diff --git a/ndctl/check.c b/ndctl/check.c
> index 8a7125053865..5969012eba84 100644
> --- a/ndctl/check.c
> +++ b/ndctl/check.c
> @@ -907,59 +907,100 @@ static int btt_discover_arenas(struct btt_chk *bttc)
>   return ret;
>  }
>  
> -static int btt_create_mappings(struct btt_chk *bttc)
> +/*
> + * Wrap call to mmap(2) to work with btt device offsets that are not aligned
> + * to system page boundary. It works by rounding down the requested offset
> + * to sys_page_size when calling mmap(2) and then returning a fixed-up 
> pointer
> + * to the correct offset in the mmaped region.
> + */
> +static void *btt_mmap(struct btt_chk *bttc, size_t length, off_t offset)
>  {
> - struct arena_info *a;
> - int mmap_flags;
> - int i;
> + off_t page_offset;
> + int prot_flags;
> + uint8_t *addr;
>  
>   if (!bttc->opts->repair)
> - mmap_flags = PROT_READ;
> + prot_flags = PROT_READ;
>   else
> - mmap_flags = PROT_READ|PROT_WRITE;
> + prot_flags = PROT_READ|PROT_WRITE;
> +
> + /* Calculate the page_offset from the system page boundary */
> + page_offset = offset - rounddown(offset, bttc->sys_page_size);
> +
> + /* Update the offset and length with the page_offset calculated above */
> + offset -= page_offset;
> + length += page_offset;

Don't you have to ensure that the length is also a multiple of the
system page size?

-Jeff

> +
> + addr = mmap(NULL, length, prot_flags, MAP_SHARED, bttc->fd, offset);
> +
> + /* If needed fixup the return pointer to correct offset requested */
> + if (addr != MAP_FAILED)
> + addr += page_offset;
> +
> + dbg(bttc, "addr = %p, length = %#lx, offset = %#lx, page_offset = 
> %#lx\n",
> + (void *) addr, length, offset, page_offset);
> +
> + return addr == MAP_FAILED ? NULL : addr;
> +}
> +
> +static void btt_unmap(struct btt_chk *bttc, void *ptr, size_t length)
> +{
> + off_t page_offset;
> + uintptr_t addr = (uintptr_t) ptr;
> +
> + /* Calculate the page_offset from system page boundary */
> + page_offset = addr - rounddown(addr, bttc->sys_page_size);
> +
> + addr -= page_offset;
> + length += page_offset;
> +
> + munmap((void *) addr, length);
> + dbg(bttc, "addr = %p, length = %#lx, page_offset = %#lx\n",
> + (void *) addr, length, page_offset);
> +}
> +
> +static int btt_create_mappings(struct btt_chk *bttc)
> +{
> + struct arena_info *a;
> + int i;
>  
>   for (i = 0; i < btt

Re: [ndctl PATCH] daxctl/test: Skip daxctl-devices.sh on older kernels

Dan Williams  writes:

> On Mon, Aug 12, 2019 at 1:45 PM Jeff Moyer  wrote:
>>
>> Dan Williams  writes:
>>
>> > If the 'kmem' module is missing skip the test to support running the
>> > unit tests on older -stable kernels pre-v5.1.
>> >
>> > Signed-off-by: Dan Williams 
>>
>> Note that the kernel module could also just not be configured.
>
> Yes, but then the test would still report "SKIP" and the developer
> could take a deeper look if they expected to see all "PASS".

Right, I see this as a good thing.  I meant to point out that your
commit message was a bit misleading.

>> > ---
>> >  test/daxctl-devices.sh |7 +++
>> >  1 file changed, 7 insertions(+)
>> >
>> > diff --git a/test/daxctl-devices.sh b/test/daxctl-devices.sh
>> > index 04f53f7b13ab..4102fb6990ae 100755
>> > --- a/test/daxctl-devices.sh
>> > +++ b/test/daxctl-devices.sh
>> > @@ -18,6 +18,13 @@ find_testdev()
>> >  {
>> >   local rc=77
>> >
>> > + # The kmem driver is needed to change the device mode, only
>> > + # kernels >= v5.1 might have it available. Skip if not.
>> > + if ! modinfo kmem; then
>> > + "Unable to find kmem module"
>>
>> I think you need a printf, there.
>
> Whoops, yes.
>
>> Also, do you want the modinfo output in the test log?
>
> Don't think it matters either way since it all ends up in the log.

It's tough to say whether it adds value, so sure, leave it in.

Cheers,
Jeff
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] mm/memremap: Fix reuse of pgmap instances with internal references

Dan Williams  writes:

> On Mon, Aug 12, 2019 at 8:51 AM Jeff Moyer  wrote:
>>
>> Dan Williams  writes:
>>
>> > Currently, attempts to shutdown and re-enable a device-dax instance
>> > trigger:
>>
>> What does "shutdown and re-enable" translate to?  If I disable and
>> re-enable a device-dax namespace, I don't see this behavior.
>
> I was not seeing this either until I made sure I was in 'bus" device model 
> mode.
>
> # cat /etc/modprobe.d/daxctl.conf
> blacklist dax_pmem_compat
> alias nd:t7* dax_pmem
>
> # make TESTS="daxctl-devices.sh" check -j 40 2>out
>
> # dmesg | grep WARN.*devm
> [  225.588651] WARNING: CPU: 10 PID: 9103 at mm/memremap.c:211
> devm_memremap_pages+0x234/0x850
> [  225.679828] WARNING: CPU: 10 PID: 9103 at mm/memremap.c:211
> devm_memremap_pages+0x234/0x850

Ah, you see this when reconfiguring the device.  So, the lifetime of the
pgmap is tied to the character device, which doesn't get torn down.  The
fix looks good to me, and tests out fine.

Reviewed-by: Jeff Moyer 
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [ndctl PATCH] daxctl/test: Skip daxctl-devices.sh on older kernels

2019-08-12 Thread Dan Williams

On Mon, Aug 12, 2019 at 1:45 PM Jeff Moyer  wrote:
>
> Dan Williams  writes:
>
> > If the 'kmem' module is missing skip the test to support running the
> > unit tests on older -stable kernels pre-v5.1.
> >
> > Signed-off-by: Dan Williams 
>
> Note that the kernel module could also just not be configured.

Yes, but then the test would still report "SKIP" and the developer
could take a deeper look if they expected to see all "PASS".

>
> > ---
> >  test/daxctl-devices.sh |7 +++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/test/daxctl-devices.sh b/test/daxctl-devices.sh
> > index 04f53f7b13ab..4102fb6990ae 100755
> > --- a/test/daxctl-devices.sh
> > +++ b/test/daxctl-devices.sh
> > @@ -18,6 +18,13 @@ find_testdev()
> >  {
> >   local rc=77
> >
> > + # The kmem driver is needed to change the device mode, only
> > + # kernels >= v5.1 might have it available. Skip if not.
> > + if ! modinfo kmem; then
> > + "Unable to find kmem module"
>
> I think you need a printf, there.

Whoops, yes.

> Also, do you want the modinfo output in the test log?

Don't think it matters either way since it all ends up in the log.
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [RFC PATCH v2 15/19] mm/gup: Introduce vaddr_pin_pages()

2019-08-12 Thread John Hubbard


On 8/12/19 2:00 PM, Ira Weiny wrote:

On Fri, Aug 09, 2019 at 05:09:54PM -0700, John Hubbard wrote:

On 8/9/19 3:58 PM, ira.we...@intel.com wrote:

From: Ira Weiny 

...


At one point I wanted to (and had in my tree) a new flag but I went away from
it.  Prior to the discussion on mlock last week I did not think we needed it.
But I'm ok to add it back in.

I was not ignoring the idea for this RFC I just wanted to get this out there
for people to see.  I see that you threw out a couple of patches which add this
flag in.

FWIW, I think it would be good to differentiate between an indefinite pinned
page vs a referenced "gotten" page.

What you and I have been working on is the former.  So it would be easy to
change your refcounting patches to simply key off of FOLL_PIN.

Would you like me to add in your FOLL_PIN patches to this series?


Sure, that would be perfect. They don't make any sense on their own, and
it's all part of the same design idea.

thanks,
--
John Hubbard
NVIDIA
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [RFC PATCH v2 16/19] RDMA/uverbs: Add back pointer to system file object

On Mon, Aug 12, 2019 at 02:56:15PM -0300, Jason Gunthorpe wrote:
> On Mon, Aug 12, 2019 at 10:28:27AM -0700, Ira Weiny wrote:
> > On Mon, Aug 12, 2019 at 10:00:40AM -0300, Jason Gunthorpe wrote:
> > > On Fri, Aug 09, 2019 at 03:58:30PM -0700, ira.we...@intel.com wrote:
> > > > From: Ira Weiny 
> > > > 
> > > > In order for MRs to be tracked against the open verbs context the ufile
> > > > needs to have a pointer to hand to the GUP code.
> > > > 
> > > > No references need to be taken as this should be valid for the lifetime
> > > > of the context.
> > > > 
> > > > Signed-off-by: Ira Weiny 
> > > >  drivers/infiniband/core/uverbs.h  | 1 +
> > > >  drivers/infiniband/core/uverbs_main.c | 1 +
> > > >  2 files changed, 2 insertions(+)
> > > > 
> > > > diff --git a/drivers/infiniband/core/uverbs.h 
> > > > b/drivers/infiniband/core/uverbs.h
> > > > index 1e5aeb39f774..e802ba8c67d6 100644
> > > > +++ b/drivers/infiniband/core/uverbs.h
> > > > @@ -163,6 +163,7 @@ struct ib_uverbs_file {
> > > > struct page *disassociate_page;
> > > >  
> > > > struct xarray   idr;
> > > > +   struct file *sys_file; /* backpointer to system 
> > > > file object */
> > > >  };
> > > 
> > > The 'struct file' has a lifetime strictly shorter than the
> > > ib_uverbs_file, which is kref'd on its own lifetime. Having a back
> > > pointer like this is confouding as it will be invalid for some of the
> > > lifetime of the struct.
> > 
> > Ah...  ok.  I really thought it was the other way around.
> > 
> > __fput() should not call ib_uverbs_close() until the last reference on 
> > struct
> > file is released...  What holds references to struct ib_uverbs_file past 
> > that?
> 
> Child fds hold onto the internal ib_uverbs_file until they are closed

The FDs hold the struct file, don't they?

> 
> > Perhaps I need to add this (untested)?
> > 
> > diff --git a/drivers/infiniband/core/uverbs_main.c
> > b/drivers/infiniband/core/uverbs_main.c
> > index f628f9e4c09f..654e774d9cf2 100644
> > +++ b/drivers/infiniband/core/uverbs_main.c
> > @@ -1125,6 +1125,8 @@ static int ib_uverbs_close(struct inode *inode, 
> > struct file *filp)
> > list_del_init(&file->list);
> > mutex_unlock(&file->device->lists_mutex);
> >  
> > +   file->sys_file = NULL;
> 
> Now this has unlocked updates to that data.. you'd need some lock and
> get not zero pattern

You can't call "get" here because I'm 99% sure we only get here when struct
file has no references left...  I could be wrong.  It took me a while to work
through the reference counting so I could have missed something.

Ira

___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v9 04/18] kunit: test: add kunit_stream a std::stream like logger

On Fri, Aug 02, 2019 at 09:37:53AM +0200, John Ogness wrote:
> On 2019-08-01, Brendan Higgins  wrote:
> > On Fri, Jul 26, 2019 at 1:31 AM Petr Mladek  wrote:
> >> On Thu 2019-07-25 13:21:12, Brendan Higgins wrote:
> >>> On Wed, Jul 24, 2019 at 12:31 AM Petr Mladek  wrote:
>  On Mon 2019-07-22 16:54:10, Stephen Boyd wrote:
> > Quoting Brendan Higgins (2019-07-22 15:30:49)
> >> On Mon, Jul 22, 2019 at 1:03 PM Stephen Boyd  wrote:
> >>> What's the calling context of the assertions and expectations? I
> >>> still don't like the fact that string stream needs to allocate
> >>> buffers and throw them into a list somewhere because the calling
> >>> context matters there.
> >>
> >> The calling context is the same as before, which is anywhere.
> >
> > Ok. That's concerning then.
> >
> >>> I'd prefer we just wrote directly to the console/log via printk
> >>> instead. That way things are simple because we use the existing
> >>> buffering path of printk, but maybe there's some benefit to the
> >>> string stream that I don't see? Right now it looks like it
> >>> builds a string and then dumps it to printk so I'm sort of lost
> >>> what the benefit is over just writing directly with printk.
> >>
> >> It's just buffering it so the whole string gets printed
> >> uninterrupted.  If we were to print out piecemeal to printk,
> >> couldn't we have another call to printk come in causing it to
> >> garble the KUnit message we are in the middle of printing?
> >
> > Yes, printing piecemeal by calling printk many times could lead to
> > interleaving of messages if something else comes in such as an
> > interrupt printing something. Printk has some support to hold
> > "records" but I'm not sure how that would work here because
> > KERN_CONT talks about only being used early on in boot code. I
> > haven't looked at printk in detail though so maybe I'm all wrong
> > and KERN_CONT just works?
> 
>  KERN_CONT does not guarantee that the message will get printed
>  together. The pieces get interleaved with messages printed in
>  parallel.
> 
>  Note that KERN_CONT was originally really meant to be used only
>  during boot. It was later used more widely and ended in the best
>  effort category.
> 
>  There were several attempts to make it more reliable. But it was
>  always either too complicated or error prone or both.
> 
>  You need to use your own buffering if you rely want perfect output.
>  The question is if it is really worth the complexity. Also note
>  that any buffering reduces the chance that the messages will reach
>  the console.
> >>>
> >>> Seems like that settles it then. Thanks!
> >>>
>  BTW: There is a work in progress on a lockless printk ring buffer.
>  It will make printk() more secure regarding deadlocks. But it might
>  make transparent handling of continuous lines even more tricky.
> 
>  I guess that local buffering, before calling printk(), will be
>  even more important then. Well, it might really force us to create
>  an API for it.
> >>>
> >>> Cool! Can you CC me on that discussion?
> >>
> >> Adding John Oggness into CC.
> >>
> >> John, please CC Brendan Higgins on the patchsets eventually switching
> >> printk() into the lockless buffer. The test framework is going to
> >> do its own buffering to keep the related messages together.
> >>
> >> The lockless ringbuffer might make handling of related (partial)
> >> lines worse or better. It might justify KUnit's extra buffering
> >> or it might allow to get rid of it.
> >
> > Thanks for CC'ing me on the printk ringbuffer thread. It looks like it
> > actually probably won't affect my needs for KUnit logging. The biggest
> > reason I need some sort of buffering system is to be able to compose
> > messages piece meal into a single message that will be printed out to
> > the user as a single message with no messages from other printk
> > callers printed out in the middle of mine.
> 
> printk has this same requirement for its CONT messages. You can read
> about how I propose to implement that here[0], using a separate prb
> ringbuffer for buffered storage until all the pieces are available.
> 
> It is not my goal that multiple subsystems start making use of the prb
> ringbuffer. However, its features can be attractive if you don't want to
> worry about multiple writers/readers or context (including NMI). Before

That sounds like it might be useful down the road, but not to replace
the string_stream.

> writing "yet another ringbuffer" [1] it might be worth the effort to at
> least see if one of the existing implementations can work (or be
> extended to work) for you.

In regards to the conversation here about string_stream/kunit_stream, I
think Petr already answered that question. As I said previously:
> [I]t appears that to get the
> semantics that I need, I

Re: [RFC PATCH v2 15/19] mm/gup: Introduce vaddr_pin_pages()

On Sun, Aug 11, 2019 at 04:07:23PM -0700, John Hubbard wrote:
> On 8/9/19 3:58 PM, ira.we...@intel.com wrote:
> > From: Ira Weiny 
> > 
> > The addition of FOLL_LONGTERM has taken on additional meaning for CMA
> > pages.
> > 
> > In addition subsystems such as RDMA require new information to be passed
> > to the GUP interface to track file owning information.  As such a simple
> > FOLL_LONGTERM flag is no longer sufficient for these users to pin pages.
> > 
> > Introduce a new GUP like call which takes the newly introduced vaddr_pin
> > information.  Failure to pass the vaddr_pin object back to a vaddr_put*
> > call will result in a failure if pins were created on files during the
> > pin operation.
> > 
> > Signed-off-by: Ira Weiny 
> > 
> 
> I'm creating a new call site conversion series, to replace the 
> "put_user_pages(): miscellaneous call sites" series. This uses
> vaddr_pin_pages*() where appropriate. So it's based on your series here.
> 
> btw, while doing that, I noticed one more typo while re-reading some of the 
> comments. 
> Thought you probably want to collect them all for the next spin. Below...
> 
> > ---
> > Changes from list:
> > Change to vaddr_put_pages_dirty_lock
> > Change to vaddr_unpin_pages_dirty_lock
> > 
> >  include/linux/mm.h |  5 
> >  mm/gup.c   | 59 ++
> >  2 files changed, 64 insertions(+)
> > 
> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index 657c947bda49..90c5802866df 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -1603,6 +1603,11 @@ int account_locked_vm(struct mm_struct *mm, unsigned 
> > long pages, bool inc);
> >  int __account_locked_vm(struct mm_struct *mm, unsigned long pages, bool 
> > inc,
> > struct task_struct *task, bool bypass_rlim);
> >  
> > +long vaddr_pin_pages(unsigned long addr, unsigned long nr_pages,
> > +unsigned int gup_flags, struct page **pages,
> > +struct vaddr_pin *vaddr_pin);
> > +void vaddr_unpin_pages_dirty_lock(struct page **pages, unsigned long 
> > nr_pages,
> > + struct vaddr_pin *vaddr_pin, bool make_dirty);
> >  bool mapping_inode_has_layout(struct vaddr_pin *vaddr_pin, struct page 
> > *page);
> >  
> >  /* Container for pinned pfns / pages */
> > diff --git a/mm/gup.c b/mm/gup.c
> > index eeaa0ddd08a6..6d23f70d7847 100644
> > --- a/mm/gup.c
> > +++ b/mm/gup.c
> > @@ -2536,3 +2536,62 @@ int get_user_pages_fast(unsigned long start, int 
> > nr_pages,
> > return ret;
> >  }
> >  EXPORT_SYMBOL_GPL(get_user_pages_fast);
> > +
> > +/**
> > + * vaddr_pin_pages pin pages by virtual address and return the pages to the
> > + * user.
> > + *
> > + * @addr, start address
> > + * @nr_pages, number of pages to pin
> > + * @gup_flags, flags to use for the pin
> > + * @pages, array of pages returned
> > + * @vaddr_pin, initalized meta information this pin is to be associated
> 
> Typo:
>   initialized

Thanks fixed.
Ira

> 
> 
> thanks,
> -- 
> John Hubbard
> NVIDIA
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [RFC PATCH v2 15/19] mm/gup: Introduce vaddr_pin_pages()

On Fri, Aug 09, 2019 at 05:09:54PM -0700, John Hubbard wrote:
> On 8/9/19 3:58 PM, ira.we...@intel.com wrote:
> > From: Ira Weiny 
> > 
> > The addition of FOLL_LONGTERM has taken on additional meaning for CMA
> > pages.
> > 
> > In addition subsystems such as RDMA require new information to be passed
> > to the GUP interface to track file owning information.  As such a simple
> > FOLL_LONGTERM flag is no longer sufficient for these users to pin pages.
> > 
> > Introduce a new GUP like call which takes the newly introduced vaddr_pin
> > information.  Failure to pass the vaddr_pin object back to a vaddr_put*
> > call will result in a failure if pins were created on files during the
> > pin operation.
> > 
> > Signed-off-by: Ira Weiny 
> > 
> > ---
> > Changes from list:
> > Change to vaddr_put_pages_dirty_lock
> > Change to vaddr_unpin_pages_dirty_lock
> > 
> >  include/linux/mm.h |  5 
> >  mm/gup.c   | 59 ++
> >  2 files changed, 64 insertions(+)
> > 
> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index 657c947bda49..90c5802866df 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -1603,6 +1603,11 @@ int account_locked_vm(struct mm_struct *mm, unsigned 
> > long pages, bool inc);
> >  int __account_locked_vm(struct mm_struct *mm, unsigned long pages, bool 
> > inc,
> > struct task_struct *task, bool bypass_rlim);
> >  
> > +long vaddr_pin_pages(unsigned long addr, unsigned long nr_pages,
> > +unsigned int gup_flags, struct page **pages,
> > +struct vaddr_pin *vaddr_pin);
> > +void vaddr_unpin_pages_dirty_lock(struct page **pages, unsigned long 
> > nr_pages,
> > + struct vaddr_pin *vaddr_pin, bool make_dirty);
> 
> Hi Ira,
> 
> OK, the API seems fine to me, anyway. :)
> 
> A bit more below...
> 
> >  bool mapping_inode_has_layout(struct vaddr_pin *vaddr_pin, struct page 
> > *page);
> >  
> >  /* Container for pinned pfns / pages */
> > diff --git a/mm/gup.c b/mm/gup.c
> > index eeaa0ddd08a6..6d23f70d7847 100644
> > --- a/mm/gup.c
> > +++ b/mm/gup.c
> > @@ -2536,3 +2536,62 @@ int get_user_pages_fast(unsigned long start, int 
> > nr_pages,
> > return ret;
> >  }
> >  EXPORT_SYMBOL_GPL(get_user_pages_fast);
> > +
> > +/**
> > + * vaddr_pin_pages pin pages by virtual address and return the pages to the
> > + * user.
> > + *
> > + * @addr, start address
> 
> What's with the commas? I thought kernel-doc wants colons, like this, right?
> 
> @addr: start address

:-/  I don't know.

Fixed.

> 
> 
> > + * @nr_pages, number of pages to pin
> > + * @gup_flags, flags to use for the pin
> > + * @pages, array of pages returned
> > + * @vaddr_pin, initalized meta information this pin is to be associated
> > + * with.
> > + *
> > + * NOTE regarding vaddr_pin:
> > + *
> > + * Some callers can share pins via file descriptors to other processes.
> > + * Callers such as this should use the f_owner field of vaddr_pin to 
> > indicate
> > + * the file the fd points to.  All other callers should use the mm this 
> > pin is
> > + * being made against.  Usually "current->mm".
> > + *
> > + * Expects mmap_sem to be read locked.
> > + */
> > +long vaddr_pin_pages(unsigned long addr, unsigned long nr_pages,
> > +unsigned int gup_flags, struct page **pages,
> > +struct vaddr_pin *vaddr_pin)
> > +{
> > +   long ret;
> > +
> > +   gup_flags |= FOLL_LONGTERM;
> 
> 
> Is now the right time to introduce and use FOLL_PIN? If not, then I can always
> add it on top of this later, as part of gup-tracking patches. But you did 
> point
> out that FOLL_LONGTERM is taking on additional meaning, and so maybe it's 
> better
> to split that meaning up right from the start.
> 

At one point I wanted to (and had in my tree) a new flag but I went away from
it.  Prior to the discussion on mlock last week I did not think we needed it.
But I'm ok to add it back in.

I was not ignoring the idea for this RFC I just wanted to get this out there
for people to see.  I see that you threw out a couple of patches which add this
flag in.

FWIW, I think it would be good to differentiate between an indefinite pinned
page vs a referenced "gotten" page.

What you and I have been working on is the former.  So it would be easy to
change your refcounting patches to simply key off of FOLL_PIN.

Would you like me to add in your FOLL_PIN patches to this series?

> 
> > +
> > +   if (!vaddr_pin || (!vaddr_pin->mm && !vaddr_pin->f_owner))
> > +   return -EINVAL;
> > +
> > +   ret = __gup_longterm_locked(current,
> > +   vaddr_pin->mm,
> > +   addr, nr_pages,
> > +   pages, NULL, gup_flags,
> > +   vaddr_pin);
> > +   return ret;
> > +}
> > +EXPORT_SYMBOL(vaddr_pin_pages);
> > +
> > +/**
> > + * vaddr_unpin_pages_dirty_lock - counterpart to vaddr_p

Re: [RFC PATCH v2 12/19] mm/gup: Prep put_user_pages() to take an vaddr_pin struct

On Fri, Aug 09, 2019 at 05:30:00PM -0700, John Hubbard wrote:
> On 8/9/19 3:58 PM, ira.we...@intel.com wrote:
> > From: Ira Weiny 
> > 
> > Once callers start to use vaddr_pin the put_user_pages calls will need
> > to have access to this data coming in.  Prep put_user_pages() for this
> > data.
> > 
> > Signed-off-by: Ira Weiny 

[snip]

> > diff --git a/mm/gup.c b/mm/gup.c
> > index a7a9d2f5278c..10cfd30ff668 100644
> > --- a/mm/gup.c
> > +++ b/mm/gup.c
> > @@ -24,30 +24,41 @@
> >  
> >  #include "internal.h"
> >  
> > -/**
> > - * put_user_pages_dirty_lock() - release and optionally dirty gup-pinned 
> > pages
> > - * @pages:  array of pages to be maybe marked dirty, and definitely 
> > released.
> 
> A couple comments from our circular review chain: some fellow with the same
> last name as you, recommended wording it like this:
> 
>   @pages:  array of pages to be put

Sure, see below...

> 
> > - * @npages: number of pages in the @pages array.
> > - * @make_dirty: whether to mark the pages dirty
> > - *
> > - * "gup-pinned page" refers to a page that has had one of the 
> > get_user_pages()
> > - * variants called on that page.
> > - *
> > - * For each page in the @pages array, make that page (or its head page, if 
> > a
> > - * compound page) dirty, if @make_dirty is true, and if the page was 
> > previously
> > - * listed as clean. In any case, releases all pages using put_user_page(),
> > - * possibly via put_user_pages(), for the non-dirty case.
> > - *
> > - * Please see the put_user_page() documentation for details.
> > - *
> > - * set_page_dirty_lock() is used internally. If instead, set_page_dirty() 
> > is
> > - * required, then the caller should a) verify that this is really correct,
> > - * because _lock() is usually required, and b) hand code it:
> > - * set_page_dirty_lock(), put_user_page().
> > - *
> > - */
> > -void put_user_pages_dirty_lock(struct page **pages, unsigned long npages,
> > -  bool make_dirty)
> > +static void __put_user_page(struct vaddr_pin *vaddr_pin, struct page *page)
> > +{
> > +   page = compound_head(page);
> > +
> > +   /*
> > +* For devmap managed pages we need to catch refcount transition from
> > +* GUP_PIN_COUNTING_BIAS to 1, when refcount reach one it means the
> > +* page is free and we need to inform the device driver through
> > +* callback. See include/linux/memremap.h and HMM for details.
> > +*/
> > +   if (put_devmap_managed_page(page))
> > +   return;
> > +
> > +   if (put_page_testzero(page))
> > +   __put_page(page);
> > +}
> > +
> > +static void __put_user_pages(struct vaddr_pin *vaddr_pin, struct page 
> > **pages,
> > +unsigned long npages)
> > +{
> > +   unsigned long index;
> > +
> > +   /*
> > +* TODO: this can be optimized for huge pages: if a series of pages is
> > +* physically contiguous and part of the same compound page, then a
> > +* single operation to the head page should suffice.
> > +*/
> 
> As discussed in the other review thread (""), let's just delete that comment,
> as long as you're moving things around.

Done.

> 
> 
> > +   for (index = 0; index < npages; index++)
> > +   __put_user_page(vaddr_pin, pages[index]);
> > +}
> > +
> > +static void __put_user_pages_dirty_lock(struct vaddr_pin *vaddr_pin,
> > +   struct page **pages,
> > +   unsigned long npages,
> > +   bool make_dirty)
> 
> Elsewhere in this series, we pass vaddr_pin at the end of the arg list.
> Here we pass it at the beginning, and it caused a minor jar when reading it.
> Obviously just bike shedding at this point, though. Either way. :)

Yea I guess that is odd...  I changed it.  Not a big deal.

> 
> >  {
> > unsigned long index;
> >  
> > @@ -58,7 +69,7 @@ void put_user_pages_dirty_lock(struct page **pages, 
> > unsigned long npages,
> >  */
> >  
> > if (!make_dirty) {
> > -   put_user_pages(pages, npages);
> > +   __put_user_pages(vaddr_pin, pages, npages);
> > return;
> > }
> >  
> > @@ -86,9 +97,58 @@ void put_user_pages_dirty_lock(struct page **pages, 
> > unsigned long npages,
> >  */
> > if (!PageDirty(page))
> > set_page_dirty_lock(page);
> > -   put_user_page(page);
> > +   __put_user_page(vaddr_pin, page);
> > }
> >  }
> > +
> > +/**
> > + * put_user_page() - release a gup-pinned page
> > + * @page:pointer to page to be released
> > + *
> > + * Pages that were pinned via get_user_pages*() must be released via
> > + * either put_user_page(), or one of the put_user_pages*() routines
> > + * below. This is so that eventually, pages that are pinned via
> > + * get_user_pages*() can be separately tracked and uniquely handled. In
> > + * particular, interactions with RDMA and filesystems need special
> > + * handling.
> > + *
> > + *

Re: [ndctl PATCH] daxctl/test: Skip daxctl-devices.sh on older kernels

Dan Williams  writes:

> If the 'kmem' module is missing skip the test to support running the
> unit tests on older -stable kernels pre-v5.1.
>
> Signed-off-by: Dan Williams 

Note that the kernel module could also just not be configured.

> ---
>  test/daxctl-devices.sh |7 +++
>  1 file changed, 7 insertions(+)
>
> diff --git a/test/daxctl-devices.sh b/test/daxctl-devices.sh
> index 04f53f7b13ab..4102fb6990ae 100755
> --- a/test/daxctl-devices.sh
> +++ b/test/daxctl-devices.sh
> @@ -18,6 +18,13 @@ find_testdev()
>  {
>   local rc=77
>  
> + # The kmem driver is needed to change the device mode, only
> + # kernels >= v5.1 might have it available. Skip if not.
> + if ! modinfo kmem; then
> + "Unable to find kmem module"

I think you need a printf, there.  Also, do you want the modinfo output
in the test log?

-Jeff

> + exit $rc
> + fi
> +
>   # find a victim device
>   testbus="$ACPI_BUS"
>   testdev=$("$NDCTL" list -b "$testbus" -Ni | jq -er '.[0].dev | .//""')
>
> ___
> Linux-nvdimm mailing list
> Linux-nvdimm@lists.01.org
> https://lists.01.org/mailman/listinfo/linux-nvdimm
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v9 04/18] kunit: test: add kunit_stream a std::stream like logger

On Thu, Aug 1, 2019 at 2:43 PM Brendan Higgins
 wrote:
>
> On Thu, Aug 1, 2019 at 2:14 PM Stephen Boyd  wrote:
> >
> > Quoting Brendan Higgins (2019-08-01 11:59:57)
> > > On Thu, Aug 1, 2019 at 11:55 AM Brendan Higgins
> > >  wrote:
> > > >
> > > > On Fri, Jul 26, 2019 at 1:31 AM Petr Mladek  wrote:
> > > >
> > > > > To be honest I do not fully understand KUnit design. I am not
> > > > > completely sure how the tested code is isolated from the running
> > > > > system. Namely, I do not know if the tested code shares
> > > > > the same locks with the system running the test.
> > > >
> > > > No worries, I don't expect printk to be the hang up in those cases. It
> > > > sounds like KUnit has a long way to evolve before printk is going to
> > > > be a limitation.
> > >
> > > So Stephen, what do you think?
> > >
> > > Do you want me to go forward with the new kunit_assert API wrapping
> > > the string_stream as I have it now? Would you prefer to punt this to a
> > > later patch? Or would you prefer something else?
> > >
> >
> > I like the struct based approach. If anything, it can be adjusted to
> > make the code throw some records into a spinlock later on and delay the
> > formatting of the assertion if need be.
>
> That's a fair point.
>
> > Can you resend with that
> > approach? I don't think I'll have any more comments after that.

I sent a new revision, v12, that incorporates the kunit_assert stuff.

Let me know what you think!
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Работа с жалобами клиентов

2019-08-12 Thread Evelina

Секреты успеха в работе с трудными клиентами

Дата и место проведения: 20 августа

Цель программы:
- Усилить свое воздействие на клиента, создать основу для доверия, используя
незаметные, но отлично действующие психологические приемы.
- Узнать, как с наименьшими негативными последствиями сообщать негативную
информацию клиентам.
- Научиться быстро и с пользой для дальнейшей работы разрешать конфликтные
ситуации
- Найти те эмоциональные "кнопки", нажав на которые вы быстрее справитесь с
возражениями клиентов.
- Понять, как не дать загнать себя в ловушку гнева, раздражения, и бессилия,
если клиент был агрессивен, или несправедливо обвинял вас и вашу компанию.
И, наконец, узнать как организовать свою работу так, чтобы сложных, вредных,
агрессивных, злобных клиентов у вас не было. А были партнеры и друзья!

В 90% случаев, можно договориться и убедить самых сложных клиентов, можно
разрешить конфликт к взаимному удовлетворению и разобраться с жалобой на уровне
межличностных отношений.

Обзор:

1. "Анализируй это!"
- Причины, по которым клиент становится "трудным"
- А в чем, собственно, трудность? Немного о типологии клиентов.
- Непростые отношения в паре: Я + Клиент.

2. "Полюби меня такой, какая я есть".
- Может я сам виноват, что клиент стал сложным? Мелочи (и не только) вызывающие
раздражение.
- Как вызвать симпатию и доверие – 7 основных правил.

3. "Пaльбa, тpaктиpы, cтычки, шпaги, кoни…" или как не погрязнуть в конфликтной
ситуации:
- Холодный ум – в чем истинная причина конфликта?
- Горячее сердце – парадоксальные техники поведения в конфликтных ситуациях.
- Как отделить себя от неприятной ситуации – лайфхак от Майкла Гриндера.
- И еще – эффективные техники снятия/снижения агрессии клиента и сохранение
собственных ресурсов.

4. "Склоненную голову меч не сечет" - работаем с претензиями и жалобами
клиентов:
- Как реагировать на критику клиента? Невинные манипуляции нам в помощь.
- Алгоритм работы с жалобами и претензиями клиентов.
- Искусство отказать клиенту, сохраняя отношения.

5. "А я не хочу не хочу по расчету, а я по любви, по любви хочу!"
- Что стоит за возражениями клиентов?
- 6 эмоциональных точек, используя которые мы найдем наилучшие аргументы.
- 7 правил о которых нельзя забывать, работая с возражениями.

6. "Любов – це таке почуття…"
- Тайны психологического настроя на клиента и успех, или как полюбить даже
самого трудного клиента!

Подробнее на сайте>
http://profi-b2b.in.ua/training/144/sekreti-uspeha-v-rabote-s-trudnimi-klientami.htm

Чтобы отписаться от рассылки, пройдите по этой ссылке Отписаться от рассылки.
List-Unsubscribe from the newsletter оr complain аbоut SPАМ.
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [RFC PATCH v2 11/19] mm/gup: Pass follow_page_context further down the call stack

On Fri, Aug 09, 2019 at 05:18:31PM -0700, John Hubbard wrote:
> On 8/9/19 3:58 PM, ira.we...@intel.com wrote:
> > From: Ira Weiny 
> > 
> > In preparation for passing more information (vaddr_pin) into
> > follow_page_pte(), follow_devmap_pud(), and follow_devmap_pmd().
> > 
> > Signed-off-by: Ira Weiny 

[snip]

> > @@ -786,7 +782,8 @@ static int check_vma_flags(struct vm_area_struct *vma, 
> > unsigned long gup_flags)
> >  static long __get_user_pages(struct task_struct *tsk, struct mm_struct *mm,
> > unsigned long start, unsigned long nr_pages,
> > unsigned int gup_flags, struct page **pages,
> > -   struct vm_area_struct **vmas, int *nonblocking)
> > +   struct vm_area_struct **vmas, int *nonblocking,
> > +   struct vaddr_pin *vaddr_pin)
> 
> I didn't expect to see more vaddr_pin arg passing, based on the commit
> description. Did you want this as part of patch 9 or 10 instead? If not,
> then let's mention it in the commit description.

Yea that does seem out of place now that I look at it.  I'll add to the commit
message because this is really getting vaddr_pin into the context _and_ passing
it down the stack.  With all the rebasing I may have squashed something I did
not mean to.  But I think this patch is ok because it is not to complicated to
see what is going on.

Thanks,
Ira

> 
> >  {
> > long ret = 0, i = 0;
> > struct vm_area_struct *vma = NULL;
> > @@ -797,6 +794,8 @@ static long __get_user_pages(struct task_struct *tsk, 
> > struct mm_struct *mm,
> >  
> > VM_BUG_ON(!!pages != !!(gup_flags & FOLL_GET));
> >  
> > +   ctx.vaddr_pin = vaddr_pin;
> > +
> > /*
> >  * If FOLL_FORCE is set then do not force a full fault as the hinting
> >  * fault information is unrelated to the reference behaviour of a task
> > @@ -1025,7 +1024,7 @@ static __always_inline long 
> > __get_user_pages_locked(struct task_struct *tsk,
> > lock_dropped = false;
> > for (;;) {
> > ret = __get_user_pages(tsk, mm, start, nr_pages, flags, pages,
> > -  vmas, locked);
> > +  vmas, locked, vaddr_pin);
> > if (!locked)
> > /* VM_FAULT_RETRY couldn't trigger, bypass */
> > return ret;
> > @@ -1068,7 +1067,7 @@ static __always_inline long 
> > __get_user_pages_locked(struct task_struct *tsk,
> > lock_dropped = true;
> > down_read(&mm->mmap_sem);
> > ret = __get_user_pages(tsk, mm, start, 1, flags | FOLL_TRIED,
> > -  pages, NULL, NULL);
> > +  pages, NULL, NULL, vaddr_pin);
> > if (ret != 1) {
> > BUG_ON(ret > 1);
> > if (!pages_done)
> > @@ -1226,7 +1225,7 @@ long populate_vma_page_range(struct vm_area_struct 
> > *vma,
> >  * not result in a stack expansion that recurses back here.
> >  */
> > return __get_user_pages(current, mm, start, nr_pages, gup_flags,
> > -   NULL, NULL, nonblocking);
> > +   NULL, NULL, nonblocking, NULL);
> >  }
> >  
> >  /*
> > @@ -1311,7 +1310,7 @@ struct page *get_dump_page(unsigned long addr)
> >  
> > if (__get_user_pages(current, current->mm, addr, 1,
> >  FOLL_FORCE | FOLL_DUMP | FOLL_GET, &page, &vma,
> > -NULL) < 1)
> > +NULL, NULL) < 1)
> > return NULL;
> > flush_cache_page(vma, addr, page_to_pfn(page));
> > return page;
> > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > index bc1a07a55be1..7e09f2f17ed8 100644
> > --- a/mm/huge_memory.c
> > +++ b/mm/huge_memory.c
> > @@ -916,8 +916,9 @@ static void touch_pmd(struct vm_area_struct *vma, 
> > unsigned long addr,
> >  }
> >  
> >  struct page *follow_devmap_pmd(struct vm_area_struct *vma, unsigned long 
> > addr,
> > -   pmd_t *pmd, int flags, struct dev_pagemap **pgmap)
> > +   pmd_t *pmd, int flags, struct follow_page_context *ctx)
> >  {
> > +   struct dev_pagemap **pgmap = &ctx->pgmap;
> > unsigned long pfn = pmd_pfn(*pmd);
> > struct mm_struct *mm = vma->vm_mm;
> > struct page *page;
> > @@ -1068,8 +1069,9 @@ static void touch_pud(struct vm_area_struct *vma, 
> > unsigned long addr,
> >  }
> >  
> >  struct page *follow_devmap_pud(struct vm_area_struct *vma, unsigned long 
> > addr,
> > -   pud_t *pud, int flags, struct dev_pagemap **pgmap)
> > +   pud_t *pud, int flags, struct follow_page_context *ctx)
> >  {
> > +   struct dev_pagemap **pgmap = &ctx->pgmap;
> > unsigned long pfn = pud_pfn(*pud);
> > struct mm_struct *mm = vma->vm_mm;
> > struct page *page;
> > diff --git a/mm/internal.h b/mm/internal.h
> > index 0d5f720c75ab..46ada5279856 100644
> > --- a/mm/internal.h
> > +++ b/mm/internal.h
> > @@ -12,6 +12,34 @@
> >  #include 
> >  #include 
> >  
> > +struct follow_p

[PATCH v12 09/18] kunit: test: add support for test abort

Add support for aborting/bailing out of test cases, which is needed for
implementing assertions.

An assertion is like an expectation, but bails out of the test case
early if the assertion is not met. The idea with assertions is that you
use them to state all the preconditions for your test. Logically
speaking, these are the premises of the test case, so if a premise isn't
true, there is no point in continuing the test case because there are no
conclusions that can be drawn without the premises. Whereas, the
expectation is the thing you are trying to prove.

Signed-off-by: Brendan Higgins 
Reviewed-by: Greg Kroah-Hartman 
Reviewed-by: Logan Gunthorpe 
---
 include/kunit/test.h  |  14 +++
 include/kunit/try-catch.h |  69 +++
 kunit/Makefile|   3 +-
 kunit/test.c  | 179 ++
 kunit/try-catch.c |  95 
 5 files changed, 344 insertions(+), 16 deletions(-)
 create mode 100644 include/kunit/try-catch.h
 create mode 100644 kunit/try-catch.c

diff --git a/include/kunit/test.h b/include/kunit/test.h
index 2625bcfeb19ac..93381f841e09f 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -13,6 +13,7 @@
 #include 
 #include 
 #include 
+#include 
 
 struct kunit_resource;
 
@@ -167,6 +168,7 @@ struct kunit {
 
/* private: internal use only. */
const char *name; /* Read only after initialization! */
+   struct kunit_try_catch try_catch;
/*
 * success starts as true, and may only be set to false during a test
 * case; thus, it is safe to update this across multiple threads using
@@ -176,6 +178,11 @@ struct kunit {
 */
bool success; /* Read only after test_case finishes! */
spinlock_t lock; /* Gaurds all mutable test state. */
+   /*
+* death_test may be both set and unset from multiple threads in a test
+* case.
+*/
+   bool death_test; /* Protected by lock. */
/*
 * Because resources is a list that may be updated multiple times (with
 * new resources) from any thread associated with a test case, we must
@@ -184,6 +191,13 @@ struct kunit {
struct list_head resources; /* Protected by lock. */
 };
 
+static inline void kunit_set_death_test(struct kunit *test, bool death_test)
+{
+   spin_lock(&test->lock);
+   test->death_test = death_test;
+   spin_unlock(&test->lock);
+}
+
 void kunit_init_test(struct kunit *test, const char *name);
 
 int kunit_run_tests(struct kunit_suite *suite);
diff --git a/include/kunit/try-catch.h b/include/kunit/try-catch.h
new file mode 100644
index 0..8a414a9af0b64
--- /dev/null
+++ b/include/kunit/try-catch.h
@@ -0,0 +1,69 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * An API to allow a function, that may fail, to be executed, and recover in a
+ * controlled manner.
+ *
+ * Copyright (C) 2019, Google LLC.
+ * Author: Brendan Higgins 
+ */
+
+#ifndef _KUNIT_TRY_CATCH_H
+#define _KUNIT_TRY_CATCH_H
+
+#include 
+
+typedef void (*kunit_try_catch_func_t)(void *);
+
+struct kunit;
+
+/*
+ * struct kunit_try_catch - provides a generic way to run code which might 
fail.
+ * @context: used to pass user data to the try and catch functions.
+ *
+ * kunit_try_catch provides a generic, architecture independent way to execute
+ * an arbitrary function of type kunit_try_catch_func_t which may bail out by
+ * calling kunit_try_catch_throw(). If kunit_try_catch_throw() is called, @try
+ * is stopped at the site of invocation and @catch is catch is called.
+ *
+ * struct kunit_try_catch provides a generic interface for the functionality
+ * needed to implement kunit->abort() which in turn is needed for implementing
+ * assertions. Assertions allow stating a precondition for a test simplifying
+ * how test cases are written and presented.
+ *
+ * Assertions are like expectations, except they abort (call
+ * kunit_try_catch_throw()) when the specified condition is not met. This is
+ * useful when you look at a test case as a logical statement about some piece
+ * of code, where assertions are the premises for the test case, and the
+ * conclusion is a set of predicates, rather expectations, that must all be
+ * true. If your premises are violated, it does not makes sense to continue.
+ */
+struct kunit_try_catch {
+   /* private: internal use only. */
+   struct kunit *test;
+   struct completion *try_completion;
+   int try_result;
+   kunit_try_catch_func_t try;
+   kunit_try_catch_func_t catch;
+   void *context;
+};
+
+void kunit_try_catch_init(struct kunit_try_catch *try_catch,
+ struct kunit *test,
+ kunit_try_catch_func_t try,
+ kunit_try_catch_func_t catch);
+
+void kunit_try_catch_run(struct kunit_try_catch *try_catch, void *context);
+
+void __noreturn kunit_try_catch_throw(struct kunit_try_catch *try_catch);
+
+static inline int kunit_t

[PATCH v12 00/18] kunit: introduce KUnit, the Linux kernel unit testing framework

## TL;DR

This revision removes dependence on kunit_stream in favor of
kunit_assert, as suggested by Stephen Boyd. kunit_assert provides a more
structured interface for constructing messages and allows most required
data to be stored on the stack for most expectations until it is
determined that a failure message must be produced.

As a part of introducing kunit_assert, expectations (KUNIT_EXPECT_*) and
assertions (KUNIT_ASSERT_*) have been substantially refactored.
Nevertheless, behavior should be the same.

As this revision, adds a new patch, it, [PATCH v12 04/18], needs to be
reviewed. All other patches have appropriate reviews and acks.

I also rebased the patchset on v5.3-rc3.

## 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.3/v12 branch.

## Changes Since Last Version

- Dropped patch "[PATCH v11 04/18] kunit: test: add kunit_stream a
  std::stream like logger" and replaced it with "[PATCH v12 04/18]
  kunit: test: add assertion printing library", which provides a totally
  new mechanism for constructing expectation/assertion failure messages.
- Substantially refactored expectations and assertions definitions in
  [PATCH 05/18] and [PATCH 11/18] respectively.
- Rebased patchset on v5.3-rc3.
- Fixed a minor documentation bug.

[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.3/v12

-- 
2.23.0.rc1.153.gdeed80330f-goog

___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH v12 15/18] Documentation: kunit: add documentation for KUnit

Add documentation for KUnit, the Linux kernel unit testing framework.
- Add intro and usage guide for KUnit
- Add API reference

Signed-off-by: Felix Guo 
Signed-off-by: Brendan Higgins 
Cc: Jonathan Corbet 
Reviewed-by: Greg Kroah-Hartman 
Reviewed-by: Logan Gunthorpe 
---
 Documentation/dev-tools/index.rst   |   1 +
 Documentation/dev-tools/kunit/api/index.rst |  16 +
 Documentation/dev-tools/kunit/api/test.rst  |  14 +
 Documentation/dev-tools/kunit/faq.rst   |  62 +++
 Documentation/dev-tools/kunit/index.rst |  79 +++
 Documentation/dev-tools/kunit/start.rst | 180 ++
 Documentation/dev-tools/kunit/usage.rst | 576 
 7 files changed, 928 insertions(+)
 create mode 100644 Documentation/dev-tools/kunit/api/index.rst
 create mode 100644 Documentation/dev-tools/kunit/api/test.rst
 create mode 100644 Documentation/dev-tools/kunit/faq.rst
 create mode 100644 Documentation/dev-tools/kunit/index.rst
 create mode 100644 Documentation/dev-tools/kunit/start.rst
 create mode 100644 Documentation/dev-tools/kunit/usage.rst

diff --git a/Documentation/dev-tools/index.rst 
b/Documentation/dev-tools/index.rst
index b0522a4dd1073..09dee10d25928 100644
--- a/Documentation/dev-tools/index.rst
+++ b/Documentation/dev-tools/index.rst
@@ -24,6 +24,7 @@ whole; patches welcome!
gdb-kernel-debugging
kgdb
kselftest
+   kunit/index
 
 
 .. only::  subproject and html
diff --git a/Documentation/dev-tools/kunit/api/index.rst 
b/Documentation/dev-tools/kunit/api/index.rst
new file mode 100644
index 0..9b9bffe5d41a0
--- /dev/null
+++ b/Documentation/dev-tools/kunit/api/index.rst
@@ -0,0 +1,16 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+=
+API Reference
+=
+.. toctree::
+
+   test
+
+This section documents the KUnit kernel testing API. It is divided into the
+following sections:
+
+= 
==
+:doc:`test`   documents all of the standard testing API
+  excluding mocking or mocking related 
features.
+= 
==
diff --git a/Documentation/dev-tools/kunit/api/test.rst 
b/Documentation/dev-tools/kunit/api/test.rst
new file mode 100644
index 0..d0ce19b1e1185
--- /dev/null
+++ b/Documentation/dev-tools/kunit/api/test.rst
@@ -0,0 +1,14 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+
+Test API
+
+
+This file documents all of the standard testing API excluding mocking or 
mocking
+related features.
+
+.. kernel-doc:: include/kunit/test.h
+   :internal:
+
+.. kernel-doc:: include/kunit/kunit-stream.h
+   :internal:
diff --git a/Documentation/dev-tools/kunit/faq.rst 
b/Documentation/dev-tools/kunit/faq.rst
new file mode 100644
index 0..bf2095112d899
--- /dev/null
+++ b/Documentation/dev-tools/kunit/faq.rst
@@ -0,0 +1,62 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+==
+Frequently Asked Questions
+==
+
+How is this different from Autotest, kselftest, etc?
+
+KUnit is a unit testing framework. Autotest, kselftest (and some others) are
+not.
+
+A `unit test `_ is supposed to
+test a single unit of code in isolation, hence the name. A unit test should be
+the finest granularity of testing and as such should allow all possible code
+paths to be tested in the code under test; this is only possible if the code
+under test is very small and does not have any external dependencies outside of
+the test's control like hardware.
+
+There are no testing frameworks currently available for the kernel that do not
+require installing the kernel on a test machine or in a VM and all require
+tests to be written in userspace and run on the kernel under test; this is true
+for Autotest, kselftest, and some others, disqualifying any of them from being
+considered unit testing frameworks.
+
+Does KUnit support running on architectures other than UML?
+===
+
+Yes, well, mostly.
+
+For the most part, the KUnit core framework (what you use to write the tests)
+can compile to any architecture; it compiles like just another part of the
+kernel and runs when the kernel boots. However, there is some infrastructure,
+like the KUnit Wrapper (``tools/testing/kunit/kunit.py``) that does not support
+other architectures.
+
+In short, this means that, yes, you can run KUnit on other architectures, but
+it might require more work than using KUnit on UML.
+
+For more information, see :ref:`kunit-on-non-uml`.
+
+What is the difference between a unit test and these other kinds of tests?
+==
+Most existing tests for the Linux kernel would be categorized as an integration
+test, or an end-to-end test

[PATCH v12 14/18] kunit: defconfig: add defconfigs for building KUnit tests

Add defconfig for UML and a fragment that can be used to configure other
architectures for building KUnit tests. Add option to kunit_tool to use
a defconfig to create the kunitconfig.

Signed-off-by: Brendan Higgins 
Reviewed-by: Greg Kroah-Hartman 
Reviewed-by: Logan Gunthorpe 
---
 arch/um/configs/kunit_defconfig  |  8 ++
 tools/testing/kunit/configs/all_tests.config |  8 ++
 tools/testing/kunit/kunit.py | 28 +---
 tools/testing/kunit/kunit_kernel.py  |  3 ++-
 4 files changed, 42 insertions(+), 5 deletions(-)
 create mode 100644 arch/um/configs/kunit_defconfig
 create mode 100644 tools/testing/kunit/configs/all_tests.config

diff --git a/arch/um/configs/kunit_defconfig b/arch/um/configs/kunit_defconfig
new file mode 100644
index 0..bfe49689038f1
--- /dev/null
+++ b/arch/um/configs/kunit_defconfig
@@ -0,0 +1,8 @@
+CONFIG_OF=y
+CONFIG_OF_UNITTEST=y
+CONFIG_OF_OVERLAY=y
+CONFIG_I2C=y
+CONFIG_I2C_MUX=y
+CONFIG_KUNIT=y
+CONFIG_KUNIT_TEST=y
+CONFIG_KUNIT_EXAMPLE_TEST=y
diff --git a/tools/testing/kunit/configs/all_tests.config 
b/tools/testing/kunit/configs/all_tests.config
new file mode 100644
index 0..bfe49689038f1
--- /dev/null
+++ b/tools/testing/kunit/configs/all_tests.config
@@ -0,0 +1,8 @@
+CONFIG_OF=y
+CONFIG_OF_UNITTEST=y
+CONFIG_OF_OVERLAY=y
+CONFIG_I2C=y
+CONFIG_I2C_MUX=y
+CONFIG_KUNIT=y
+CONFIG_KUNIT_TEST=y
+CONFIG_KUNIT_EXAMPLE_TEST=y
diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py
index da11bd62a4b82..0944dea5c3211 100755
--- a/tools/testing/kunit/kunit.py
+++ b/tools/testing/kunit/kunit.py
@@ -11,6 +11,7 @@ import argparse
 import sys
 import os
 import time
+import shutil
 
 from collections import namedtuple
 from enum import Enum, auto
@@ -21,7 +22,7 @@ import kunit_parser
 
 KunitResult = namedtuple('KunitResult', ['status','result'])
 
-KunitRequest = namedtuple('KunitRequest', ['raw_output','timeout', 'jobs', 
'build_dir'])
+KunitRequest = namedtuple('KunitRequest', ['raw_output','timeout', 'jobs', 
'build_dir', 'defconfig'])
 
 class KunitStatus(Enum):
SUCCESS = auto()
@@ -29,8 +30,16 @@ class KunitStatus(Enum):
BUILD_FAILURE = auto()
TEST_FAILURE = auto()
 
+def create_default_kunitconfig():
+   if not os.path.exists(kunit_kernel.KUNITCONFIG_PATH):
+   shutil.copyfile('arch/um/configs/kunit_defconfig',
+   kunit_kernel.KUNITCONFIG_PATH)
+
 def run_tests(linux: kunit_kernel.LinuxSourceTree,
  request: KunitRequest) -> KunitResult:
+   if request.defconfig:
+   create_default_kunitconfig()
+
config_start = time.time()
success = linux.build_reconfig(request.build_dir)
config_end = time.time()
@@ -72,7 +81,7 @@ def run_tests(linux: kunit_kernel.LinuxSourceTree,
else:
return KunitResult(KunitStatus.SUCCESS, test_result)
 
-def main(argv, linux):
+def main(argv, linux=None):
parser = argparse.ArgumentParser(
description='Helps writing and running KUnit tests.')
subparser = parser.add_subparsers(dest='subcommand')
@@ -99,13 +108,24 @@ def main(argv, linux):
'directory.',
type=str, default=None, metavar='build_dir')
 
+   run_parser.add_argument('--defconfig',
+   help='Uses a default kunitconfig.',
+   action='store_true')
+
cli_args = parser.parse_args(argv)
 
if cli_args.subcommand == 'run':
+   if cli_args.defconfig:
+   create_default_kunitconfig()
+
+   if not linux:
+   linux = kunit_kernel.LinuxSourceTree()
+
request = KunitRequest(cli_args.raw_output,
   cli_args.timeout,
   cli_args.jobs,
-  cli_args.build_dir)
+  cli_args.build_dir,
+  cli_args.defconfig)
result = run_tests(linux, request)
if result.status != KunitStatus.SUCCESS:
sys.exit(1)
@@ -113,4 +133,4 @@ def main(argv, linux):
parser.print_help()
 
 if __name__ == '__main__':
-   main(sys.argv[1:], kunit_kernel.LinuxSourceTree())
+   main(sys.argv[1:])
diff --git a/tools/testing/kunit/kunit_kernel.py 
b/tools/testing/kunit/kunit_kernel.py
index 07c0abf2f47df..bf38768353313 100644
--- a/tools/testing/kunit/kunit_kernel.py
+++ b/tools/testing/kunit/kunit_kernel.py
@@ -14,6 +14,7 @@ import os
 import kunit_config
 
 KCONFIG_PATH = '.config'
+KUNITCONFIG_PATH = 'kunitconfig'
 
 class ConfigError(Exception):
"""Represents an error trying to configure the Linux kernel."""
@@ -81,7 +82,7 @@ class LinuxSourceTree(object):
 
def __init__(self):
se

[PATCH v12 12/18] kunit: test: add tests for KUnit managed resources

From: Avinash Kondareddy 

Add unit tests for KUnit managed resources. KUnit managed resources
(struct kunit_resource) are resources that are automatically cleaned up
at the end of a KUnit test, similar to the concept of devm_* managed
resources.

Signed-off-by: Avinash Kondareddy 
Signed-off-by: Brendan Higgins 
Reviewed-by: Greg Kroah-Hartman 
Reviewed-by: Logan Gunthorpe 
---
 kunit/test-test.c | 225 ++
 1 file changed, 225 insertions(+)

diff --git a/kunit/test-test.c b/kunit/test-test.c
index 058f3fb37458a..725f1486376fa 100644
--- a/kunit/test-test.c
+++ b/kunit/test-test.c
@@ -101,3 +101,228 @@ static struct kunit_suite kunit_try_catch_test_suite = {
.test_cases = kunit_try_catch_test_cases,
 };
 kunit_test_suite(kunit_try_catch_test_suite);
+
+/*
+ * Context for testing test managed resources
+ * is_resource_initialized is used to test arbitrary resources
+ */
+struct kunit_test_resource_context {
+   struct kunit test;
+   bool is_resource_initialized;
+   int allocate_order[2];
+   int free_order[2];
+};
+
+static int fake_resource_init(struct kunit_resource *res, void *context)
+{
+   struct kunit_test_resource_context *ctx = context;
+
+   res->allocation = &ctx->is_resource_initialized;
+   ctx->is_resource_initialized = true;
+   return 0;
+}
+
+static void fake_resource_free(struct kunit_resource *res)
+{
+   bool *is_resource_initialized = res->allocation;
+
+   *is_resource_initialized = false;
+}
+
+static void kunit_resource_test_init_resources(struct kunit *test)
+{
+   struct kunit_test_resource_context *ctx = test->priv;
+
+   kunit_init_test(&ctx->test, "testing_test_init_test");
+
+   KUNIT_EXPECT_TRUE(test, list_empty(&ctx->test.resources));
+}
+
+static void kunit_resource_test_alloc_resource(struct kunit *test)
+{
+   struct kunit_test_resource_context *ctx = test->priv;
+   struct kunit_resource *res;
+   kunit_resource_free_t free = fake_resource_free;
+
+   res = kunit_alloc_and_get_resource(&ctx->test,
+  fake_resource_init,
+  fake_resource_free,
+  GFP_KERNEL,
+  ctx);
+
+   KUNIT_ASSERT_NOT_ERR_OR_NULL(test, res);
+   KUNIT_EXPECT_PTR_EQ(test,
+   &ctx->is_resource_initialized,
+   (bool *) res->allocation);
+   KUNIT_EXPECT_TRUE(test, list_is_last(&res->node, &ctx->test.resources));
+   KUNIT_EXPECT_PTR_EQ(test, free, res->free);
+}
+
+static void kunit_resource_test_free_resource(struct kunit *test)
+{
+   struct kunit_test_resource_context *ctx = test->priv;
+   struct kunit_resource *res = kunit_alloc_and_get_resource(
+   &ctx->test,
+   fake_resource_init,
+   fake_resource_free,
+   GFP_KERNEL,
+   ctx);
+
+   kunit_free_resource(&ctx->test, res);
+
+   KUNIT_EXPECT_FALSE(test, ctx->is_resource_initialized);
+   KUNIT_EXPECT_TRUE(test, list_empty(&ctx->test.resources));
+}
+
+static void kunit_resource_test_cleanup_resources(struct kunit *test)
+{
+   int i;
+   struct kunit_test_resource_context *ctx = test->priv;
+   struct kunit_resource *resources[5];
+
+   for (i = 0; i < ARRAY_SIZE(resources); i++) {
+   resources[i] = kunit_alloc_and_get_resource(&ctx->test,
+   fake_resource_init,
+   fake_resource_free,
+   GFP_KERNEL,
+   ctx);
+   }
+
+   kunit_cleanup(&ctx->test);
+
+   KUNIT_EXPECT_TRUE(test, list_empty(&ctx->test.resources));
+}
+
+static void kunit_resource_test_mark_order(int order_array[],
+  size_t order_size,
+  int key)
+{
+   int i;
+
+   for (i = 0; i < order_size && order_array[i]; i++)
+   ;
+
+   order_array[i] = key;
+}
+
+#define KUNIT_RESOURCE_TEST_MARK_ORDER(ctx, order_field, key) \
+   kunit_resource_test_mark_order(ctx->order_field,   \
+  ARRAY_SIZE(ctx->order_field),   \
+  key)
+
+static int fake_resource_2_init(struct kunit_resource *res, void *context)
+{
+   struct kunit_test_resource_context *ctx = context;
+
+   KUNIT_RESOURCE_TEST_MARK_ORDER(ctx, allocate_order, 2);
+
+   res->allocation = ctx;
+
+   return 0;
+}
+
+static void fake_resource_2_free(struct kunit_resource *res)
+{
+   struct kunit_test_resource_context *ctx = res->allocation;
+
+   KUNIT_RESOURCE_

[PATCH v12 08/18] objtool: add kunit_try_catch_throw to the noreturn list

Fix the following warning seen on GCC 7.3:
  kunit/test-test.o: warning: objtool: kunit_test_unsuccessful_try() falls 
through to next function kunit_test_catch()

kunit_try_catch_throw is a function added in the following patch in this
series; it allows KUnit, a unit testing framework for the kernel, to
bail out of a broken test. As a consequence, it is a new __noreturn
function that objtool thinks is broken (as seen above). So fix this
warning by adding kunit_try_catch_throw to objtool's noreturn list.

Reported-by: kbuild test robot 
Signed-off-by: Brendan Higgins 
Acked-by: Josh Poimboeuf 
Link: https://www.spinics.net/lists/linux-kbuild/msg21708.html
Cc: Peter Zijlstra 
---
 tools/objtool/check.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 176f2f0840609..0c8e17f946cda 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -145,6 +145,7 @@ static bool __dead_end_function(struct objtool_file *file, 
struct symbol *func,
"usercopy_abort",
"machine_real_restart",
"rewind_stack_do_exit",
+   "kunit_try_catch_throw",
};
 
if (!func)
-- 
2.23.0.rc1.153.gdeed80330f-goog

___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH v12 05/18] kunit: test: add the concept of expectations

Add support for expectations, which allow properties to be specified and
then verified in tests.

Signed-off-by: Brendan Higgins 
Reviewed-by: Greg Kroah-Hartman 
Reviewed-by: Logan Gunthorpe 
---
 include/kunit/test.h | 843 +++
 kunit/test.c |  58 +++
 2 files changed, 901 insertions(+)

diff --git a/include/kunit/test.h b/include/kunit/test.h
index d0bf112910caf..2625bcfeb19ac 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -9,8 +9,10 @@
 #ifndef _KUNIT_TEST_H
 #define _KUNIT_TEST_H
 
+#include 
 #include 
 #include 
+#include 
 
 struct kunit_resource;
 
@@ -319,4 +321,845 @@ void __printf(3, 4) kunit_printk(const char *level,
 #define kunit_err(test, fmt, ...) \
kunit_printk(KERN_ERR, test, fmt, ##__VA_ARGS__)
 
+/*
+ * Generates a compile-time warning in case of comparing incompatible types.
+ */
+#define __kunit_typecheck(lhs, rhs) \
+   ((void) __typecheck(lhs, rhs))
+
+/**
+ * KUNIT_SUCCEED() - A no-op expectation. Only exists for code clarity.
+ * @test: The test context object.
+ *
+ * The opposite of KUNIT_FAIL(), it is an expectation that cannot fail. In 
other
+ * words, it does nothing and only exists for code clarity. See
+ * KUNIT_EXPECT_TRUE() for more information.
+ */
+#define KUNIT_SUCCEED(test) do {} while (0)
+
+void kunit_do_assertion(struct kunit *test,
+   struct kunit_assert *assert,
+   bool pass,
+   const char *fmt, ...);
+
+#define KUNIT_ASSERTION(test, pass, assert_class, INITIALIZER, fmt, ...) do {  
\
+   struct assert_class __assertion = INITIALIZER; \
+   kunit_do_assertion(test,   \
+  &__assertion.assert,\
+  pass,   \
+  fmt,\
+  ##__VA_ARGS__); \
+} while (0)
+
+
+#define KUNIT_FAIL_ASSERTION(test, assert_type, fmt, ...) \
+   KUNIT_ASSERTION(test,  \
+   false, \
+   kunit_fail_assert, \
+   KUNIT_INIT_FAIL_ASSERT_STRUCT(test,\
+ assert_type),\
+   fmt,   \
+   ##__VA_ARGS__)
+
+/**
+ * KUNIT_FAIL() - Always causes a test to fail when evaluated.
+ * @test: The test context object.
+ * @fmt: an informational message to be printed when the assertion is made.
+ * @...: string format arguments.
+ *
+ * The opposite of KUNIT_SUCCEED(), it is an expectation that always fails. In
+ * other words, it always results in a failed expectation, and consequently
+ * always causes the test case to fail when evaluated. See KUNIT_EXPECT_TRUE()
+ * for more information.
+ */
+#define KUNIT_FAIL(test, fmt, ...)\
+   KUNIT_FAIL_ASSERTION(test, \
+KUNIT_EXPECTATION,\
+fmt,  \
+##__VA_ARGS__)
+
+#define KUNIT_UNARY_ASSERTION(test,   \
+ assert_type, \
+ condition,   \
+ expected_true,   \
+ fmt, \
+ ...) \
+   KUNIT_ASSERTION(test,  \
+   !!(condition) == !!expected_true,  \
+   kunit_unary_assert,\
+   KUNIT_INIT_UNARY_ASSERT_STRUCT(test,   \
+  assert_type,\
+  #condition, \
+  expected_true), \
+   fmt,   \
+   ##__VA_ARGS__)
+
+#define KUNIT_TRUE_MSG_ASSERTION(test, assert_type, condition, fmt, ...)   
\
+   KUNIT_UNARY_ASSERTION(test,\
+ assert_type,

[PATCH v12 16/18] MAINTAINERS: add entry for KUnit the unit testing framework

Add myself as maintainer of KUnit, the Linux kernel's unit testing
framework.

Signed-off-by: Brendan Higgins 
Reviewed-by: Greg Kroah-Hartman 
Reviewed-by: Logan Gunthorpe 
---
 MAINTAINERS | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index a2c343ee3b2ca..f0bd77e8a8a2f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8799,6 +8799,17 @@ S:   Maintained
 F: tools/testing/selftests/
 F: Documentation/dev-tools/kselftest*
 
+KERNEL UNIT TESTING FRAMEWORK (KUnit)
+M: Brendan Higgins 
+L: linux-kselft...@vger.kernel.org
+L: kunit-...@googlegroups.com
+W: https://google.github.io/kunit-docs/third_party/kernel/docs/
+S: Maintained
+F: Documentation/dev-tools/kunit/
+F: include/kunit/
+F: kunit/
+F: tools/testing/kunit/
+
 KERNEL USERMODE HELPER
 M: Luis Chamberlain 
 L: linux-ker...@vger.kernel.org
-- 
2.23.0.rc1.153.gdeed80330f-goog

___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH v12 01/18] kunit: test: add KUnit test runner core

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 
Reviewed-by: Greg Kroah-Hartman 
Reviewed-by: Logan Gunthorpe 
Reviewed-by: Luis Chamberlain 
Reviewed-by: Stephen Boyd 
---
 include/kunit/test.h | 179 
 kunit/Kconfig|  17 
 kunit/Makefile   |   1 +
 kunit/test.c | 191 +++
 4 files changed, 388 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 0..e0b34acb9ee4e
--- /dev/null
+++ b/include/kunit/test.h
@@ -0,0 +1,179 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Base unit test (KUnit) API.
+ *
+ * Copyright (C) 2019, Google LLC.
+ * Author: Brendan Higgins 
+ */
+
+#ifndef _KUNIT_TEST_H
+#define _KUNIT_TEST_H
+
+#include 
+
+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.
+ */
+struct kunit {
+   void *priv;
+
+   /* private: internal use only. */
+   const char *name; /* Read only after initialization! */
+   /*
+* success starts as true, and may only be set to false during a test
+* case; thus, it is safe to update this across multiple threads using
+* WRITE_ONCE; however, as a consequence, it may only be read after the
+* test case finishes once all threads associated with the test case
+* have terminated.
+

[PATCH v12 10/18] kunit: test: add tests for kunit test abort

Add KUnit tests for the KUnit test abort mechanism (see preceding
commit). Add tests both for general try catch mechanism as well as
non-architecture specific mechanism.

Signed-off-by: Brendan Higgins 
Reviewed-by: Greg Kroah-Hartman 
Reviewed-by: Logan Gunthorpe 
---
 kunit/Makefile|   3 +-
 kunit/test-test.c | 101 ++
 2 files changed, 103 insertions(+), 1 deletion(-)
 create mode 100644 kunit/test-test.c

diff --git a/kunit/Makefile b/kunit/Makefile
index c9176c9c578c6..769d9402b5d3a 100644
--- a/kunit/Makefile
+++ b/kunit/Makefile
@@ -3,6 +3,7 @@ obj-$(CONFIG_KUNIT) +=  test.o \
assert.o \
try-catch.o
 
-obj-$(CONFIG_KUNIT_TEST) +=string-stream-test.o
+obj-$(CONFIG_KUNIT_TEST) +=test-test.o \
+   string-stream-test.o
 
 obj-$(CONFIG_KUNIT_EXAMPLE_TEST) +=example-test.o
diff --git a/kunit/test-test.c b/kunit/test-test.c
new file mode 100644
index 0..88f4cdf03db2a
--- /dev/null
+++ b/kunit/test-test.c
@@ -0,0 +1,101 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * KUnit test for core test infrastructure.
+ *
+ * Copyright (C) 2019, Google LLC.
+ * Author: Brendan Higgins 
+ */
+#include 
+
+struct kunit_try_catch_test_context {
+   struct kunit_try_catch *try_catch;
+   bool function_called;
+};
+
+void kunit_test_successful_try(void *data)
+{
+   struct kunit *test = data;
+   struct kunit_try_catch_test_context *ctx = test->priv;
+
+   ctx->function_called = true;
+}
+
+void kunit_test_no_catch(void *data)
+{
+   struct kunit *test = data;
+
+   KUNIT_FAIL(test, "Catch should not be called\n");
+}
+
+static void kunit_test_try_catch_successful_try_no_catch(struct kunit *test)
+{
+   struct kunit_try_catch_test_context *ctx = test->priv;
+   struct kunit_try_catch *try_catch = ctx->try_catch;
+
+   kunit_try_catch_init(try_catch,
+test,
+kunit_test_successful_try,
+kunit_test_no_catch);
+   kunit_try_catch_run(try_catch, test);
+
+   KUNIT_EXPECT_TRUE(test, ctx->function_called);
+}
+
+void kunit_test_unsuccessful_try(void *data)
+{
+   struct kunit *test = data;
+   struct kunit_try_catch_test_context *ctx = test->priv;
+   struct kunit_try_catch *try_catch = ctx->try_catch;
+
+   kunit_try_catch_throw(try_catch);
+   KUNIT_FAIL(test, "This line should never be reached\n");
+}
+
+void kunit_test_catch(void *data)
+{
+   struct kunit *test = data;
+   struct kunit_try_catch_test_context *ctx = test->priv;
+
+   ctx->function_called = true;
+}
+
+static void kunit_test_try_catch_unsuccessful_try_does_catch(struct kunit 
*test)
+{
+   struct kunit_try_catch_test_context *ctx = test->priv;
+   struct kunit_try_catch *try_catch = ctx->try_catch;
+
+   kunit_try_catch_init(try_catch,
+test,
+kunit_test_unsuccessful_try,
+kunit_test_catch);
+   kunit_try_catch_run(try_catch, test);
+
+   KUNIT_EXPECT_TRUE(test, ctx->function_called);
+}
+
+static int kunit_try_catch_test_init(struct kunit *test)
+{
+   struct kunit_try_catch_test_context *ctx;
+
+   ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
+   test->priv = ctx;
+
+   ctx->try_catch = kunit_kmalloc(test,
+  sizeof(*ctx->try_catch),
+  GFP_KERNEL);
+
+   return 0;
+}
+
+static struct kunit_case kunit_try_catch_test_cases[] = {
+   KUNIT_CASE(kunit_test_try_catch_successful_try_no_catch),
+   KUNIT_CASE(kunit_test_try_catch_unsuccessful_try_does_catch),
+   {}
+};
+
+static struct kunit_suite kunit_try_catch_test_suite = {
+   .name = "kunit-try-catch-test",
+   .init = kunit_try_catch_test_init,
+   .test_cases = kunit_try_catch_test_cases,
+};
+kunit_test_suite(kunit_try_catch_test_suite);
-- 
2.23.0.rc1.153.gdeed80330f-goog

___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH v12 03/18] kunit: test: add string_stream a std::stream like string builder

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. string_stream is really just a string builder,
nothing more.

Signed-off-by: Brendan Higgins 
Reviewed-by: Greg Kroah-Hartman 
Reviewed-by: Logan Gunthorpe 
---
 include/kunit/string-stream.h |  49 +++
 kunit/Makefile|   3 +-
 kunit/string-stream.c | 152 ++
 3 files changed, 203 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 0..4fa107e38deb5
--- /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 
+ */
+
+#ifndef _KUNIT_STRING_STREAM_H
+#define _KUNIT_STRING_STREAM_H
+
+#include 
+#include 
+#include 
+
+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 *test;
+   gfp_t gfp;
+};
+
+struct kunit;
+
+struct string_stream *alloc_string_stream(struct kunit *test, gfp_t gfp);
+
+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);
+
+int string_stream_append(struct string_stream *stream,
+struct string_stream *other);
+
+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 0..bcd56d6755544
--- /dev/null
+++ b/kunit/string-stream.c
@@ -0,0 +1,152 @@
+// 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 
+ */
+
+#include 
+#include 
+#include 
+#include 
+
+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;
+
+   /* 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 = kunit_kmalloc(stream->test, sizeof(*frag_container),
+  stream->gfp);
+   if (!frag_container)
+   return -ENOMEM;
+
+   frag_container->fragment = kunit_kmalloc(stream->test, len,
+stream->gfp);
+   if (!frag_container->fragment)
+   return -ENOMEM;
+
+   len = vsnprintf(frag_container->fragment, len, fmt, args);
+   spin_lock(&stream->lock);
+   stream->length += len;
+   list_add_tail(&frag_container->node, &stream->fragments);
+   spin_unlock(&stream->lock);
+
+   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;
+
+   spin_lock(&stream->lock);
+   list_for_each_entry_safe(frag_container,
+frag_container_safe,
+&stream->fragments,
+node) {
+   list_del(&frag_container->node);
+   }
+   stream->length = 0;
+   spin_unlock(&stream->lock);
+}
+
+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;
+
+   buf = k

[PATCH v12 11/18] kunit: test: add the concept of assertions

Add support for assertions which are like expectations except the test
terminates if the assertion is not satisfied.

The idea with assertions is that you use them to state all the
preconditions for your test. Logically speaking, these are the premises
of the test case, so if a premise isn't true, there is no point in
continuing the test case because there are no conclusions that can be
drawn without the premises. Whereas, the expectation is the thing you
are trying to prove. It is not used universally in x-unit style test
frameworks, but I really like it as a convention.  You could still
express the idea of a premise using the above idiom, but I think
KUNIT_ASSERT_* states the intended idea perfectly.

Signed-off-by: Brendan Higgins 
Reviewed-by: Greg Kroah-Hartman 
Reviewed-by: Logan Gunthorpe 
---
 include/kunit/test.h   | 297 -
 kunit/string-stream-test.c |  12 +-
 kunit/test-test.c  |   2 +
 3 files changed, 302 insertions(+), 9 deletions(-)

diff --git a/include/kunit/test.h b/include/kunit/test.h
index 93381f841e09f..dcebaca456a1f 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -86,9 +86,10 @@ struct kunit;
  * @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.
+ * that makes expectations and assertions (see KUNIT_EXPECT_TRUE() and
+ * KUNIT_ASSERT_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
@@ -1176,4 +1177,294 @@ do {
   \
fmt,   \
##__VA_ARGS__)
 
+#define KUNIT_ASSERT_FAILURE(test, fmt, ...) \
+   KUNIT_FAIL_ASSERTION(test, KUNIT_ASSERTION, fmt, ##__VA_ARGS__)
+
+/**
+ * KUNIT_ASSERT_TRUE() - Sets an assertion that @condition is true.
+ * @test: The test context object.
+ * @condition: an arbitrary boolean expression. The test fails and aborts when
+ * this does not evaluate to true.
+ *
+ * This and assertions of the form `KUNIT_ASSERT_*` will cause the test case to
+ * fail *and immediately abort* when the specified condition is not met. Unlike
+ * an expectation failure, it will prevent the test case from continuing to 
run;
+ * this is otherwise known as an *assertion failure*.
+ */
+#define KUNIT_ASSERT_TRUE(test, condition) \
+   KUNIT_TRUE_ASSERTION(test, KUNIT_ASSERTION, condition)
+
+#define KUNIT_ASSERT_TRUE_MSG(test, condition, fmt, ...)  \
+   KUNIT_TRUE_MSG_ASSERTION(test, \
+KUNIT_ASSERTION,  \
+condition,\
+fmt,  \
+##__VA_ARGS__)
+
+/**
+ * KUNIT_ASSERT_FALSE() - Sets an assertion that @condition is false.
+ * @test: The test context object.
+ * @condition: an arbitrary boolean expression.
+ *
+ * Sets an assertion that the value that @condition evaluates to is false. This
+ * is the same as KUNIT_EXPECT_FALSE(), except it causes an assertion failure
+ * (see KUNIT_ASSERT_TRUE()) when the assertion is not met.
+ */
+#define KUNIT_ASSERT_FALSE(test, condition)   \
+   KUNIT_FALSE_ASSERTION(test, KUNIT_ASSERTION, condition)
+
+#define KUNIT_ASSERT_FALSE_MSG(test, condition, fmt, ...) \
+   KUNIT_FALSE_MSG_ASSERTION(test,\
+ KUNIT_ASSERTION, \
+ condition,   \
+ fmt, \
+ ##__VA_ARGS__)
+
+/**
+ * KUNIT_ASSERT_EQ() - Sets an assertion that @left and @right are equal.
+ * @test: The test context object.
+ * @left: an arbitrary expression that evaluates to a primitive C type.
+ * @right: an arbitrary expression that evaluates to a primitive C type.
+ *
+ * Sets an assertion that the values that @left and @right evaluate to are
+ * equal. This is the same as KUNIT_EXPECT_EQ(), except it causes an assertion
+ * failure (see KUNIT_ASSERT_TRUE()) when the assertion is not me

[PATCH v12 04/18] kunit: test: add assertion printing library

Add `struct kunit_assert` and friends which provide a structured way to
capture data from an expectation or an assertion (introduced later in
the series) so that it may be printed out in the event of a failure.

Signed-off-by: Brendan Higgins 
---
 include/kunit/assert.h | 183 +
 kunit/Makefile |   3 +-
 kunit/assert.c | 141 +++
 3 files changed, 326 insertions(+), 1 deletion(-)
 create mode 100644 include/kunit/assert.h
 create mode 100644 kunit/assert.c

diff --git a/include/kunit/assert.h b/include/kunit/assert.h
new file mode 100644
index 0..55f1b88b0cb4d
--- /dev/null
+++ b/include/kunit/assert.h
@@ -0,0 +1,183 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Assertion and expectation serialization API.
+ *
+ * Copyright (C) 2019, Google LLC.
+ * Author: Brendan Higgins 
+ */
+
+#ifndef _KUNIT_ASSERT_H
+#define _KUNIT_ASSERT_H
+
+#include 
+#include 
+
+struct kunit;
+
+enum kunit_assert_type {
+   KUNIT_ASSERTION,
+   KUNIT_EXPECTATION,
+};
+
+struct kunit_assert {
+   struct kunit *test;
+   enum kunit_assert_type type;
+   int line;
+   const char *file;
+   struct va_format message;
+   void (*format)(const struct kunit_assert *assert,
+  struct string_stream *stream);
+};
+
+#define KUNIT_INIT_VA_FMT_NULL { .fmt = NULL, .va = NULL }
+
+#define KUNIT_INIT_ASSERT_STRUCT(kunit, assert_type, fmt) {   \
+   .test = kunit, \
+   .type = assert_type,   \
+   .file = __FILE__,  \
+   .line = __LINE__,  \
+   .message = KUNIT_INIT_VA_FMT_NULL, \
+   .format = fmt  \
+}
+
+void kunit_base_assert_format(const struct kunit_assert *assert,
+ struct string_stream *stream);
+
+void kunit_assert_print_msg(const struct kunit_assert *assert,
+   struct string_stream *stream);
+
+struct kunit_fail_assert {
+   struct kunit_assert assert;
+};
+
+void kunit_fail_assert_format(const struct kunit_assert *assert,
+ struct string_stream *stream);
+
+#define KUNIT_INIT_FAIL_ASSERT_STRUCT(test, type) {   \
+   .assert = KUNIT_INIT_ASSERT_STRUCT(test,   \
+  type,   \
+  kunit_fail_assert_format)   \
+}
+
+struct kunit_unary_assert {
+   struct kunit_assert assert;
+   const char *condition;
+   bool expected_true;
+};
+
+void kunit_unary_assert_format(const struct kunit_assert *assert,
+  struct string_stream *stream);
+
+#define KUNIT_INIT_UNARY_ASSERT_STRUCT(test, type, cond, expect_true) {
   \
+   .assert = KUNIT_INIT_ASSERT_STRUCT(test,   \
+  type,   \
+  kunit_unary_assert_format), \
+   .condition = cond, \
+   .expected_true = expect_true   \
+}
+
+struct kunit_ptr_not_err_assert {
+   struct kunit_assert assert;
+   const char *text;
+   const void *value;
+};
+
+void kunit_ptr_not_err_assert_format(const struct kunit_assert *assert,
+struct string_stream *stream);
+
+#define KUNIT_INIT_PTR_NOT_ERR_STRUCT(test, type, txt, val) { \
+   .assert = KUNIT_INIT_ASSERT_STRUCT(test,   \
+  type,   \
+  kunit_ptr_not_err_assert_format),   \
+   .text = txt,   \
+   .value = val   \
+}
+
+struct kunit_binary_assert {
+   struct kunit_assert assert;
+   const char *operation;
+   const char *left_text;
+   long long left_value;
+   const char *right_text;
+   long long right_value;
+};
+
+void kunit_binary_assert_format(const struct kunit_assert *assert,
+   struct string_stream *stream);
+
+#define KUNIT_INIT_BINARY_ASSERT_STRUCT(test, \
+   type,  \
+   op_str,\
+   left_str,  \
+

[PATCH v12 07/18] kunit: test: add initial tests

Add a test for string stream along with a simpler example.

Signed-off-by: Brendan Higgins 
Reviewed-by: Greg Kroah-Hartman 
Reviewed-by: Logan Gunthorpe 
---
 kunit/Kconfig  | 21 +
 kunit/Makefile |  4 ++
 kunit/example-test.c   | 88 ++
 kunit/string-stream-test.c | 74 
 4 files changed, 187 insertions(+)
 create mode 100644 kunit/example-test.c
 create mode 100644 kunit/string-stream-test.c

diff --git a/kunit/Kconfig b/kunit/Kconfig
index 330ae83527c23..8541ef95b65ad 100644
--- a/kunit/Kconfig
+++ b/kunit/Kconfig
@@ -14,4 +14,25 @@ config KUNIT
  architectures. For more information, please see
  Documentation/dev-tools/kunit/.
 
+config KUNIT_TEST
+   bool "KUnit test for KUnit"
+   depends on KUNIT
+   help
+ Enables the unit tests for the KUnit test framework. These tests test
+ the KUnit test framework itself; the tests are both written using
+ KUnit and test KUnit. This option should only be enabled for testing
+ purposes by developers interested in testing that KUnit works as
+ expected.
+
+config KUNIT_EXAMPLE_TEST
+   bool "Example test for KUnit"
+   depends on KUNIT
+   help
+ Enables an example unit test that illustrates some of the basic
+ features of KUnit. This test only exists to help new users understand
+ what KUnit is and how it is used. Please refer to the example test
+ itself, kunit/example-test.c, for more information. This option is
+ intended for curious hackers who would like to understand how to use
+ KUnit for kernel development.
+
 endmenu
diff --git a/kunit/Makefile b/kunit/Makefile
index 6dcbe309036b8..4e46450bcb3a8 100644
--- a/kunit/Makefile
+++ b/kunit/Makefile
@@ -1,3 +1,7 @@
 obj-$(CONFIG_KUNIT) += test.o \
string-stream.o \
assert.o
+
+obj-$(CONFIG_KUNIT_TEST) +=string-stream-test.o
+
+obj-$(CONFIG_KUNIT_EXAMPLE_TEST) +=example-test.o
diff --git a/kunit/example-test.c b/kunit/example-test.c
new file mode 100644
index 0..f64a829aa441f
--- /dev/null
+++ b/kunit/example-test.c
@@ -0,0 +1,88 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Example KUnit test to show how to use KUnit.
+ *
+ * Copyright (C) 2019, Google LLC.
+ * Author: Brendan Higgins 
+ */
+
+#include 
+
+/*
+ * This is the most fundamental element of KUnit, the test case. A test case
+ * makes a set EXPECTATIONs and ASSERTIONs about the behavior of some code; if
+ * any expectations or assertions are not met, the test fails; otherwise, the
+ * test passes.
+ *
+ * In KUnit, a test case is just a function with the signature
+ * `void (*)(struct kunit *)`. `struct kunit` is a context object that stores
+ * information about the current test.
+ */
+static void example_simple_test(struct kunit *test)
+{
+   /*
+* This is an EXPECTATION; it is how KUnit tests things. When you want
+* to test a piece of code, you set some expectations about what the
+* code should do. KUnit then runs the test and verifies that the code's
+* behavior matched what was expected.
+*/
+   KUNIT_EXPECT_EQ(test, 1 + 1, 2);
+}
+
+/*
+ * This is run once before each test case, see the comment on
+ * example_test_suite for more information.
+ */
+static int example_test_init(struct kunit *test)
+{
+   kunit_info(test, "initializing\n");
+
+   return 0;
+}
+
+/*
+ * Here we make a list of all the test cases we want to add to the test suite
+ * below.
+ */
+static struct kunit_case example_test_cases[] = {
+   /*
+* This is a helper to create a test case object from a test case
+* function; its exact function is not important to understand how to
+* use KUnit, just know that this is how you associate test cases with a
+* test suite.
+*/
+   KUNIT_CASE(example_simple_test),
+   {}
+};
+
+/*
+ * This defines a suite or grouping of tests.
+ *
+ * Test cases are defined as belonging to the suite by adding them to
+ * `kunit_cases`.
+ *
+ * Often it is desirable to run some function which will set up things which
+ * will be used by every test; this is accomplished with an `init` function
+ * which runs before each test case is invoked. Similarly, an `exit` function
+ * may be specified which runs after every test case and can be used to for
+ * cleanup. For clarity, running tests in a test suite would behave as follows:
+ *
+ * suite.init(test);
+ * suite.test_case[0](test);
+ * suite.exit(test);
+ * suite.init(test);
+ * suite.test_case[1](test);
+ * suite.exit(test);
+ * ...;
+ */
+static struct kunit_suite example_test_suite = {
+   .name = "example",
+   .init = example_test_init,
+   .test_cases = example_test_cases,
+};
+
+/*
+ * This registers the above test suite tellin

[PATCH v12 17/18] kernel/sysctl-test: Add null pointer test for sysctl.c:proc_dointvec()

From: Iurii Zaikin 

KUnit tests for initialized data behavior of proc_dointvec that is
explicitly checked in the code. Includes basic parsing tests including
int min/max overflow.

Signed-off-by: Iurii Zaikin 
Signed-off-by: Brendan Higgins 
Reviewed-by: Greg Kroah-Hartman 
Reviewed-by: Logan Gunthorpe 
Acked-by: Luis Chamberlain 
---
 kernel/Makefile  |   2 +
 kernel/sysctl-test.c | 392 +++
 lib/Kconfig.debug|  11 ++
 3 files changed, 405 insertions(+)
 create mode 100644 kernel/sysctl-test.c

diff --git a/kernel/Makefile b/kernel/Makefile
index ef0d95a190b41..63e9ea6122c2c 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -113,6 +113,8 @@ obj-$(CONFIG_TORTURE_TEST) += torture.o
 obj-$(CONFIG_HAS_IOMEM) += iomem.o
 obj-$(CONFIG_RSEQ) += rseq.o
 
+obj-$(CONFIG_SYSCTL_KUNIT_TEST) += sysctl-test.o
+
 obj-$(CONFIG_GCC_PLUGIN_STACKLEAK) += stackleak.o
 KASAN_SANITIZE_stackleak.o := n
 KCOV_INSTRUMENT_stackleak.o := n
diff --git a/kernel/sysctl-test.c b/kernel/sysctl-test.c
new file mode 100644
index 0..2a63241a8453b
--- /dev/null
+++ b/kernel/sysctl-test.c
@@ -0,0 +1,392 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * KUnit test of proc sysctl.
+ */
+
+#include 
+#include 
+
+#define KUNIT_PROC_READ 0
+#define KUNIT_PROC_WRITE 1
+
+static int i_zero;
+static int i_one_hundred = 100;
+
+/*
+ * Test that proc_dointvec will not try to use a NULL .data field even when the
+ * length is non-zero.
+ */
+static void sysctl_test_api_dointvec_null_tbl_data(struct kunit *test)
+{
+   struct ctl_table null_data_table = {
+   .procname = "foo",
+   /*
+* Here we are testing that proc_dointvec behaves correctly when
+* we give it a NULL .data field. Normally this would point to a
+* piece of memory where the value would be stored.
+*/
+   .data   = NULL,
+   .maxlen = sizeof(int),
+   .mode   = 0644,
+   .proc_handler   = proc_dointvec,
+   .extra1 = &i_zero,
+   .extra2 = &i_one_hundred,
+   };
+   /*
+* proc_dointvec expects a buffer in user space, so we allocate one. We
+* also need to cast it to __user so sparse doesn't get mad.
+*/
+   void __user *buffer = (void __user *)kunit_kzalloc(test, sizeof(int),
+  GFP_USER);
+   size_t len;
+   loff_t pos;
+
+   /*
+* We don't care what the starting length is since proc_dointvec should
+* not try to read because .data is NULL.
+*/
+   len = 1234;
+   KUNIT_EXPECT_EQ(test, 0, proc_dointvec(&null_data_table,
+  KUNIT_PROC_READ, buffer, &len,
+  &pos));
+   KUNIT_EXPECT_EQ(test, (size_t)0, len);
+
+   /*
+* See above.
+*/
+   len = 1234;
+   KUNIT_EXPECT_EQ(test, 0, proc_dointvec(&null_data_table,
+  KUNIT_PROC_WRITE, buffer, &len,
+  &pos));
+   KUNIT_EXPECT_EQ(test, (size_t)0, len);
+}
+
+/*
+ * Similar to the previous test, we create a struct ctrl_table that has a .data
+ * field that proc_dointvec cannot do anything with; however, this time it is
+ * because we tell proc_dointvec that the size is 0.
+ */
+static void sysctl_test_api_dointvec_table_maxlen_unset(struct kunit *test)
+{
+   int data = 0;
+   struct ctl_table data_maxlen_unset_table = {
+   .procname = "foo",
+   .data   = &data,
+   /*
+* So .data is no longer NULL, but we tell proc_dointvec its
+* length is 0, so it still shouldn't try to use it.
+*/
+   .maxlen = 0,
+   .mode   = 0644,
+   .proc_handler   = proc_dointvec,
+   .extra1 = &i_zero,
+   .extra2 = &i_one_hundred,
+   };
+   void __user *buffer = (void __user *)kunit_kzalloc(test, sizeof(int),
+  GFP_USER);
+   size_t len;
+   loff_t pos;
+
+   /*
+* As before, we don't care what buffer length is because proc_dointvec
+* cannot do anything because its internal .data buffer has zero length.
+*/
+   len = 1234;
+   KUNIT_EXPECT_EQ(test, 0, proc_dointvec(&data_maxlen_unset_table,
+  KUNIT_PROC_READ, buffer, &len,
+  &pos));
+   KUNIT_EXPECT_EQ(test, (size_t)0, len);
+
+   /*
+* See previous comment.
+*/
+   len = 1234;
+   KUNIT_EXPECT_EQ(test, 0, proc_dointvec(&data_maxlen_unset_table,
+  KU

[PATCH v12 02/18] kunit: test: add test resource management API

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 
Reviewed-by: Greg Kroah-Hartman 
Reviewed-by: Logan Gunthorpe 
---
 include/kunit/test.h | 143 +++
 kunit/test.c |  92 
 2 files changed, 235 insertions(+)

diff --git a/include/kunit/test.h b/include/kunit/test.h
index e0b34acb9ee4e..d0bf112910caf 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -10,6 +10,70 @@
 #define _KUNIT_TEST_H
 
 #include 
+#include 
+
+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;
 
@@ -109,6 +173,13 @@ struct kunit {
 * have terminated.
 */
bool success; /* Read only after test_case finishes! */
+   spinlock_t lock; /* Gaurds all mutable test state. */
+   /*
+* Because resources is a list that may be updated multiple times (with
+* new resources) from any thread associated with a test case, we must
+* protect it with some type of lock.
+*/
+   struct list_head resources; /* Protected by lock. */
 };
 
 void kunit_init_test(struct kunit *test, const char *name);
@@ -141,6 +212,78 @@ int kunit_run_tests(struct kunit_suite *suite);
}  \
late_initcall(kunit_suite_init##suite)
 
+/*
+ * Like kunit_alloc_resource() below, but returns the struct kunit_resource
+ * object that contains the allocation. This is mostly for testing purposes.
+ */
+struct kunit_resource *kunit_alloc_and_get_resource(struct kunit *test,
+   kunit_resource_init_t init,
+   kunit_resource_free_t free,
+   gfp_t internal_gfp,
+   void *context);
+
+/**
+ * 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.
+ * @internal_gfp: gfp to use for internal allocations, if unsure, use 
GFP_KERNEL
+ * @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.
+ *
+ * NOTE: KUnit needs to allocate memory for each kunit_resource object. You 
must
+ * specify an @internal_gfp that is compatible with the use context of your
+ * resource.
+ */
+static inline void *kunit_alloc_resource(struct kunit *test,
+kunit_resource_init_t init,
+kunit_resource_free_t free,
+

[PATCH v12 06/18] kbuild: enable building KUnit

KUnit is a new unit testing framework for the kernel and when used is
built into the kernel as a part of it. Add KUnit to the root Kconfig and
Makefile to allow it to be actually built.

Signed-off-by: Brendan Higgins 
Acked-by: Masahiro Yamada 
Cc: Michal Marek 
Reviewed-by: Greg Kroah-Hartman 
Reviewed-by: Logan Gunthorpe 
Reviewed-by: Stephen Boyd 
---
 Kconfig  | 2 ++
 Makefile | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/Kconfig b/Kconfig
index e10b3ee084d4d..47886dbd6c2a6 100644
--- a/Kconfig
+++ b/Kconfig
@@ -32,3 +32,5 @@ source "lib/Kconfig"
 source "lib/Kconfig.debug"
 
 source "Documentation/Kconfig"
+
+source "kunit/Kconfig"
diff --git a/Makefile b/Makefile
index 23cdf1f413646..3795d0a5d0376 100644
--- a/Makefile
+++ b/Makefile
@@ -1005,6 +1005,8 @@ PHONY += prepare0
 ifeq ($(KBUILD_EXTMOD),)
 core-y += kernel/ certs/ mm/ fs/ ipc/ security/ crypto/ block/
 
+core-$(CONFIG_KUNIT) += kunit/
+
 vmlinux-dirs   := $(patsubst %/,%,$(filter %/, $(init-y) $(init-m) \
 $(core-y) $(core-m) $(drivers-y) $(drivers-m) \
 $(net-y) $(net-m) $(libs-y) $(libs-m) $(virt-y)))
-- 
2.23.0.rc1.153.gdeed80330f-goog

___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH v12 13/18] kunit: tool: add Python wrappers for running KUnit tests

From: Felix Guo 

The ultimate goal is to create minimal isolated test binaries; in the
meantime we are using UML to provide the infrastructure to run tests, so
define an abstract way to configure and run tests that allow us to
change the context in which tests are built without affecting the user.
This also makes pretty and dynamic error reporting, and a lot of other
nice features easier.

kunit_config.py:
  - parse .config and Kconfig files.

kunit_kernel.py: provides helper functions to:
  - configure the kernel using kunitconfig.
  - build the kernel with the appropriate configuration.
  - provide function to invoke the kernel and stream the output back.

kunit_parser.py: parses raw logs returned out by kunit_kernel and
displays them in a user friendly way.

test_data/*: samples of test data for testing kunit.py, kunit_config.py,
etc.

Signed-off-by: Felix Guo 
Signed-off-by: Brendan Higgins 
Reviewed-by: Greg Kroah-Hartman 
Reviewed-by: Logan Gunthorpe 
---
 tools/testing/kunit/.gitignore|   3 +
 tools/testing/kunit/kunit.py  | 116 +++
 tools/testing/kunit/kunit_config.py   |  66 
 tools/testing/kunit/kunit_kernel.py   | 148 +
 tools/testing/kunit/kunit_parser.py   | 310 ++
 tools/testing/kunit/kunit_tool_test.py| 206 
 .../test_is_test_passed-all_passed.log|  32 ++
 .../test_data/test_is_test_passed-crash.log   |  69 
 .../test_data/test_is_test_passed-failure.log |  36 ++
 .../test_is_test_passed-no_tests_run.log  |  75 +
 .../test_output_isolated_correctly.log| 106 ++
 .../test_data/test_read_from_file.kconfig |  17 +
 12 files changed, 1184 insertions(+)
 create mode 100644 tools/testing/kunit/.gitignore
 create mode 100755 tools/testing/kunit/kunit.py
 create mode 100644 tools/testing/kunit/kunit_config.py
 create mode 100644 tools/testing/kunit/kunit_kernel.py
 create mode 100644 tools/testing/kunit/kunit_parser.py
 create mode 100755 tools/testing/kunit/kunit_tool_test.py
 create mode 100644 
tools/testing/kunit/test_data/test_is_test_passed-all_passed.log
 create mode 100644 tools/testing/kunit/test_data/test_is_test_passed-crash.log
 create mode 100644 
tools/testing/kunit/test_data/test_is_test_passed-failure.log
 create mode 100644 
tools/testing/kunit/test_data/test_is_test_passed-no_tests_run.log
 create mode 100644 
tools/testing/kunit/test_data/test_output_isolated_correctly.log
 create mode 100644 tools/testing/kunit/test_data/test_read_from_file.kconfig

diff --git a/tools/testing/kunit/.gitignore b/tools/testing/kunit/.gitignore
new file mode 100644
index 0..c791ff59a37a9
--- /dev/null
+++ b/tools/testing/kunit/.gitignore
@@ -0,0 +1,3 @@
+# Byte-compiled / optimized / DLL files
+__pycache__/
+*.py[cod]
\ No newline at end of file
diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py
new file mode 100755
index 0..da11bd62a4b82
--- /dev/null
+++ b/tools/testing/kunit/kunit.py
@@ -0,0 +1,116 @@
+#!/usr/bin/python3
+# SPDX-License-Identifier: GPL-2.0
+#
+# A thin wrapper on top of the KUnit Kernel
+#
+# Copyright (C) 2019, Google LLC.
+# Author: Felix Guo 
+# Author: Brendan Higgins 
+
+import argparse
+import sys
+import os
+import time
+
+from collections import namedtuple
+from enum import Enum, auto
+
+import kunit_config
+import kunit_kernel
+import kunit_parser
+
+KunitResult = namedtuple('KunitResult', ['status','result'])
+
+KunitRequest = namedtuple('KunitRequest', ['raw_output','timeout', 'jobs', 
'build_dir'])
+
+class KunitStatus(Enum):
+   SUCCESS = auto()
+   CONFIG_FAILURE = auto()
+   BUILD_FAILURE = auto()
+   TEST_FAILURE = auto()
+
+def run_tests(linux: kunit_kernel.LinuxSourceTree,
+ request: KunitRequest) -> KunitResult:
+   config_start = time.time()
+   success = linux.build_reconfig(request.build_dir)
+   config_end = time.time()
+   if not success:
+   return KunitResult(KunitStatus.CONFIG_FAILURE, 'could not 
configure kernel')
+
+   kunit_parser.print_with_timestamp('Building KUnit Kernel ...')
+
+   build_start = time.time()
+   success = linux.build_um_kernel(request.jobs, request.build_dir)
+   build_end = time.time()
+   if not success:
+   return KunitResult(KunitStatus.BUILD_FAILURE, 'could not build 
kernel')
+
+   kunit_parser.print_with_timestamp('Starting KUnit Kernel ...')
+   test_start = time.time()
+
+   test_result = kunit_parser.TestResult(kunit_parser.TestStatus.SUCCESS,
+ [],
+ 'Tests not Parsed.')
+   if request.raw_output:
+   kunit_parser.raw_output(
+   linux.run_kernel(timeout=request.timeout))
+   else:
+   kunit_output = linux.run_kernel(timeout=request.timeout)
+   test_result = kunit_parser.parse_run_tes

[PATCH v12 18/18] MAINTAINERS: add proc sysctl KUnit test to PROC SYSCTL section

Add entry for the new proc sysctl KUnit test to the PROC SYSCTL section,
and add Iurii as a maintainer.

Signed-off-by: Brendan Higgins 
Cc: Iurii Zaikin 
Reviewed-by: Greg Kroah-Hartman 
Reviewed-by: Logan Gunthorpe 
Acked-by: Luis Chamberlain 
---
 MAINTAINERS | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index f0bd77e8a8a2f..0cac78807137b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12965,12 +12965,14 @@ F:Documentation/filesystems/proc.txt
 PROC SYSCTL
 M: Luis Chamberlain 
 M: Kees Cook 
+M: Iurii Zaikin 
 L: linux-ker...@vger.kernel.org
 L: linux-fsde...@vger.kernel.org
 S: Maintained
 F: fs/proc/proc_sysctl.c
 F: include/linux/sysctl.h
 F: kernel/sysctl.c
+F: kernel/sysctl-test.c
 F: tools/testing/selftests/sysctl/
 
 PS3 NETWORK SUPPORT
-- 
2.23.0.rc1.153.gdeed80330f-goog

___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [RFC PATCH v2 08/19] fs/xfs: Fail truncate if page lease can't be broken

On Sat, Aug 10, 2019 at 09:22:09AM +1000, Dave Chinner wrote:
> On Fri, Aug 09, 2019 at 03:58:22PM -0700, ira.we...@intel.com wrote:
> > From: Ira Weiny 
> > 
> > If pages are under a lease fail the truncate operation.  We change the 
> > order of
> > lease breaks to directly fail the operation if the lease exists.
> > 
> > Select EXPORT_BLOCK_OPS for FS_DAX to ensure that xfs_break_lease_layouts() 
> > is
> > defined for FS_DAX as well as pNFS.
> > 
> > Signed-off-by: Ira Weiny 
> > ---
> >  fs/Kconfig| 1 +
> >  fs/xfs/xfs_file.c | 5 +++--
> >  2 files changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/Kconfig b/fs/Kconfig
> > index 14cd4abdc143..c10b91f92528 100644
> > --- a/fs/Kconfig
> > +++ b/fs/Kconfig
> > @@ -48,6 +48,7 @@ config FS_DAX
> > select DEV_PAGEMAP_OPS if (ZONE_DEVICE && !FS_DAX_LIMITED)
> > select FS_IOMAP
> > select DAX
> > +   select EXPORTFS_BLOCK_OPS
> > help
> >   Direct Access (DAX) can be used on memory-backed block devices.
> >   If the block device supports DAX and the filesystem supports DAX,
> 
> That looks wrong.

It may be...

>
> If you require xfs_break_lease_layouts() outside
> of pnfs context, then move the function in the XFS code base to a
> file that is built in. It's only external dependency is on the
> break_layout() function, and XFS already has other unconditional
> direct calls to break_layout()...

I'll check.  This patch was part of the original series and I must admit I
don't remember why I did it this way...

Thanks,
Ira

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> da...@fromorbit.com
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [RFC PATCH v2 07/19] fs/xfs: Teach xfs to use new dax_layout_busy_page()

On Sat, Aug 10, 2019 at 09:30:37AM +1000, Dave Chinner wrote:
> On Fri, Aug 09, 2019 at 03:58:21PM -0700, ira.we...@intel.com wrote:
> > From: Ira Weiny 
> > 
> > dax_layout_busy_page() can now operate on a sub-range of the
> > address_space provided.
> > 
> > Have xfs specify the sub range to dax_layout_busy_page()
> 
> Hmmm. I've got patches that change all these XFS interfaces to
> support range locks. I'm not sure the way the ranges are passed here
> is the best way to do it, and I suspect they aren't correct in some
> cases, either
> 
> > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> > index ff3c1fae5357..f0de5486f6c1 100644
> > --- a/fs/xfs/xfs_iops.c
> > +++ b/fs/xfs/xfs_iops.c
> > @@ -1042,10 +1042,16 @@ xfs_vn_setattr(
> > xfs_ilock(ip, XFS_MMAPLOCK_EXCL);
> > iolock = XFS_IOLOCK_EXCL | XFS_MMAPLOCK_EXCL;
> >  
> > -   error = xfs_break_layouts(inode, &iolock, BREAK_UNMAP);
> > -   if (error) {
> > -   xfs_iunlock(ip, XFS_MMAPLOCK_EXCL);
> > -   return error;
> > +   if (iattr->ia_size < inode->i_size) {
> > +   loff_t  off = iattr->ia_size;
> > +   loff_t  len = inode->i_size - 
> > iattr->ia_size;
> > +
> > +   error = xfs_break_layouts(inode, &iolock, off, len,
> > + BREAK_UNMAP);
> > +   if (error) {
> > +   xfs_iunlock(ip, XFS_MMAPLOCK_EXCL);
> > +   return error;
> > +   }
> 
> This isn't right - truncate up still needs to break the layout on
> the last filesystem block of the file,

I'm not following this?  From a user perspective they can't have done anything
with the data beyond the EOF.  So isn't it safe to allow EOF to grow without
changing the layout of that last block?

> and truncate down needs to
> extend to "maximum file offset" because we remove all extents beyond
> EOF on a truncate down.

Ok, I was trying to allow a user to extend the file without conflicts if they
were to have a pin on the 'beginning' of the original file.  This sounds like
you are saying that a layout lease must be dropped to do that?  In some ways I
think I understand what you are driving at and I think I see how I may have
been playing "fast and loose" with the strictness of the layout lease.  But
from a user perspective if there is a part of the file which "does not exist"
(beyond EOF) does it matter that the layout there may change?

> 
> i.e. when we use preallocation, the extent map extends beyond EOF,
> and layout leases need to be able to extend beyond the current EOF
> to allow the lease owner to do extending writes, extending truncate,
> preallocation beyond EOF, etc safely without having to get a new
> lease to cover the new region in the extended file...

I'm not following this.  What determines when preallocation is done?

Forgive my ignorance on file systems but how can we have a layout for every
file which is "maximum file offset" for every file even if a file is only 1
page long?

Thanks,
Ira

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> da...@fromorbit.com
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [RFC PATCH v2 01/19] fs/locks: Export F_LAYOUT lease to user space

On Sat, Aug 10, 2019 at 09:52:31AM +1000, Dave Chinner wrote:
> On Fri, Aug 09, 2019 at 03:58:15PM -0700, ira.we...@intel.com wrote:
> > From: Ira Weiny 
> > 
> > In order to support an opt-in policy for users to allow long term pins
> > of FS DAX pages we need to export the LAYOUT lease to user space.
> > 
> > This is the first of 2 new lease flags which must be used to allow a
> > long term pin to be made on a file.
> > 
> > After the complete series:
> > 
> > 0) Registrations to Device DAX char devs are not affected
> > 
> > 1) The user has to opt in to allowing page pins on a file with an exclusive
> >layout lease.  Both exclusive and layout lease flags are user visible 
> > now.
> > 
> > 2) page pins will fail if the lease is not active when the file back page is
> >encountered.
> > 
> > 3) Any truncate or hole punch operation on a pinned DAX page will fail.
> > 
> > 4) The user has the option of holding the lease or releasing it.  If they
> >release it no other pin calls will work on the file.
> > 
> > 5) Closing the file is ok.
> > 
> > 6) Unmapping the file is ok
> > 
> > 7) Pins against the files are tracked back to an owning file or an owning mm
> >depending on the internal subsystem needs.  With RDMA there is an owning
> >file which is related to the pined file.
> > 
> > 8) Only RDMA is currently supported
> > 
> > 9) Truncation of pages which are not actively pinned nor covered by a lease
> >will succeed.
> 
> This has nothing to do with layout leases or what they provide
> access arbitration over. Layout leases have _nothing_ to do with
> page pinning or RDMA - they arbitrate behaviour the file offset ->
> physical block device mapping within the filesystem and the
> behaviour that will occur when a specific lease is held.
> 
> The commit descripting needs to describe what F_LAYOUT actually
> protects, when they'll get broken, etc, not how RDMA is going to use
> it.

Ok yes I've been lax in mixing the cover letter for the series and this first
commit message.  My apologies.

> 
> > @@ -2022,8 +2030,26 @@ static int do_fcntl_add_lease(unsigned int fd, 
> > struct file *filp, long arg)
> > struct file_lock *fl;
> > struct fasync_struct *new;
> > int error;
> > +   unsigned int flags = 0;
> > +
> > +   /*
> > +* NOTE on F_LAYOUT lease
> > +*
> > +* LAYOUT lease types are taken on files which the user knows that
> > +* they will be pinning in memory for some indeterminate amount of
> > +* time.
> 
> Indeed, layout leases have nothing to do with pinning of memory.

Yep, Fair enough.  I'll rework the comment.

> That's something an application taht uses layout leases might do,
> but it largely irrelevant to the functionality layout leases
> provide. What needs to be done here is explain what the layout lease
> API actually guarantees w.r.t. the physical file layout, not what
> some application is going to do with a lease. e.g.
> 
>   The layout lease F_RDLCK guarantees that the holder will be
>   notified that the physical file layout is about to be
>   changed, and that it needs to release any resources it has
>   over the range of this lease, drop the lease and then
>   request it again to wait for the kernel to finish whatever
>   it is doing on that range.
> 
>   The layout lease F_RDLCK also allows the holder to modify
>   the physical layout of the file. If an operation from the
>   lease holder occurs that would modify the layout, that lease
>   holder does not get notification that a change will occur,
>   but it will block until all other F_RDLCK leases have been
>   released by their holders before going ahead.
> 
>   If there is a F_WRLCK lease held on the file, then a F_RDLCK
>   holder will fail any operation that may modify the physical
>   layout of the file. F_WRLCK provides exclusive physical
>   modification access to the holder, guaranteeing nothing else
>   will change the layout of the file while it holds the lease.
> 
>   The F_WRLCK holder can change the physical layout of the
>   file if it so desires, this will block while F_RDLCK holders
>   are notified and release their leases before the
>   modification will take place.
> 
> We need to define the semantics we expose to userspace first.

Agreed.  I believe I have implemented the semantics you describe above.  Do I
have your permission to use your verbiage as part of reworking the comment and
commit message?

Thanks,
Ira

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> da...@fromorbit.com
> 
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [RFC PATCH v2 16/19] RDMA/uverbs: Add back pointer to system file object

On Mon, Aug 12, 2019 at 10:00:40AM -0300, Jason Gunthorpe wrote:
> On Fri, Aug 09, 2019 at 03:58:30PM -0700, ira.we...@intel.com wrote:
> > From: Ira Weiny 
> > 
> > In order for MRs to be tracked against the open verbs context the ufile
> > needs to have a pointer to hand to the GUP code.
> > 
> > No references need to be taken as this should be valid for the lifetime
> > of the context.
> > 
> > Signed-off-by: Ira Weiny 
> >  drivers/infiniband/core/uverbs.h  | 1 +
> >  drivers/infiniband/core/uverbs_main.c | 1 +
> >  2 files changed, 2 insertions(+)
> > 
> > diff --git a/drivers/infiniband/core/uverbs.h 
> > b/drivers/infiniband/core/uverbs.h
> > index 1e5aeb39f774..e802ba8c67d6 100644
> > +++ b/drivers/infiniband/core/uverbs.h
> > @@ -163,6 +163,7 @@ struct ib_uverbs_file {
> > struct page *disassociate_page;
> >  
> > struct xarray   idr;
> > +   struct file *sys_file; /* backpointer to system file object 
> > */
> >  };
> 
> The 'struct file' has a lifetime strictly shorter than the
> ib_uverbs_file, which is kref'd on its own lifetime. Having a back
> pointer like this is confouding as it will be invalid for some of the
> lifetime of the struct.

Ah...  ok.  I really thought it was the other way around.

__fput() should not call ib_uverbs_close() until the last reference on struct
file is released...  What holds references to struct ib_uverbs_file past that?

Perhaps I need to add this (untested)?

diff --git a/drivers/infiniband/core/uverbs_main.c
b/drivers/infiniband/core/uverbs_main.c
index f628f9e4c09f..654e774d9cf2 100644
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -1125,6 +1125,8 @@ static int ib_uverbs_close(struct inode *inode, struct 
file *filp)
list_del_init(&file->list);
mutex_unlock(&file->device->lists_mutex);
 
+   file->sys_file = NULL;
+
kref_put(&file->ref, ib_uverbs_release_file);
 
return 0;

___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] mm/memremap: Fix reuse of pgmap instances with internal references

2019-08-12 Thread Dan Williams

On Mon, Aug 12, 2019 at 8:51 AM Jeff Moyer  wrote:
>
> Dan Williams  writes:
>
> > Currently, attempts to shutdown and re-enable a device-dax instance
> > trigger:
>
> What does "shutdown and re-enable" translate to?  If I disable and
> re-enable a device-dax namespace, I don't see this behavior.

I was not seeing this either until I made sure I was in 'bus" device model mode.

# cat /etc/modprobe.d/daxctl.conf
blacklist dax_pmem_compat
alias nd:t7* dax_pmem

# make TESTS="daxctl-devices.sh" check -j 40 2>out

# dmesg | grep WARN.*devm
[  225.588651] WARNING: CPU: 10 PID: 9103 at mm/memremap.c:211
devm_memremap_pages+0x234/0x850
[  225.679828] WARNING: CPU: 10 PID: 9103 at mm/memremap.c:211
devm_memremap_pages+0x234/0x850
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] mm/memremap: Fix reuse of pgmap instances with internal references

Dan Williams  writes:

> Currently, attempts to shutdown and re-enable a device-dax instance
> trigger:

What does "shutdown and re-enable" translate to?  If I disable and
re-enable a device-dax namespace, I don't see this behavior.

-Jeff

>
> Missing reference count teardown definition
> WARNING: CPU: 37 PID: 1608 at mm/memremap.c:211 
> devm_memremap_pages+0x234/0x850
> [..]
> RIP: 0010:devm_memremap_pages+0x234/0x850
> [..]
> Call Trace:
>  dev_dax_probe+0x66/0x190 [device_dax]
>  really_probe+0xef/0x390
>  driver_probe_device+0xb4/0x100
>  device_driver_attach+0x4f/0x60
>
> Given that the setup path initializes pgmap->ref, arrange for it to be
> also torn down so devm_memremap_pages() is ready to be called again and
> not be mistaken for the 3rd-party per-cpu-ref case.
>
> Fixes: 24917f6b1041 ("memremap: provide an optional internal refcount in 
> struct dev_pagemap")
> Reported-by: Fan Du 
> Tested-by: Vishal Verma 
> Cc: Andrew Morton 
> Cc: Christoph Hellwig 
> Cc: Ira Weiny 
> Cc: Jason Gunthorpe 
> Signed-off-by: Dan Williams 
> ---
>
> Andrew, I have another dax fix pending, so I'm ok to take this through
> the nvdimm tree, holler if you want it in -mm.
>
>  mm/memremap.c |6 ++
>  1 file changed, 6 insertions(+)
>
> diff --git a/mm/memremap.c b/mm/memremap.c
> index 6ee03a816d67..86432650f829 100644
> --- a/mm/memremap.c
> +++ b/mm/memremap.c
> @@ -91,6 +91,12 @@ static void dev_pagemap_cleanup(struct dev_pagemap *pgmap)
>   wait_for_completion(&pgmap->done);
>   percpu_ref_exit(pgmap->ref);
>   }
> + /*
> +  * Undo the pgmap ref assignment for the internal case as the
> +  * caller may re-enable the same pgmap.
> +  */
> + if (pgmap->ref == &pgmap->internal_ref)
> + pgmap->ref = NULL;
>  }
>  
>  static void devm_memremap_pages_release(void *data)
>
> ___
> Linux-nvdimm mailing list
> Linux-nvdimm@lists.01.org
> https://lists.01.org/mailman/listinfo/linux-nvdimm
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 5/5] memremap: provide a not device managed memremap_pages

2019-08-12 Thread Christoph Hellwig

On Mon, Aug 12, 2019 at 08:20:58PM +0530, Bharata B Rao wrote:
> On Sun, Aug 11, 2019 at 10:12:47AM +0200, Christoph Hellwig wrote:
> > The kvmppc ultravisor code wants a device private memory pool that is
> > system wide and not attached to a device.  Instead of faking up one
> > provide a low-level memremap_pages for it.  Note that this function is
> > not exported, and doesn't have a cleanup routine associated with it to
> > discourage use from more driver like users.
> 
> The kvmppc secure pages management code will be part of kvm-hv which
> can be built as module too. So it would require memremap_pages() to be
> exported.
> 
> Additionally, non-dev version of the cleanup routine
> devm_memremap_pages_release() or equivalent would also be requried.
> With device being present, put_device() used to take care of this
> cleanup.

Oh well.  We can add them fairly easily if we really need to, but I
tried to avoid that.  Can you try to see if this works non-modular
for you for now until we hear more feedback from Dan?
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 5/5] memremap: provide a not device managed memremap_pages

2019-08-12 Thread Bharata B Rao

On Sun, Aug 11, 2019 at 10:12:47AM +0200, Christoph Hellwig wrote:
> The kvmppc ultravisor code wants a device private memory pool that is
> system wide and not attached to a device.  Instead of faking up one
> provide a low-level memremap_pages for it.  Note that this function is
> not exported, and doesn't have a cleanup routine associated with it to
> discourage use from more driver like users.

The kvmppc secure pages management code will be part of kvm-hv which
can be built as module too. So it would require memremap_pages() to be
exported.

Additionally, non-dev version of the cleanup routine
devm_memremap_pages_release() or equivalent would also be requried.
With device being present, put_device() used to take care of this
cleanup.

Regards,
Bharata.

___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [ndctl PATCH] ndctl/test: Add xfs reflink dependency