Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

28 participants
7088 discussions

Re: [PATCH v7 3/5] rust: sync: Add abstraction for rcu_barrier()

by Gary Guo

On Wed Jul 29, 2026 at 6:18 PM BST, Paul E. McKenney wrote: > On Wed, Jul 29, 2026 at 11:45:40AM +0200, Philipp Stanner wrote: >> rcu_barrier() is a frequently used C function which is always safe to be >> called. > > Just checking... Here "always safe" means only from task level, with BH, > preemption, and interrupts all enabled, correct? > > In contrast, if you do this: > > preempt_disable(); > rcu_barrier(); > preempt_enable(); > > the results won't be safe. This is same for all sleepable functions. In order to avoid having to mark all sleeping function unsafe, we've decided that sleeping from non-preemptable context is "safe", but is a bug regardless. Best, Gary

3 weeks, 5 days

Re: [PATCH v4 09/14] nvme-pci: add SGL support for the dmabuf path

by Christoph Hellwig

On Wed, Jul 29, 2026 at 12:45:47PM +0100, Pavel Begunkov wrote: > It was exposed in early version as I was passing a [{dma,len}, ...] > array, but we moved from that. Maybe I should put the minimum > segment size in the map structure, and (possibly over) split using > that for now? Keith had this chunk in his patches: > > + int offset = offset_in_page(bio->bi_iter.bi_bvec_done); > + > + nsegs = ALIGN(bio->bi_iter.bi_size + offset, PAGE_SIZE) >> > + PAGE_SHIFT; > + if (bio->bi_iter.bi_size > max_bytes) { > + bytes = max_bytes; > + nsegs = (bytes + offset) >> PAGE_SHIFT; > + } else if (nsegs > lim->max_segments) { > + nsegs = lim->max_segments; > + bytes = PAGE_SIZE * nsegs - offset; > + } else { > + *segs = nsegs; > + return NULL; > + } This seems very pessimistic, especially for the case of the registration only having a single segment, which I'd expect to be fairly common due to P2P bar mappings, huge pages or IOMMU coalescing. So at very least we'd want to special case that, but in an idea world the caller would be required to provide a useful nr_segments for the I/O.

3 weeks, 6 days

Re: [PATCH v4 09/14] nvme-pci: add SGL support for the dmabuf path

by Christoph Hellwig

On Wed, Jul 29, 2026 at 03:47:23PM +0530, Anuj Gupta/Anuj Gupta wrote: > > But I also don't understand what the use case for this function > > is to start with. struct sg_table tells us how many segments > > exist on the DMA side in the nents member, which should be just > > fine for the SGL threshold calculation. > > sg_table->nents covers the entire exported buffer (<=1GiB), while a > request only covers a subrange[bi_offset, bi_offset+payload). Using > nents would overcount the request's segments. Urgg, yes. > >> + if (!entries) > >> + return BLK_STS_IOERR; > >> + if (entries > NVME_MAX_SEGS) > >> + return BLK_STS_AGAIN; > > > > Given that the block layer enforced data in rw/command and the > > max_segments limit, why do we need the extra check here? > > A dmabuf bio reports nsegs=1 (bio_split_io_at) to the block layer, so > max_segments isn't enforced against the SG entries actually spanned by > the request. Hence the explicit check. We'll need to expose the actual nsegs to the block layer and split based on that. Otherwise I/O might work or fail based on the device capabilities.

3 weeks, 6 days

Re: [PATCH v4 01/14] dma-buf: introduce initial file I/O infrastructure

by Christoph Hellwig

On Wed, Jul 29, 2026 at 11:37:19AM +0100, Pavel Begunkov wrote: > On 7/29/26 07:59, Christoph Hellwig wrote: >> The method name feels a bit convoluted, but given all the >> previous discussions I don't care too strongly. I'll leave >> the dma-buf side review to those who understand it. > > I assume you mean this: Yes. > > + int (*init_dma_buf_io_ctx)(struct file *, struct dma_buf_io_ctx *); > > I agree, and all dma_buf_io_[ctx,map] look clunky, but I don't > see what I can drop out of the name. Suggestions? Maybe I at least > should make the fs op sth like "register_dma_buf". I just remember scares from the last discussion :) register_dma_buf sounds fine to be, but unless I misremember there were objections to that before. > > -- > Pavel Begunkov ---end quoted text---

3 weeks, 6 days

Re: [PATCH v4 09/14] nvme-pci: add SGL support for the dmabuf path

