gnttab_end_foreign_access_ref() leaking grant entries?

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

gnttab_end_foreign_access_ref() leaking grant entries?

Pratyush Yadav-2
Hi everyone,

I was looking at gnttab_end_foreign_access_ref() and I notice that
while gnttab_end_foreign_access_ref() ends the foreign access, it does
not free the grant reference.

gnttab_end_foreign_access() free the reference by calling put_free_entry(ref).

gnttab_end_foreign_access_references() also frees the grant entries.

Shouldn't gnttab_end_foreign_access_ref() also free the grant entry?
It is an inconsistency at best and a bug at worst.

Is it ok if I submit a patch that calls put_free_entry(ref) in
gnttab_end_foreign_access_ref()?

--
Regards,
Pratyush Yadav
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-xen
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: gnttab_end_foreign_access_ref() leaking grant entries?

Roger Pau Monné-2
On Mon, Jun 18, 2018 at 06:11:00PM +0530, Pratyush Yadav wrote:

> Hi everyone,
>
> I was looking at gnttab_end_foreign_access_ref() and I notice that
> while gnttab_end_foreign_access_ref() ends the foreign access, it does
> not free the grant reference.
>
> gnttab_end_foreign_access() free the reference by calling put_free_entry(ref).
>
> gnttab_end_foreign_access_references() also frees the grant entries.
>
> Shouldn't gnttab_end_foreign_access_ref() also free the grant entry?
> It is an inconsistency at best and a bug at worst.

I think the point of using gnttab_end_foreign_access_ref is that it
doesn't free the reference. The naming convention of grant reference
related functions is just horrible, but I think this one is actually
clear, it just removes the foreign access permissions of this
reference, but it doesn't free it.

I think I would rather rename gnttab_end_foreign_access to
gnttab_end_foreign_access_and_free (and the same with the rest of the
end_foreign_ family of functions). This is more descriptive.

In any case, this is not a bug. Current netfront code makes use of
this function in order to keep a pool of grant references, so by
changing gnttab_end_foreign_access_ref to free the references you will
break netfront.

And now I realize this is something that you will have to take into
account for the busdma grant reference project. You will have to
introduce a flag that allows creating a dmamap with pre-allocated
references that are not freed until the map is destroyed. But let's
focus on that after the initial implementation is done.

Roger.
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-xen
To unsubscribe, send any mail to "[hidden email]"