On Thu 12-10-17 10:30:57, Mel Gorman wrote:
> During truncation, the mapping has already been checked for shmem and dax
> so it's known that workingset_update_node is required. This patch avoids
> the checks on mapping for each page being truncated. In all other cases,
> a lookup helper is used to determine if workingset_update_node() needs
> to be called. The one danger is that the API is slightly harder to use as
> calling workingset_update_node directly without checking for dax or shmem
> mappings could lead to surprises. However, the API rarely needs to be used
> and hopefully the comment is enough to give people the hint.
>
> sparsetruncate (tiny)
> 4.14.0-rc4 4.14.0-rc4
> oneirq-v1r1 pickhelper-v1r1
> Min Time 141.00 ( 0.00%) 140.00 ( 0.71%)
> 1st-qrtle Time 142.00 ( 0.00%) 141.00 ( 0.70%)
> 2nd-qrtle Time 142.00 ( 0.00%) 142.00 ( 0.00%)
> 3rd-qrtle Time 143.00 ( 0.00%) 143.00 ( 0.00%)
> Max-90% Time 144.00 ( 0.00%) 144.00 ( 0.00%)
> Max-95% Time 147.00 ( 0.00%) 145.00 ( 1.36%)
> Max-99% Time 195.00 ( 0.00%) 191.00 ( 2.05%)
> Max Time 230.00 ( 0.00%) 205.00 ( 10.87%)
> Amean Time 144.37 ( 0.00%) 143.82 ( 0.38%)
> Stddev Time 10.44 ( 0.00%) 9.00 ( 13.74%)
> Coeff Time 7.23 ( 0.00%) 6.26 ( 13.41%)
> Best99%Amean Time 143.72 ( 0.00%) 143.34 ( 0.26%)
> Best95%Amean Time 142.37 ( 0.00%) 142.00 ( 0.26%)
> Best90%Amean Time 142.19 ( 0.00%) 141.85 ( 0.24%)
> Best75%Amean Time 141.92 ( 0.00%) 141.58 ( 0.24%)
> Best50%Amean Time 141.69 ( 0.00%) 141.31 ( 0.27%)
> Best25%Amean Time 141.38 ( 0.00%) 140.97 ( 0.29%)
>
> As you'd expect, the gain is marginal but it can be detected. The differences
> in bonnie are all within the noise which is not surprising given the impact
> on the microbenchmark.
>
> Signed-off-by: Mel Gorman <mgorman@xxxxxxxxxxxxxxxxxxx>
...
> diff --git a/mm/workingset.c b/mm/workingset.c
> index 7119cd745ace..a80d52387734 100644
> --- a/mm/workingset.c
> +++ b/mm/workingset.c
> @@ -341,12 +341,6 @@ static struct list_lru shadow_nodes;
>
> void workingset_update_node(struct radix_tree_node *node, void *private)
> {
> - struct address_space *mapping = private;
> -
> - /* Only regular page cache has shadow entries */
> - if (dax_mapping(mapping) || shmem_mapping(mapping))
> - return;
> -
Hum, we don't need to pass 'mapping' from call sites then? Either pass NULL
or just remove the argument completely since nobody needs it anymore...
Otherwise the patch looks good.
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
