Fabio M. De Francesco <fmdefrancesco@xxxxxxxxx> 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
