Date: Mon, 15 Sep 2008 09:55:25 +0300 From: Kostik Belousov <kostikbel@gmail.com> To: Robert Watson <rwatson@freebsd.org> Cc: rnoland@freebsd.org, current@freebsd.org, "Yair K." <cesium2@gmail.com>, vehemens <vehemens@verizon.net> Subject: Re: cdevpriv and mmap(2) Message-ID: <20080915065525.GD39652@deviant.kiev.zoral.com.ua> In-Reply-To: <alpine.BSF.1.10.0809142004480.5662@fledge.watson.org> References: <20080914174801.GC39652@deviant.kiev.zoral.com.ua> <alpine.BSF.1.10.0809142004480.5662@fledge.watson.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--SlNwLc1tujQOaj7L
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
On Sun, Sep 14, 2008 at 08:15:04PM +0100, Robert Watson wrote:
>=20
> On Sun, 14 Sep 2008, Kostik Belousov wrote:
>=20
> >When implementing cdevpriv, I completeley missed the support for d_mmap(=
)=20
> >driver method. This method is called by the kernel (at least) twice: one=
=20
> >time to validate the mmaped region and protection mode (see=20
> >vm/device_pager.c:dev_pager_alloc()), and second time to obtain the=20
> >physical address when serving page fault (see dev_pager_getpages()).
> >
> >Support for cdevpriv for the first call(s) is trivial, and is implemente=
d=20
> >by the patch below. Second calls are much harder and essentially require=
=20
> >attaching cdevpriv bookkeeping data to the struct vm_map_entry. In fact,=
I=20
> >am not sure whether this support for the second time calls is needed at=
=20
> >all in real usage.
> >
> >I Cc:ed people that pointed me to the issue, please evaluate the patch=
=20
> >against your needs. I think I will commit it shortly after your feedback=
.=20
> >On the other hand, I would think about implementing full support for=20
> >d_mmap only if actually needed.
>=20
> I'm not really sure that these changes "make sense" given the way our=20
> device pager works right now. My understanding is that most consumers of=
=20
> cdevpriv use it in order to provide session-centric device nodes as a=20
> cleaner alternative to cloning. However, even with your change, it's not=
=20
> possible to support session-centric memory mapping of devices as the memo=
ry=20
> map and device pager code assumes there is one VM object for each device,=
=20
> and hence all pages mapped independently from different file descriptors =
on=20
> the same device are from the same set of pages (and hence in the same VM=
=20
> object page cache). In order to implement session-centric semantics, I=
=20
> think it's actually quite a bit more complicated than just adding=20
> vm_map_entry book-keeping -- we also need to have a different VM object f=
or=20
> each session.
>=20
> And, arguably, we need a more mature device_pager that understands that=
=20
> pages might someday no longer be associated with a device due to that=20
> device being removed...
The issues you raised are obviously important, but IMHO they are
ortogonal to the cdevpriv KPI working in for _any_ pager.
Pager' getpages method is usually called from the context where kernel
does not have naturally supplied filedescriptor. For instance, most
typical caller if vm_fault(). Thus, whatever pager is used, we have to
provide a way for kernel to somehow associate struct file with faulted
page (region), and make that file available to the pager.
[Hmm, because kernel is allowed to fault too, vm_fault() should
save/restore td_fpop.]
Another point is that we have important consumers of the existing device
pager interface that already want to use cdevpriv. Their usage ATM
is limited to authentification, whatever it means. I assume it means
checking that the caller was given some token the validation step. The
code should be structured approx. like this:
dri_mmap(...)
{
some_dri_data *p;
int error;
error =3D devfs_get_cdevpriv(&p);
if (error =3D=3D 0) {
/* authenticate; */
}
/*
* Auth successfull or error =3D=3D EBADF
* Do whatever needed to return phys address
*/
...
}
And, the last issue you raised, the need for the new pager is actually
real for GEM/TTM or whatever the newest DRI interface is called. I have
an intent to play with it, but more smart thing would be to wait some
time more. Hopefully, then DRI folks will finally decide on the (more)
stable interface. I am sure that it would be changed several dozen
times in the future, but have a hope that it will not be designed from
scratch.
--SlNwLc1tujQOaj7L
Content-Type: application/pgp-signature
Content-Disposition: inline
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (FreeBSD)
iEYEARECAAYFAkjOBtwACgkQC3+MBN1Mb4iaRwCglRl560YvTHhWavJeI7Ih0C7C
MJcAnjX9IubccQVwoYmUamaJ0oPBP3wO
=p2bF
-----END PGP SIGNATURE-----
--SlNwLc1tujQOaj7L--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20080915065525.GD39652>