by Christoph Hellwig

On Tue, Jul 28, 2026 at 10:29:21PM +0100, Pavel Begunkov wrote: > From: Anuj Gupta <anuj20.g(a)samsung.com> > > Add SGL support in addition to PRP for dmabuf-backed requests, > coalescing the mapping's sg_table into NVMe SGL data descriptors. > > Signed-off-by: Anuj Gupta <anuj20.g(a)samsung.com> > [pavel: rebased] > Signed-off-by: Pavel Begunkov <asml.silence(a)gmail.com> > --- > drivers/nvme/host/pci.c | 191 +++++++++++++++++++++++++++++++++++++++- > 1 file changed, 187 insertions(+), 4 deletions(-) > > diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c > index f1b67c191892..cbb321fb7c50 100644 > --- a/drivers/nvme/host/pci.c > +++ b/drivers/nvme/host/pci.c > @@ -1287,12 +1287,18 @@ static blk_status_t nvme_pci_setup_data_prp(struct request *req, > return BLK_STS_IOERR; > } > > +static void nvme_pci_sgl_set_data_addr(struct nvme_sgl_desc *sge, > + dma_addr_t addr, u32 len) > +{ > + sge->addr = cpu_to_le64(addr); > + sge->length = cpu_to_le32(len); > + sge->type = NVME_SGL_FMT_DATA_DESC << 4; > +} > + > static void nvme_pci_sgl_set_data(struct nvme_sgl_desc *sge, > struct blk_dma_iter *iter) > { > - sge->addr = cpu_to_le64(iter->addr); > - sge->length = cpu_to_le32(iter->len); > - sge->type = NVME_SGL_FMT_DATA_DESC << 4; > + nvme_pci_sgl_set_data_addr(sge, iter->addr, iter->len); > } The naming is a bit confusing (and me passing the iter to nvme_pci_sgl_set_data is probably at faul for that). So maybe spin out a prep patch to rename the old nvme_pci_sgl_set_data to nvme_pci_dma_iter_set_sgl or so, and then add the new one as nvme_pci_sgl_set_data (as before the dma_iter conversion). > > +static unsigned int nvme_pci_dmabuf_sgl_nents(struct request *req, > + dma_addr_t *first_dma, > + u32 *first_len) This is a really good example why the aligning to the opening braces produces totally unreadble code.. But I also don't understand what the use case for this function is to start with. struct sg_table tells us how many segments exist on the DMA side in the nents member, which should be just fine for the SGL threshold calculation. > +{ > + struct nvme_iod *iod = blk_mq_rq_to_pdu(req); > + struct bio *bio = req->bio; > + struct nvme_dmabuf_map *map = to_nvme_dmabuf_map(bio->bi_dmabuf_map); > + size_t length = blk_rq_payload_bytes(req); > + struct nvme_sgl_desc *sg_list = NULL; > + dma_addr_t sgl_dma = 0, last_end = 0; > + unsigned int mapped = 0; > + unsigned long tmp; > + struct scatterlist *sg; > + size_t offset, remaining; > + bool have = false; > + > + if (!entries) > + return BLK_STS_IOERR; > + if (entries > NVME_MAX_SEGS) > + return BLK_STS_AGAIN; Given that the block layer enforced data in rw/command and the max_segments limit, why do we need the extra check here? > + continue; > + } > + > + addr += offset; > + sg_len -= offset; > + offset = 0; > + > + while (sg_len && remaining) { These can't be false on the first iteration, so maybe turn this into a do {} while loop? > + u32 chunk = min_t(size_t, remaining, sg_len); > + > + if (have && last_end == addr) { > + u32 old = le32_to_cpu(sg_list[mapped - 1].length); > + > + sg_list[mapped - 1].length = cpu_to_le32(old + chunk); Overly long line. > + } else { > + if (WARN_ON_ONCE(mapped == entries)) > + goto err_free; > + nvme_pci_sgl_set_data_addr(&sg_list[mapped++], > + addr, chunk); > + } Why do we need this merging? dma_map_sg should have already done any interesting merging, or am I missing something? > + if (use_sgl != SGL_UNSUPPORTED) { > + dma_addr_t first_dma; > + u32 first_len; > + unsigned int entries; > + > + entries = nvme_pci_dmabuf_sgl_nents(req, &first_dma, > + &first_len); > + > + if (use_sgl == SGL_FORCED) { > + ret = nvme_rq_setup_dmabuf_sgl(req, nvmeq, > + entries, first_dma, first_len); > + return ret == BLK_STS_AGAIN ? BLK_STS_IOERR : ret; > + } > + > + if (sgl_threshold && entries && > + DIV_ROUND_UP(blk_rq_payload_bytes(req), entries) >= > + sgl_threshold) { > + ret = nvme_rq_setup_dmabuf_sgl(req, nvmeq, > + entries, first_dma, first_len); > + if (ret != BLK_STS_AGAIN) > + return ret; > + } > + } Various overly long lines. Please factor out a helper for the decisions to use sgl vs not instead of open coding it here.

3 weeks, 6 days

Re: [PATCH v4 08/14] nvme-pci: implement dma_token backed requests

by Christoph Hellwig

> +static int nvme_init_dma_buf_io_ctx(struct block_device *bdev, > + struct dma_buf_io_ctx *ctx) Please stick to two-tab indents for function declaration continuation for the nvme code to keep the code easy to maintain. > +#if defined(CONFIG_DMA_SHARED_BUFFER) This should be good old #ifdef. Same in a few other places over the series. Modulo these minor nits the patch looks good.

3 weeks, 6 days

Re: [PATCH v4 07/14] block: forward init_dma_buf_io_ctx to drivers

by Christoph Hellwig

Looks good: Reviewed-by: Christoph Hellwig <hch(a)lst.de>

3 weeks, 6 days

Re: [PATCH v4 06/14] block: introduce dma map backed bio type

by Christoph Hellwig

> - bio->bi_io_vec = bio_src->bi_io_vec; > + > + if (op_is_dmabuf(bio->bi_opf)) { > + bio->bi_dmabuf_map = bio_src->bi_dmabuf_map; > + } else { > + bio->bi_io_vec = bio_src->bi_io_vec; > + } No need for the braces. But given that these are the fields why even bother doing both sides and not rely on the union? > +void bio_dmabuf_map_set(struct bio *bio, struct iov_iter *iter) > +{ > + WARN_ON_ONCE(bio->bi_max_vecs); > + > + bio->bi_dmabuf_map = iter->dmabuf_map; > + bio->bi_vcnt = 0; > + bio->bi_iter.bi_offset = iter->iov_offset; > + bio->bi_iter.bi_size = iov_iter_count(iter); > + bio->bi_opf |= REQ_NOMERGE | REQ_DMABUF; This seems to be largely copied from bio_iov_bvec_set, but misses the REQ_CLONE there that we should probably set as well to indicate that the data descriptor is not owned (although not setting it is not really a bug). What about merging these two into a single and easier to use interface like this: diff --git a/block/bio.c b/block/bio.c index cc2bb9183c1a..25393bdd119d 100644 --- a/block/bio.c +++ b/block/bio.c @@ -860,12 +860,9 @@ static int __bio_clone(struct bio *bio, struct bio *bio_src, gfp_t gfp) bio->bi_write_hint = bio_src->bi_write_hint; bio->bi_write_stream = bio_src->bi_write_stream; bio->bi_iter = bio_src->bi_iter; - - if (op_is_dmabuf(bio->bi_opf)) { - bio->bi_dmabuf_map = bio_src->bi_dmabuf_map; - } else { - bio->bi_io_vec = bio_src->bi_io_vec; - } + static_assert(offsetof(struct bio, bi_io_vec) == + offsetof(struct bio, bi_dmabuf_map)); + bio->bi_io_vec = bio_src->bi_io_vec; if (bio->bi_bdev) { if (bio->bi_bdev == bio_src->bi_bdev && @@ -1186,26 +1183,21 @@ void __bio_release_pages(struct bio *bio, bool mark_dirty) } EXPORT_SYMBOL_GPL(__bio_release_pages); -void bio_iov_bvec_set(struct bio *bio, const struct iov_iter *iter) +bool bio_iov_iter_set(struct bio *bio, const struct iov_iter *iter) { WARN_ON_ONCE(bio->bi_max_vecs); + if (!iov_iter_is_bvec(iter) && !iov_iter_is_dmabuf_map(iter)) + return false; + bio->bi_io_vec = (struct bio_vec *)iter->bvec; bio->bi_iter.bi_idx = 0; bio->bi_iter.bi_offset = iter->iov_offset; bio->bi_iter.bi_size = iov_iter_count(iter); bio_set_flag(bio, BIO_CLONED); -} - -void bio_dmabuf_map_set(struct bio *bio, struct iov_iter *iter) -{ - WARN_ON_ONCE(bio->bi_max_vecs); - - bio->bi_dmabuf_map = iter->dmabuf_map; - bio->bi_vcnt = 0; - bio->bi_iter.bi_offset = iter->iov_offset; - bio->bi_iter.bi_size = iov_iter_count(iter); - bio->bi_opf |= REQ_NOMERGE | REQ_DMABUF; + if (iov_iter_is_dmabuf_map(iter)) + bio->bi_opf |= REQ_NOMERGE | REQ_DMABUF; + return true; } /* @@ -1265,13 +1257,7 @@ int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter, if (WARN_ON_ONCE(bio_flagged(bio, BIO_CLONED))) return -EIO; - if (iov_iter_is_bvec(iter)) { - bio_iov_bvec_set(bio, iter); - iov_iter_advance(iter, bio->bi_iter.bi_size); - return 0; - } - if (iov_iter_is_dmabuf_map(iter)) { - bio_dmabuf_map_set(bio, iter); + if (bio_iov_iter_set(bio, iter)) { iov_iter_advance(iter, bio->bi_iter.bi_size); return 0; } diff --git a/block/blk-map.c b/block/blk-map.c index d1d6bbe0ecf1..34816e6e64de 100644 --- a/block/blk-map.c +++ b/block/blk-map.c @@ -473,7 +473,7 @@ static int blk_rq_map_user_bvec(struct request *rq, const struct iov_iter *iter) bio = blk_rq_map_bio_alloc(rq, 0, GFP_KERNEL); if (!bio) return -ENOMEM; - bio_iov_bvec_set(bio, iter); + bio_iov_iter_set(bio, iter); ret = blk_rq_append_bio(rq, bio); if (ret) diff --git a/block/fops.c b/block/fops.c index d83cdbab65a6..97fb124a19b2 100644 --- a/block/fops.c +++ b/block/fops.c @@ -340,17 +340,12 @@ static ssize_t __blkdev_direct_IO_async(struct kiocb *iocb, bio->bi_end_io = blkdev_bio_end_io_async; bio->bi_ioprio = iocb->ki_ioprio; - if (iov_iter_is_bvec(iter)) { - /* - * Users don't rely on the iterator being in any particular - * state for async I/O returning -EIOCBQUEUED, hence we can - * avoid expensive iov_iter_advance(). Bypass - * bio_iov_iter_get_pages() and set the bvec directly. - */ - bio_iov_bvec_set(bio, iter); - } else if (iov_iter_is_dmabuf_map(iter)) { - bio_dmabuf_map_set(bio, iter); - } else { + /* + * Users don't rely on the iterator being in any particular state for + * async I/O returning -EIOCBQUEUED, hence we can avoid the expensive + * iov_iter_advance() if we could set the bvec/dmabuf directly. + */ + if (!bio_iov_iter_set(bio, iter)) { ret = blkdev_iov_iter_get_pages(bio, iter, bdev); if (unlikely(ret)) goto out_bio_put; diff --git a/include/linux/bio.h b/include/linux/bio.h index c053e9444514..1eff02a843ef 100644 --- a/include/linux/bio.h +++ b/include/linux/bio.h @@ -484,8 +484,7 @@ int bdev_rw_virt(struct block_device *bdev, sector_t sector, void *data, int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter, unsigned len_align_mask); -void bio_iov_bvec_set(struct bio *bio, const struct iov_iter *iter); -void bio_dmabuf_map_set(struct bio *bio, struct iov_iter *iter); +bool bio_iov_iter_set(struct bio *bio, const struct iov_iter *iter); void __bio_release_pages(struct bio *bio, bool mark_dirty); extern void bio_set_pages_dirty(struct bio *bio); extern void bio_check_pages_dirty(struct bio *bio);

3 weeks, 6 days

Re: [PATCH v4 05/14] block: move bvec init into __bio_clone

by Christoph Hellwig

Looks good: Reviewed-by: Christoph Hellwig <hch(a)lst.de>

3 weeks, 6 days

Re: [PATCH v4 04/14] block: always adjust bi_offset on bio_advance_iter

by Christoph Hellwig

Looks good: Reviewed-by: Christoph Hellwig <hch(a)lst.de>

3 weeks, 6 days

← Newer
1
...
8
9
10
11
12
13
14
...
709
Older →

Jump to page:

2026

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig