RE: [PATCH 07/19] Fix RCU handling of scsi_device.vpd_pg8[03]

["Seymour, Shane M" <shane.seymour@hpe.com>]

> -----Original Message-----
> From: Bart Van Assche [mailto:Bart.VanAssche@wdc.com]
> Sent: Friday, August 25, 2017 2:54 AM
> To: hch@lst.de
> Cc: jejb@linux.vnet.ibm.com; linux-scsi@vger.kernel.org; hare@suse.de;
> jthumshirn@suse.de; martin.petersen@oracle.com; Seymour, Shane M
> <shane.seymour@hpe.com>
> Subject: Re: [PATCH 07/19] Fix RCU handling of scsi_device.vpd_pg8[03]
> 
> On Thu, 2017-08-24 at 11:07 +0200, Christoph Hellwig wrote:
> > On Wed, Aug 23, 2017 at 02:39:57PM -0700, Bart Van Assche wrote:
> > > Only annotate pointers that are shared across threads with __rcu.
> > > Use rcu_dereference() when dereferencing an RCU pointer. Protect
> > > also the RCU pointer dereferences when freeing RCU pointers. This
> > > patch suppresses about twenty sparse complaints about the
> > > vpd_pg8[03] pointers.
> >
> > Shouldn't the kfrees be kfree_rcu?  or where else is the rcu
> > protection for them?
> 
> Hello Christoph,
> 

Hi Bart,

> My understanding of the SCSI VPD code is as follows:
> * rcu_read_lock() / rcu_read_unlock() is used to prevent that another thread
>   updates a VPD buffer while it is being read.

My understanding is that it doesn't do that - you can update an RCU pointer with rcu_assign_pointer() after someone has called rcu_read_lock() and before they call rcu_read_unlock(). 

What rcu_read_lock() / rcu_read_unlock() do is mark a read-side critical section when accessing an RCU data item. If you have 2 CPUs in a read-side critical section and a 3rd CPU replacing the pointer using rcu_assign_pointer() one CPU could potentially end up with the old pointer and the other one with the new pointer or both old or both new (the only guarantee you have is that the pointer won't be partially updated with bits of old and the new pointer). To free the old pointer directly you have to call synchronize_rcu() after which you can call kfree() or if you don't call synchronize_rcu() you have to use a delayed freeing mechanism like kfree_rcu() so you can guarantee that the old pointer is still valid while used in a read-side critical section. Using something like kfree_rcu() means that you also don’t have to wait like I believe you can do if you call synchronize_rcu() since you could be forced to wait for a RCU grace period to end before you can call kfree().

So what they really do is ensure that someone updating the pointer can't free the old pointer (in case someone is using it) until everyone has left their read-side critical section (if the read-side critical section started before synchronize_rcu() was called).

You'll find a good example here that uses synchronize_rcu():

https://www.kernel.org/doc/Documentation/RCU/whatisRCU.txt

Search for " WHAT ARE SOME EXAMPLE USES OF CORE RCU API?" - it has a lot of other information about RCU.

> * All code that either updates or reads a VPD buffer holds a reference on
>   the SCSI device that buffer is associated with. That is why I think it is
>   not needed to use kfree_rcu() in scsi_device_dev_release_usercontext().

The only counter argument I'd put up into why it shouldn't be done the way you want to is that if someone else sees that code and doesn't understand the context and can't guarantee similar to this situation where all references to the structure should have already been dropped and think that it's ok to directly kfree something returned from rcu_dereference() when it's something that they really shouldn't do. The only real difference is that kfree_rcu will return the memory for reuse when RCU processing gets done in softirq and what you're doing will do it immediately. It's easier to use kfree_rcu() from what I can see.

It is possible I may be being too picky (it's a personal failing sometimes) but is it really that a large overhead to free the RCU pointers in a way that RCU pointers are expected to work even if the pointers shouldn't be accessible to anything?

Shane

> 
> Bart.
>