[PATCH 1/4] drm/ttm: add multihop infrastrucutre (v2)

[Christian König]

Am 10.11.20 um 18:11 schrieb Daniel Vetter:
> On Tue, Nov 10, 2020 at 4:48 PM Ville Syrjälä
> <ville.syrjala at linux.intel.com> wrote:
>> On Tue, Nov 10, 2020 at 03:24:32PM +1000, Dave Airlie wrote:
>>> On Tue, 10 Nov 2020 at 07:27, Ville Syrjälä
>>> <ville.syrjala at linux.intel.com> wrote:
>>>> On Mon, Nov 09, 2020 at 09:48:04PM +0100, Christian König wrote:
>>>>> Am 09.11.20 um 17:43 schrieb Ville Syrjälä:
>>>>>> On Mon, Nov 09, 2020 at 05:20:17PM +0100, Christian König wrote:
>>>>>>> Am 09.11.20 um 17:18 schrieb Ville Syrjälä:
>>>>>>>> On Mon, Nov 09, 2020 at 04:57:29PM +0100, Christian König wrote:
>>>>>>>>> Am 09.11.20 um 16:16 schrieb Ville Syrjälä:
>>>>>>>>>> On Wed, Nov 11, 2020 at 06:13:02PM +0100, Christian König wrote:
>>>>>>>>>>> Am 09.11.20 um 01:54 schrieb Dave Airlie:
>>>>>>>>>>>> @@ -1432,15 +1479,18 @@ int ttm_bo_swapout(struct ttm_operation_ctx *ctx)
>>>>>>>>>>>>        if (bo->mem.mem_type != TTM_PL_SYSTEM) {
>>>>>>>>>>>>                struct ttm_operation_ctx ctx = { false, false };
>>>>>>>>>>>>                struct ttm_resource evict_mem;
>>>>>>>>>>>> +             struct ttm_place hop = {};
>>>>>>>>>>> Please always use memset() if you want to zero initialize something in
>>>>>>>>>>> the kernel, we had enough trouble with that.
>>>>>>>>>> What trouble is that? I've not heard of anything, and we use
>>>>>>>>>> ={} quite extensively in drm land.
>>>>>>>>> ={} initializes only named fields, not padding.
>>>>>>>> Has that actually happened?
>>>>>>> YES! Numerous times!
>>>>>> You wouldn't happen to have links/etc. to the discussion?
>>>>>> I'd like to check them out.
>>>>> Uff, that was years ago. Just google for something like "mesa memset
>>>>> hash", there was recently (~ the last year) another discussion because
>>>>> somebody ran into exactly that problem once more.
>>>> Found this:
>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fmesa%2Fmesa%2F-%2Fissues%2F2071&data=04%7C01%7Cchristian.koenig%40amd.com%7C66be3d367f2b401b2e2808d8859ba4ee%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637406250838085232%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=X2z5zxsBvhXQvmd2oJzuN2YzDMGZnYUff6qxuJL%2BjLE%3D&reserved=0
>>>> which does suprise me a bit. Though I suspect ={} might
>>>> behave differently since the compiler might treat it
>>>> more like a memset().
>>> It doesn't there's a bit of info out there on what happens, it really
>>> only matters for structs we are passing to userspace, but it's
>>> unlikely to matter here,
>>> but I've changed this to memset in my local tree, because why not.
>> Structs passed to userspace should really have no implicit padding.
>> One of those how to botch your ioctl things...
>>
>> The main problems with memset() are:
>> - it's ugly
>> - people often get the sizeof wrong
>>
>> I guess the latter could be alleviated with a cpp macro that
>> does the sizeof correctly for you.
> Yeah imo not using = {} and instead insisting on memset is really bad
> w/a for not padding your api structs properly. memset is only one
> place where this goes horribly wrong.

I'm not a fan of memset either, but there are more problems than just 
the padding in the UAPI.

I've seen at least the following in the wild:
1. UAPI information leak.
2. Hash and fingerprinting functions returning unstable results even 
when all meaningful members of a structure are initialized.
3. Valgrind/KASAN/Coverity randomly pointing this out as problem.
4. There is the discussion if ={} (not ISO C conform) or ={0] or even 
={{0}} should be used.

My preference would be to just teach compilers that not initializing 
padding in the kernel is a undesired behavior which should be avoided.

Regards
Christian.

>
> I think for the gallium state tracker hasing issue memset is probably
> ok ot use for these specifically, with a big comment explaining why.
> And some code assurance that the memset is the same length you're
> passing to the crc function and all that. But anywhere were it's more
> (like anything related to api, which the gallium CSO hashing isnt) you
> really want to have no implicit holes at all.
> -Daniel