linux-kernel - Re: [RFC 00/29] De-stage android's sync framework

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <569FA187.2060601@linux.intel.com>
Date:	Wed, 20 Jan 2016 16:02:31 +0100
From:	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>
To:	Gustavo Padovan <gustavo@...ovan.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	devel@...verdev.osuosl.org, daniels@...labora.com,
	Daniel Vetter <daniel.vetter@...ll.ch>,
	Riley Andrews <riandrews@...roid.com>,
	dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
	Arve Hjønnevåg <arve@...roid.com>,
	Gustavo Padovan <gustavo.padovan@...labora.co.uk>,
	John Harrison <John.C.Harrison@...el.com>
Subject: Re: [RFC 00/29] De-stage android's sync framework

Op 20-01-16 om 15:32 schreef Gustavo Padovan:
> 2016-01-20 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>:
>
>> Hey,
>>
>> Op 15-01-16 om 15:55 schreef Gustavo Padovan:
>>> From: Gustavo Padovan <gustavo.padovan@...labora.co.uk>
>>>
>>> This patch series de-stage the sync framework, and in order to accomplish that
>>> a bunch of cleanups/improvements on the sync and fence were made.
>>>
>>> The sync framework contained some abstractions around struct fence and those
>>> were removed in the de-staging process among other changes:
>>>
>>> Userspace visible changes
>>> -------------------------
>>>
>>>  * The sw_sync file was moved from /dev/sw_sync to <debugfs>/sync/sw_sync. No
>>>  other change.
>>>
>>> Kernel API changes
>>> ------------------
>>>
>>>  * struct sync_timeline is now struct fence_timeline
>>>  * sync_timeline_ops is now fence_timeline_ops and they now carry struct
>>>  fence as parameter instead of struct sync_pt
>>>  * a .cleanup() fence op was added to allow sync_fence to run a cleanup when
>>>  the fence_timeline is destroyed
>>>  * added fence_add_used_data() to pass a private point to struct fence. This
>>>  pointer is sent back on the .cleanup op.
>>>  * The sync timeline function were moved to be fence_timeline functions:
>>> 	 - sync_timeline_create()	-> fence_timeline_create()
>>> 	 - sync_timeline_get()		-> fence_timeline_get()
>>> 	 - sync_timeline_put()		-> fence_timeline_put()
>>> 	 - sync_timeline_destroy()	-> fence_timeline_destroy()
>>> 	 - sync_timeline_signal()	-> fence_timeline_signal()
>>>
>>>   * sync_pt_create() was replaced be fence_create_on_timeline()
>>>
>>> Internal changes
>>> ----------------
>>>
>>>  * fence_timeline_ops was removed in favor of direct use fence_ops
>>>  * fence default functions were created for fence_ops
>>>  * removed structs sync_pt, sw_sync_timeline and sw_sync_pt
>>>
>>> Gustavo Padovan (29):
>>>   staging/android: fix sync framework documentation
>>>   staging/android: fix checkpatch warning
>>>   staging/android: rename sync_fence_release
>>>   staging/android: rename 'android_fence' to 'sync_fence'
>>>   staging/android: remove not used sync_timeline ops
>>>   staging/android: create a 'sync' dir for debugfs information
>>>   staging/android: move sw_sync file to debugfs file
>>>   staging/android: Remove WARN_ON_ONCE when releasing sync_fence
>>>   staging/android: rename struct sync_fence's variables to 'sync_fence'
>>>   staging/android: rename 'sync_pt' to 'fence' in struct sync_fence_cb
>>>   dma-buf/fence: move sync_timeline to fence_timeline
>>>   staging/android: remove struct sync_pt
>>>   dma-buf/fence: create fence_default_enable_signaling()
>>>   dma-buf/fence: create fence_default_release()
>>>   dma-buf/fence: create fence_default_get_driver_name()
>>>   dma-buf/fence: create fence_default_timeline_name()
>> This is misleading. I think timeline_fence prefix would be more appropriate here.
> Why? These fence_default_.. functions are fence_ops and not related to
> fence_timeline in any way.
Because they're using fence_parent, which should probably be renamed to fence_to_timeline()

The name makes it sound as if they could apply to all type of fences, I don't think this is the case.
>> I also believe this should be done in multiple series. First series should de-stage the userspace fence framework. The next series should fix up android_fence and maybe rename it to timeline_fence since sync_fence is already used for the userspace fd, which would add more confusion?
> Sure. I've been thinking on how to split this properly. I'm trying to
> add a bunch of clean up/renaming first, eg the sync_fence rename to
> sync_file that Daniel Vetter and I discussed. 
>
> Next my plan would be move sync_timeline to fence_timeline, add the
> fence_default.. fence_ops, clean up sw_sync and finally merge
> fence_context and fence_timeline.
>
> Looking at how sync and fence It looks easier to me to de-stage sync_timeline first than userspace
> fence.
There's already code to add a sync_fence_create_dma export [1][2]. So if you want to de-stage it then there will be users for it.

sync_pt otoh has no upstream in-kernel users. It was a wrapper to keep android drivers api compatible with the fence api.

[1] https://patchwork.freedesktop.org/patch/67845/
[2] https://patchwork.freedesktop.org/patch/67846/