Messages in this thread | | | From | David Howells <> | Subject | Re: [PATCH v3 41/55] iscsi: Assume "sendpage" is okay in iscsi_tcp_segment_map() | Date | Tue, 25 Apr 2023 09:30:30 +0100 |
| |
Fabio M. De Francesco <fmdefrancesco@gmail.com> wrote:
> > - if (recv) { > > - segment->atomic_mapped = true; > > - segment->sg_mapped = kmap_atomic(sg_page(sg)); > > - } else { > > - segment->atomic_mapped = false; > > - /* the xmit path can sleep with the page mapped so use > kmap */ > > - segment->sg_mapped = kmap(sg_page(sg)); > > - } > > - > > + segment->atomic_mapped = true; > > + segment->sg_mapped = kmap_atomic(sg_page(sg)); > > As you probably know, kmap_atomic() is deprecated. > > I must admit that I'm not an expert of this code, however, it looks like the > mapping has no need to rely on the side effects of kmap_atomic() (i.e., > pagefault_disable() and preempt_disable() - but I'm not entirely sure about > the possibility that preemption should be explicitly disabled along with the > replacement with kmap_local_page()). > > Last year I've been working on several conversions from kmap{,_atomic}() to > kmap_local_page(), however I'm still not sure to understand what's happening > here... > > Am I missing any important details? Can you please explain why we still need > that kmap_atomic() instead of kmap_local_page()?
Actually, it might be worth dropping segment->sg_mapped and segment->data and only doing the kmap_local when necessary.
And this:
struct msghdr msg = { .msg_flags = flags }; struct kvec iov = { .iov_base = segment->data + offset, .iov_len = copy };
r = kernel_sendmsg(sk, &msg, &iov, 1, copy);
should really be using struct bvec, not struct kvec - then the mapping isn't necessary. It looks like this might be the only place the mapping is used, but I'm not 100% certain.
David
| |