Re: [linux-next:master 6618/6917] kernel/sched/psi.c:1230:13: sparse: error: incompatible types in comparison expression (different address spaces)

[Suren Baghdasaryan <surenb@google.com>]

On Tue, Feb 12, 2019 at 8:35 AM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Tue, Feb 12, 2019 at 11:31:45AM -0500, Johannes Weiner wrote:
> > On Mon, Feb 11, 2019 at 05:36:06PM -0800, Matthew Wilcox wrote:
> > > On Fri, Feb 08, 2019 at 11:44:07PM -0800, Paul E. McKenney wrote:
> > > > On Fri, Feb 08, 2019 at 03:14:41PM -0800, Andrew Morton wrote:
> > > > > On Fri, 8 Feb 2019 02:29:33 +0800 kbuild test robot <lkp@intel.com> wrote:
> > > > > >   1223        static __poll_t psi_fop_poll(struct file *file, poll_table *wait)
> > > > > >   1224        {
> > > > > >   1225                struct seq_file *seq = file->private_data;
> > > > > >   1226                struct psi_trigger *t;
> > > > > >   1227                __poll_t ret;
> > > > > >   1228
> > > > > >   1229                rcu_read_lock();
> > > > > > > 1230                t = rcu_dereference(seq->private);
> > >
> > > So the problem here is the opposite of what we think it is -- seq->private
> > > is not marked as being RCU protected.
> > >
> > > > If you wish to opt into this checking, you need to mark the pointer
> > > > definitions (in this case ->private) with __rcu.  It may also
> > > > be necessary to mark function parameters as well, as is done for
> > > > radix_tree_iter_resume().  If you do not wish to use this checking,
> > > > you should ignore these sparse warnings.
> >
> > We cannot make struct seq_file->private generally __rcu, but the
> > cgroup code has a similar thing with kernfs, where it's doing rcu for
> > its particular use of struct kernfs_node->private. This is how it does
> > the dereference:
> >
> >       cgrp = rcu_dereference(*(void __rcu __force **)&kn->priv);
> >
> > We could do this here as well.
> >
> > It's ugly, though. I'd also be fine with ignoring the sparse warning.
>
> How about:
>
> +++ b/include/linux/seq_file.h
> @@ -26,7 +26,10 @@ struct seq_file {
>         const struct seq_operations *op;
>         int poll_event;
>         const struct file *file;
> -       void *private;
> +       union {
> +               void *private;
> +               void __rcu *rcu_private;
> +       };
>  };
>
>  struct seq_operations {
>

Personally I would prefer cgrp = rcu_dereference(*(void __rcu __force
**)&kn->priv); as it's more localized change but if union would be
preferable I'll roll that into the next version of psi monitor.
Thanks,
Suren.