On Fri, Jul 16, 2021 at 05:02:33PM +0800, Hou Tao wrote:
> > Cachelines don't guarantee anything, you can get partial forwards.
>
> Could you please point me to any reference ? I can not google
>
> any memory order things by using "partial forwards".
I'm not sure I have references, but there are CPUs that can do, for
example, store forwarding at a granularity below cachelines (ie at
register size).
In such a case a CPU might observe the stored value before it is
committed to memory.
> >>> @@ -224,7 +224,11 @@ static void blkdev_bio_end_io_simple(struct bio *bio)
> >>> {
> >>> struct task_struct *waiter = bio->bi_private;
> >>>
> >>> - WRITE_ONCE(bio->bi_private, NULL);
> >>> + /*
> >>> + * Paired with smp_load_acquire in __blkdev_direct_IO_simple()
> >>> + * to ensure the order between bi_private and bi_xxx
> >>> + */
> > This comment doesn't help me; where are the other stores? Presumably
> > somewhere before this is called, but how does one go about finding them?
>
> Yes, the change log is vague and it will be corrected. The other stores
>
> happen in req_bio_endio() and its callees when the request completes.
Aaah, right. So initially I was wondering if it would make sense to put
the barrier there, but having looked at this a little longer, this
really seems to be about these two DIO methods.
> > The Changelog seems to suggest you only care about bi_css, not bi_xxx in
> > general. In specific you can only care about stores that happen before
> > this; is all of bi_xxx written before here? If not, you have to be more
> > specific.
>
> Actually we care about all bi_xxx which are written in req_bio_endio,
> and all writes to bi_xxx happen before blkdev_bio_end_io_simple().
> Here I just try to use bi_status as one example.
I see req_bio_endio() change bi_status, bi_flags and bi_iter, but afaict
there's more bi_ fields.
> >>> @@ -283,7 +287,8 @@ __blkdev_direct_IO_simple(struct kiocb *iocb, struct iov_iter *iter,
> >>> qc = submit_bio(&bio);
> >>> for (;;) {
> >>> set_current_state(TASK_UNINTERRUPTIBLE);
> >>> - if (!READ_ONCE(bio.bi_private))
> >>> + /* Refer to comments in blkdev_bio_end_io_simple() */
> >>> + if (!smp_load_acquire(&bio.bi_private))
> >>> break;
> >>> if (!(iocb->ki_flags & IOCB_HIPRI) ||
> >>> !blk_poll(bdev_get_queue(bdev), qc, true))
> > That comment there doesn't help me find any relevant later loads and is
> > thus again inadequate.
> >
> > Here the purpose seems to be to ensure the bi_css load happens after the
> > bi_private load, and this again is cheaper done using smp_rmb().
> Yes and thanks again.
> >
> > Also, the implication seems to be -- but is not spelled out anywhere --
> > that if bi_private is !NULL, it is stable.
>
> What is the meaning of "it is stable" ? Do you mean if bi_private is NULL,
> the values of bi_xxx should be ensured ?
With stable I mean that if it is !NULL the value is always the same.
I've read more code and this is indeed the case, specifically, here
bi_private seems to be 'current' and will only be changed to NULL.
