| Message ID | 1456924317-23525-1-git-send-email-david.weinehall@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Mar 02, 2016 at 03:11:57PM +0200, David Weinehall wrote: > On machines that lack an LLC the pm-caching subtest will > terminate with sigbus and thus CRASH during the > I915_CACHING_CACHED iteration. This patch adds a check for > this condition and skips that iteration. you can delete the got_caching assertion and enable_one_screen_and_wait() as well, they are not exercising the associated code. > > Signed-off-by: David Weinehall <david.weinehall@linux.intel.com> > --- > tests/pm_rpm.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/tests/pm_rpm.c b/tests/pm_rpm.c > index 2aa6c1018aa2..c25252eafad0 100644 > --- a/tests/pm_rpm.c > +++ b/tests/pm_rpm.c > @@ -1813,6 +1813,16 @@ static void pm_test_caching(void) > gem_buf = gem_mmap__gtt(drm_fd, handle, gtt_obj_max_size, PROT_WRITE); > > for (i = 0; i < ARRAY_SIZE(cache_levels); i++) { > + /* > + * Skip the I915_CACHING_CACHED test > + * if we lack an LLC cache > + */ > + if (cache_levels[i] == I915_CACHING_CACHED && > + !gem_has_llc(drm_fd)) { > + igt_debug("!gem_has_llc(); skipping\n"); > + continue; > + } No. For the purposes of the test you actually want to call gem_set_caching(fd, handle, NONE). -Chris
On Wed, Mar 02, 2016 at 01:27:06PM +0000, Chris Wilson wrote: > On Wed, Mar 02, 2016 at 03:11:57PM +0200, David Weinehall wrote: > > On machines that lack an LLC the pm-caching subtest will > > terminate with sigbus and thus CRASH during the > > I915_CACHING_CACHED iteration. This patch adds a check for > > this condition and skips that iteration. > > you can delete the got_caching assertion and > enable_one_screen_and_wait() as well, they are not exercising the > associated code. Hmmm. How about the matching disable_all_screens_and_wait()? Also, isn't the got_caching assertion meant to check that when we enable GEM caching we actually get the mode we requested, and if so, do we test for this elsewhere? Or are you saying that this test doesn't achieve this purpose? > > > > Signed-off-by: David Weinehall <david.weinehall@linux.intel.com> > > --- > > tests/pm_rpm.c | 10 ++++++++++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/tests/pm_rpm.c b/tests/pm_rpm.c > > index 2aa6c1018aa2..c25252eafad0 100644 > > --- a/tests/pm_rpm.c > > +++ b/tests/pm_rpm.c > > @@ -1813,6 +1813,16 @@ static void pm_test_caching(void) > > gem_buf = gem_mmap__gtt(drm_fd, handle, gtt_obj_max_size, PROT_WRITE); > > > > for (i = 0; i < ARRAY_SIZE(cache_levels); i++) { > > + /* > > + * Skip the I915_CACHING_CACHED test > > + * if we lack an LLC cache > > + */ > > + if (cache_levels[i] == I915_CACHING_CACHED && > > + !gem_has_llc(drm_fd)) { > > + igt_debug("!gem_has_llc(); skipping\n"); > > + continue; > > + } > > No. For the purposes of the test you actually want to call > gem_set_caching(fd, handle, NONE). Wouldn't that case already be exercised in the first iteration of this test? Kind regards, DAvid
On Wed, Mar 02, 2016 at 03:55:56PM +0200, David Weinehall wrote: > On Wed, Mar 02, 2016 at 01:27:06PM +0000, Chris Wilson wrote: > > On Wed, Mar 02, 2016 at 03:11:57PM +0200, David Weinehall wrote: > > > On machines that lack an LLC the pm-caching subtest will > > > terminate with sigbus and thus CRASH during the > > > I915_CACHING_CACHED iteration. This patch adds a check for > > > this condition and skips that iteration. > > > > you can delete the got_caching assertion and > > enable_one_screen_and_wait() as well, they are not exercising the > > associated code. > > Hmmm. How about the matching disable_all_screens_and_wait()? > Also, isn't the got_caching assertion meant to check that > when we enable GEM caching we actually get the mode we requested, > and if so, do we test for this elsewhere? Or are you saying that > this test doesn't achieve this purpose? This is not a test for set-caching API, but on whether we do device accesses without rpm. get-caching doesn't touch the device at all (and never ever should) so is irrelevant for the test. > > > > > > > Signed-off-by: David Weinehall <david.weinehall@linux.intel.com> > > > --- > > > tests/pm_rpm.c | 10 ++++++++++ > > > 1 file changed, 10 insertions(+) > > > > > > diff --git a/tests/pm_rpm.c b/tests/pm_rpm.c > > > index 2aa6c1018aa2..c25252eafad0 100644 > > > --- a/tests/pm_rpm.c > > > +++ b/tests/pm_rpm.c > > > @@ -1813,6 +1813,16 @@ static void pm_test_caching(void) > > > gem_buf = gem_mmap__gtt(drm_fd, handle, gtt_obj_max_size, PROT_WRITE); > > > > > > for (i = 0; i < ARRAY_SIZE(cache_levels); i++) { > > > + /* > > > + * Skip the I915_CACHING_CACHED test > > > + * if we lack an LLC cache > > > + */ > > > + if (cache_levels[i] == I915_CACHING_CACHED && > > > + !gem_has_llc(drm_fd)) { > > > + igt_debug("!gem_has_llc(); skipping\n"); > > > + continue; > > > + } > > > > No. For the purposes of the test you actually want to call > > gem_set_caching(fd, handle, NONE). > > Wouldn't that case already be exercised in the first iteration of this > test? Not really. The test is that given a vma bound into the ggtt that we then change the cache level on, do we take the rpm around the gsm access. To exercise the code we the vma bound into the ggtt, that is what the *ptr does. Then we need it to change cache level to exercise how we handle the vma inside set-cache-level. That is the crux of the test. -Chris
On ke, 2016-03-02 at 14:04 +0000, Chris Wilson wrote: > On Wed, Mar 02, 2016 at 03:55:56PM +0200, David Weinehall wrote: > > On Wed, Mar 02, 2016 at 01:27:06PM +0000, Chris Wilson wrote: > > > On Wed, Mar 02, 2016 at 03:11:57PM +0200, David Weinehall wrote: > > > > On machines that lack an LLC the pm-caching subtest will > > > > terminate with sigbus and thus CRASH during the > > > > I915_CACHING_CACHED iteration. This patch adds a check for > > > > this condition and skips that iteration. > > > > > > you can delete the got_caching assertion and > > > enable_one_screen_and_wait() as well, they are not exercising the > > > associated code. > > > > Hmmm. How about the matching disable_all_screens_and_wait()? > > Also, isn't the got_caching assertion meant to check that > > when we enable GEM caching we actually get the mode we requested, > > and if so, do we test for this elsewhere? Or are you saying that > > this test doesn't achieve this purpose? > > This is not a test for set-caching API, but on whether we do device > accesses without rpm. get-caching doesn't touch the device at all > (and > never ever should) so is irrelevant for the test. The purpose of the enable/disable screen calls was to make sure that the object gets unbound, otherwise we may not call i915_vma_bind() which is where the actual HW access happened. But actually it would be enough to call disable_all_screens_and_wait() once and then call wait_for_suspended() instead of disable_all_screens_and_wait() in the loop. --Imre > > > > > > > > > > Signed-off-by: David Weinehall <david.weinehall@linux.intel.com > > > > > > > > > --- > > > > tests/pm_rpm.c | 10 ++++++++++ > > > > 1 file changed, 10 insertions(+) > > > > > > > > diff --git a/tests/pm_rpm.c b/tests/pm_rpm.c > > > > index 2aa6c1018aa2..c25252eafad0 100644 > > > > --- a/tests/pm_rpm.c > > > > +++ b/tests/pm_rpm.c > > > > @@ -1813,6 +1813,16 @@ static void pm_test_caching(void) > > > > gem_buf = gem_mmap__gtt(drm_fd, handle, > > > > gtt_obj_max_size, PROT_WRITE); > > > > > > > > for (i = 0; i < ARRAY_SIZE(cache_levels); i++) { > > > > + /* > > > > + * Skip the I915_CACHING_CACHED test > > > > + * if we lack an LLC cache > > > > + */ > > > > + if (cache_levels[i] == I915_CACHING_CACHED && > > > > + !gem_has_llc(drm_fd)) { > > > > + igt_debug("!gem_has_llc(); > > > > skipping\n"); > > > > + continue; > > > > + } > > > > > > No. For the purposes of the test you actually want to call > > > gem_set_caching(fd, handle, NONE). > > > > Wouldn't that case already be exercised in the first iteration of > > this > > test? > > Not really. The test is that given a vma bound into the ggtt that we > then change the cache level on, do we take the rpm around the gsm > access. To exercise the code we the vma bound into the ggtt, that is > what the *ptr does. Then we need it to change cache level to exercise > how we handle the vma inside set-cache-level. That is the crux of the > test. > -Chris >
On Wed, Mar 02, 2016 at 04:32:41PM +0200, Imre Deak wrote: > On ke, 2016-03-02 at 14:04 +0000, Chris Wilson wrote: > > On Wed, Mar 02, 2016 at 03:55:56PM +0200, David Weinehall wrote: > > > On Wed, Mar 02, 2016 at 01:27:06PM +0000, Chris Wilson wrote: > > > > On Wed, Mar 02, 2016 at 03:11:57PM +0200, David Weinehall wrote: > > > > > On machines that lack an LLC the pm-caching subtest will > > > > > terminate with sigbus and thus CRASH during the > > > > > I915_CACHING_CACHED iteration. This patch adds a check for > > > > > this condition and skips that iteration. > > > > > > > > you can delete the got_caching assertion and > > > > enable_one_screen_and_wait() as well, they are not exercising the > > > > associated code. > > > > > > Hmmm. How about the matching disable_all_screens_and_wait()? > > > Also, isn't the got_caching assertion meant to check that > > > when we enable GEM caching we actually get the mode we requested, > > > and if so, do we test for this elsewhere? Or are you saying that > > > this test doesn't achieve this purpose? > > > > This is not a test for set-caching API, but on whether we do device > > accesses without rpm. get-caching doesn't touch the device at all > > (and > > never ever should) so is irrelevant for the test. > > The purpose of the enable/disable screen calls was to make sure that > the object gets unbound, otherwise we may not call i915_vma_bind() > which is where the actual HW access happened. But actually it would be > enough to call disable_all_screens_and_wait() once and then call > wait_for_suspended() instead of disable_all_screens_and_wait() in the > loop. Actually no, that's why you have the memset/*ptr - that is what forces the vma to get bound. And to exercise the set-cache-level, you need it bound into at a different cache-level, hence the suggestion to call set-cache-level again - respecting the rules of using the GGTT. -Chris
On Wed, Mar 02, 2016 at 02:04:58PM +0000, Chris Wilson wrote: > On Wed, Mar 02, 2016 at 03:55:56PM +0200, David Weinehall wrote: > > On Wed, Mar 02, 2016 at 01:27:06PM +0000, Chris Wilson wrote: > > > On Wed, Mar 02, 2016 at 03:11:57PM +0200, David Weinehall wrote: > > > > On machines that lack an LLC the pm-caching subtest will > > > > terminate with sigbus and thus CRASH during the > > > > I915_CACHING_CACHED iteration. This patch adds a check for > > > > this condition and skips that iteration. > > > > > > you can delete the got_caching assertion and > > > enable_one_screen_and_wait() as well, they are not exercising the > > > associated code. > > > > Hmmm. How about the matching disable_all_screens_and_wait()? > > Also, isn't the got_caching assertion meant to check that > > when we enable GEM caching we actually get the mode we requested, > > and if so, do we test for this elsewhere? Or are you saying that > > this test doesn't achieve this purpose? > > This is not a test for set-caching API, but on whether we do device > accesses without rpm. get-caching doesn't touch the device at all (and > never ever should) so is irrelevant for the test. Right. How about the disable/enable screens bit? Stay or go? > > > > > > > > > > Signed-off-by: David Weinehall <david.weinehall@linux.intel.com> > > > > --- > > > > tests/pm_rpm.c | 10 ++++++++++ > > > > 1 file changed, 10 insertions(+) > > > > > > > > diff --git a/tests/pm_rpm.c b/tests/pm_rpm.c > > > > index 2aa6c1018aa2..c25252eafad0 100644 > > > > --- a/tests/pm_rpm.c > > > > +++ b/tests/pm_rpm.c > > > > @@ -1813,6 +1813,16 @@ static void pm_test_caching(void) > > > > gem_buf = gem_mmap__gtt(drm_fd, handle, gtt_obj_max_size, PROT_WRITE); > > > > > > > > for (i = 0; i < ARRAY_SIZE(cache_levels); i++) { > > > > + /* > > > > + * Skip the I915_CACHING_CACHED test > > > > + * if we lack an LLC cache > > > > + */ > > > > + if (cache_levels[i] == I915_CACHING_CACHED && > > > > + !gem_has_llc(drm_fd)) { > > > > + igt_debug("!gem_has_llc(); skipping\n"); > > > > + continue; > > > > + } > > > > > > No. For the purposes of the test you actually want to call > > > gem_set_caching(fd, handle, NONE). > > > > Wouldn't that case already be exercised in the first iteration of this > > test? > > Not really. The test is that given a vma bound into the ggtt that we > then change the cache level on, do we take the rpm around the gsm > access. To exercise the code we the vma bound into the ggtt, that is > what the *ptr does. Then we need it to change cache level to exercise > how we handle the vma inside set-cache-level. That is the crux of the > test. Thanks for the explanation! Kind regards, David
On ke, 2016-03-02 at 14:37 +0000, Chris Wilson wrote: > On Wed, Mar 02, 2016 at 04:32:41PM +0200, Imre Deak wrote: > > On ke, 2016-03-02 at 14:04 +0000, Chris Wilson wrote: > > > On Wed, Mar 02, 2016 at 03:55:56PM +0200, David Weinehall wrote: > > > > On Wed, Mar 02, 2016 at 01:27:06PM +0000, Chris Wilson wrote: > > > > > On Wed, Mar 02, 2016 at 03:11:57PM +0200, David Weinehall > > > > > wrote: > > > > > > On machines that lack an LLC the pm-caching subtest will > > > > > > terminate with sigbus and thus CRASH during the > > > > > > I915_CACHING_CACHED iteration. This patch adds a check for > > > > > > this condition and skips that iteration. > > > > > > > > > > you can delete the got_caching assertion and > > > > > enable_one_screen_and_wait() as well, they are not exercising > > > > > the > > > > > associated code. > > > > > > > > Hmmm. How about the matching disable_all_screens_and_wait()? > > > > Also, isn't the got_caching assertion meant to check that > > > > when we enable GEM caching we actually get the mode we > > > > requested, > > > > and if so, do we test for this elsewhere? Or are you saying > > > > that > > > > this test doesn't achieve this purpose? > > > > > > This is not a test for set-caching API, but on whether we do > > > device > > > accesses without rpm. get-caching doesn't touch the device at all > > > (and > > > never ever should) so is irrelevant for the test. > > > > The purpose of the enable/disable screen calls was to make sure > > that > > the object gets unbound, otherwise we may not call i915_vma_bind() > > which is where the actual HW access happened. But actually it would > > be > > enough to call disable_all_screens_and_wait() once and then call > > wait_for_suspended() instead of disable_all_screens_and_wait() in > > the > > loop. > > Actually no, that's why you have the memset/*ptr - that is what > forces > the vma to get bound. And to exercise the set-cache-level, you need > it > bound into at a different cache-level, hence the suggestion to call > set-cache-level again - respecting the rules of using the GGTT. Yes I see that it gets bound, but is guaranteed that it gets unbound during the next set-cache-level call? As I understand it will only happen if i915_gem_valid_gtt_space() returns false. --Imre
On Wed, Mar 02, 2016 at 04:41:54PM +0200, Imre Deak wrote: > On ke, 2016-03-02 at 14:37 +0000, Chris Wilson wrote: > > On Wed, Mar 02, 2016 at 04:32:41PM +0200, Imre Deak wrote: > > > On ke, 2016-03-02 at 14:04 +0000, Chris Wilson wrote: > > > > On Wed, Mar 02, 2016 at 03:55:56PM +0200, David Weinehall wrote: > > > > > On Wed, Mar 02, 2016 at 01:27:06PM +0000, Chris Wilson wrote: > > > > > > On Wed, Mar 02, 2016 at 03:11:57PM +0200, David Weinehall > > > > > > wrote: > > > > > > > On machines that lack an LLC the pm-caching subtest will > > > > > > > terminate with sigbus and thus CRASH during the > > > > > > > I915_CACHING_CACHED iteration. This patch adds a check for > > > > > > > this condition and skips that iteration. > > > > > > > > > > > > you can delete the got_caching assertion and > > > > > > enable_one_screen_and_wait() as well, they are not exercising > > > > > > the > > > > > > associated code. > > > > > > > > > > Hmmm. How about the matching disable_all_screens_and_wait()? > > > > > Also, isn't the got_caching assertion meant to check that > > > > > when we enable GEM caching we actually get the mode we > > > > > requested, > > > > > and if so, do we test for this elsewhere? Or are you saying > > > > > that > > > > > this test doesn't achieve this purpose? > > > > > > > > This is not a test for set-caching API, but on whether we do > > > > device > > > > accesses without rpm. get-caching doesn't touch the device at all > > > > (and > > > > never ever should) so is irrelevant for the test. > > > > > > The purpose of the enable/disable screen calls was to make sure > > > that > > > the object gets unbound, otherwise we may not call i915_vma_bind() > > > which is where the actual HW access happened. But actually it would > > > be > > > enough to call disable_all_screens_and_wait() once and then call > > > wait_for_suspended() instead of disable_all_screens_and_wait() in > > > the > > > loop. > > > > Actually no, that's why you have the memset/*ptr - that is what > > forces > > the vma to get bound. And to exercise the set-cache-level, you need > > it > > bound into at a different cache-level, hence the suggestion to call > > set-cache-level again - respecting the rules of using the GGTT. > > Yes I see that it gets bound, but is guaranteed that it gets unbound > during the next set-cache-level call? As I understand it will only > happen if i915_gem_valid_gtt_space() returns false. No. But that is beside the point. It is either unbound or rewritten during the set-cache-level ioctl, either way we write through the GSM if it is currently bound. (And it will only be unbound in fairly rare circumstances.) -Chris
On ke, 2016-03-02 at 14:49 +0000, Chris Wilson wrote: > On Wed, Mar 02, 2016 at 04:41:54PM +0200, Imre Deak wrote: > > On ke, 2016-03-02 at 14:37 +0000, Chris Wilson wrote: > > > On Wed, Mar 02, 2016 at 04:32:41PM +0200, Imre Deak wrote: > > > > On ke, 2016-03-02 at 14:04 +0000, Chris Wilson wrote: > > > > > On Wed, Mar 02, 2016 at 03:55:56PM +0200, David Weinehall > > > > > wrote: > > > > > > On Wed, Mar 02, 2016 at 01:27:06PM +0000, Chris Wilson > > > > > > wrote: > > > > > > > On Wed, Mar 02, 2016 at 03:11:57PM +0200, David Weinehall > > > > > > > wrote: > > > > > > > > On machines that lack an LLC the pm-caching subtest > > > > > > > > will > > > > > > > > terminate with sigbus and thus CRASH during the > > > > > > > > I915_CACHING_CACHED iteration. This patch adds a check > > > > > > > > for > > > > > > > > this condition and skips that iteration. > > > > > > > > > > > > > > you can delete the got_caching assertion and > > > > > > > enable_one_screen_and_wait() as well, they are not > > > > > > > exercising > > > > > > > the > > > > > > > associated code. > > > > > > > > > > > > Hmmm. How about the matching > > > > > > disable_all_screens_and_wait()? > > > > > > Also, isn't the got_caching assertion meant to check that > > > > > > when we enable GEM caching we actually get the mode we > > > > > > requested, > > > > > > and if so, do we test for this elsewhere? Or are you saying > > > > > > that > > > > > > this test doesn't achieve this purpose? > > > > > > > > > > This is not a test for set-caching API, but on whether we do > > > > > device > > > > > accesses without rpm. get-caching doesn't touch the device at > > > > > all > > > > > (and > > > > > never ever should) so is irrelevant for the test. > > > > > > > > The purpose of the enable/disable screen calls was to make sure > > > > that > > > > the object gets unbound, otherwise we may not > > > > call i915_vma_bind() > > > > which is where the actual HW access happened. But actually it > > > > would > > > > be > > > > enough to call disable_all_screens_and_wait() once and then > > > > call > > > > wait_for_suspended() instead of disable_all_screens_and_wait() > > > > in > > > > the > > > > loop. > > > > > > Actually no, that's why you have the memset/*ptr - that is what > > > forces > > > the vma to get bound. And to exercise the set-cache-level, you > > > need > > > it > > > bound into at a different cache-level, hence the suggestion to > > > call > > > set-cache-level again - respecting the rules of using the GGTT. > > > > Yes I see that it gets bound, but is guaranteed that it gets > > unbound > > during the next set-cache-level call? As I understand it will only > > happen if i915_gem_valid_gtt_space() returns false. > > No. But that is beside the point. It is either unbound or rewritten > during the set-cache-level ioctl, either way we write through the GSM > if > it is currently bound. (And it will only be unbound in fairly rare > circumstances.) Ah right, I missed that, thanks for explaining. But we still need to make sure the device is suspended before the set-cache-level call, that is an initial disable_all_screens() and then wait_for_suspended() before calling set-cache-level. --Imre
On Wed, Mar 02, 2016 at 05:01:19PM +0200, Imre Deak wrote: > Ah right, I missed that, thanks for explaining. But we still need to > make sure the device is suspended before the set-cache-level call, that > is an initial disable_all_screens() and then wait_for_suspended() > before calling set-cache-level. To me it would be clearer then to split it up into a disable_all_screens() before the loop and wait_for_suspend() inside before the second set-cache, and drop the enable_one_screen(). (That way the test is focusing on the set-cache-level itself and not confusing the reader/coverage with the display power blocks). You also want the wait_for_suspended() before the memset, to ensure the fault handler is also rpm away, but that is checked elsewhere so not essential. Hmm, this file has a noticeable lack of GEM domain management. E.g. gem_mmap_subtest() will only work by happenstance on !llc with gtt_mmap=false. To test the shrinker and eviction code, you can use i915_drop_caches. First populate the GGTT using a GTT mmap, then igt_drop_caches_set(DROP_BOUND); (Unbind looks best to be exercised through gem_close, though adding DROP_EVICT_GGTT seems sane). I don't see how you detect the number of illegal accesses whilst suspended? Should we not have a counter exposed through debugfs? -Chris
On ke, 2016-03-02 at 15:28 +0000, Chris Wilson wrote: > On Wed, Mar 02, 2016 at 05:01:19PM +0200, Imre Deak wrote: > [...] > Hmm, this file has a noticeable lack of GEM domain management. E.g. > gem_mmap_subtest() will only work by happenstance on !llc with > gtt_mmap=false. > > To test the shrinker and eviction code, you can use i915_drop_caches. > First populate the GGTT using a GTT mmap, then > igt_drop_caches_set(DROP_BOUND); (Unbind looks best to be exercised > through gem_close, though adding DROP_EVICT_GGTT seems sane). > > I don't see how you detect the number of illegal accesses whilst > suspended? Should we not have a counter exposed through debugfs? Ok, I'll add JIRA task(s) for these;) --Imre
diff --git a/tests/pm_rpm.c b/tests/pm_rpm.c index 2aa6c1018aa2..c25252eafad0 100644 --- a/tests/pm_rpm.c +++ b/tests/pm_rpm.c @@ -1813,6 +1813,16 @@ static void pm_test_caching(void) gem_buf = gem_mmap__gtt(drm_fd, handle, gtt_obj_max_size, PROT_WRITE); for (i = 0; i < ARRAY_SIZE(cache_levels); i++) { + /* + * Skip the I915_CACHING_CACHED test + * if we lack an LLC cache + */ + if (cache_levels[i] == I915_CACHING_CACHED && + !gem_has_llc(drm_fd)) { + igt_debug("!gem_has_llc(); skipping\n"); + continue; + } + memset(gem_buf, 16 << i, gtt_obj_max_size); disable_all_screens_and_wait(&ms_data);
On machines that lack an LLC the pm-caching subtest will terminate with sigbus and thus CRASH during the I915_CACHING_CACHED iteration. This patch adds a check for this condition and skips that iteration. Signed-off-by: David Weinehall <david.weinehall@linux.intel.com> --- tests/pm_rpm.c | 10 ++++++++++ 1 file changed, 10 insertions(+)