re: drm/vmwgfx: Fix compat shader namespace

re: drm/vmwgfx: Fix compat shader namespace

[Dan Carpenter]

Hello Thomas Hellstrom,

The patch 18e4a4669c50: "drm/vmwgfx: Fix compat shader namespace"
from Jun 9, 2014, leads to the following static checker warning:

	drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c:477 vmw_cmd_res_reloc_add()
	warn: missing error code here? 'kzalloc()' failed.

drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
   468  
   469                  ret = vmw_resource_context_res_add(dev_priv, sw_context, res);
   470                  if (unlikely(ret != 0))
   471                          goto out_err;
   472                  node->staged_bindings =
   473                          kzalloc(sizeof(*node->staged_bindings), GFP_KERNEL);
   474                  if (node->staged_bindings == NULL) {
   475                          DRM_ERROR("Failed to allocate context binding "
   476                                    "information.\n");
   477                          goto out_err;

This should just be "return -ENOMEM;".  The goto is misleading because
you expect it to do something useful.

Soon checkpatch.pl will start complaining about the extra DRM_ERROR()
because kzalloc() has a more useful printk builtin and this just wastes
memory and makes the code more verbose.

Speaking of verbose, all the likely/unlikely annotations should be
removed.  If the code were more readable then the missing error code
would have been more noticeable.  This code is buggy because it is ugly;
there is a direct cause effect relationship.

   478                  }
   479                  INIT_LIST_HEAD(&node->staged_bindings->list);
   480          }
   481  
   482          if (p_val)
   483                  *p_val = node;
   484  
   485  out_err:
   486          return ret;
   487  }

regards,
dan carpenter

Re: drm/vmwgfx: Fix compat shader namespace

[Thomas Hellström]

On 2014-07-09 14:48, Dan Carpenter wrote:
> Hello Thomas Hellstrom,
>
> The patch 18e4a4669c50: "drm/vmwgfx: Fix compat shader namespace"
> from Jun 9, 2014, leads to the following static checker warning:
>
> 	drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c:477 vmw_cmd_res_reloc_add()
> 	warn: missing error code here? 'kzalloc()' failed.
>
> drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
>     468
>     469                  ret = vmw_resource_context_res_add(dev_priv, sw_context, res);
>     470                  if (unlikely(ret != 0))
>     471                          goto out_err;
>     472                  node->staged_bindings =
>     473                          kzalloc(sizeof(*node->staged_bindings), GFP_KERNEL);
>     474                  if (node->staged_bindings == NULL) {
>     475                          DRM_ERROR("Failed to allocate context binding "
>     476                                    "information.\n");
>     477                          goto out_err;
>
> This should just be "return -ENOMEM;".  The goto is misleading because
> you expect it to do something useful.

Indeed. Thanks for pointing that out. Since this is old code being 
reorganized, the goto slipped through. The missing error code has been 
around for a while, though. I'll put together a patch for that.

>
> Soon checkpatch.pl will start complaining about the extra DRM_ERROR()
> because kzalloc() has a more useful printk builtin and this just wastes
> memory and makes the code more verbose.
Noted.

>
> Speaking of verbose, all the likely/unlikely annotations should be
> removed.

Is this your personal opinion or has there been some kind of kernel 
developer agreement not to add this annotation and remove it from the 
kernel tree? If not, I prefer to keep it.

>    If the code were more readable then the missing error code
> would have been more noticeable.  This code is buggy because it is ugly;
> there is a direct cause effect relationship.
I think ugliness in this case is in the eye of the beholder. The bug 
likely entered long ago like these bugs tend to do because you're not 
100% focused when the code is written. I find this statement a bit 
incoherent because there's no branch prediction hint in the if statement 
preceding the bug and although the error message may be redundant in 
this case, I can't see why an error message would make the code ugly or 
be the cause of a bug.

>
>     478                  }
>     479                  INIT_LIST_HEAD(&node->staged_bindings->list);
>     480          }
>     481
>     482          if (p_val)
>     483                  *p_val = node;
>     484
>     485  out_err:
>     486          return ret;
>     487  }
>
> regards,
> dan carpenter

Thanks,
Thomas

Re: drm/vmwgfx: Fix compat shader namespace

[Dan Carpenter]

On Wed, Jul 09, 2014 at 11:31:45PM +0200, Thomas Hellström wrote:
> >Speaking of verbose, all the likely/unlikely annotations should be
> >removed.
> 
> Is this your personal opinion or has there been some kind of kernel
> developer agreement not to add this annotation and remove it from
> the kernel tree? If not, I prefer to keep it.

It obviously makes the code less readable.  It makes a small speedup if
the code is called 10000 times with the and the expected value is true
every time.  If more than 1 out of 10000 values is unexpected then it is
a slow down.

There are two rules of thumb for likely/unlikely:

	1) Don't use it in the drivers/ directory.
	2) Or don't use it without benchmarking it.

These are general rules, not mine.

In the olden days we used to use it more often but then people did
benchmarking and likely/unlikely annotations didn't make a single
measurable difference on normal benchmarks at all.  Maybe on a micro
benchmark.  Also perhaps in those days people hadn't done branch
profiling so we were getting a lot of unexpected conditions and the slow
downs were canceling the speed ups.

regards,
dan carpenter

Re: drm/vmwgfx: Fix compat shader namespace

[Thomas Hellström]

On 2014-07-10 11:33, Dan Carpenter wrote:
> On Wed, Jul 09, 2014 at 11:31:45PM +0200, Thomas Hellström wrote:
>>> Speaking of verbose, all the likely/unlikely annotations should be
>>> removed.
>> Is this your personal opinion or has there been some kind of kernel
>> developer agreement not to add this annotation and remove it from
>> the kernel tree? If not, I prefer to keep it.
> It obviously makes the code less readable.  It makes a small speedup if
> the code is called 10000 times with the and the expected value is true
> every time.  If more than 1 out of 10000 values is unexpected then it is
> a slow down.

You may be right, but most people citicising branch prediction hints 
tend to stare blindly at probabilities and forget about fastpath 
optimization where you don't care at all about the execution speed of 
the slowpath and therefore hint the compiler to take the fastpath branch 
regardless of probabilities. My guess is it would be pretty hard to do 
that incorrectly.

I agree, however that readability may be more important than the 
fastpath execution speed gained from this. But in my case not so much 
that I spontaneously feel like removing all branch prediction hints from 
the vmwgfx driver.

>
> There are two rules of thumb for likely/unlikely:
>
> 	1) Don't use it in the drivers/ directory.
> 	2) Or don't use it without benchmarking it.

Could you point me to a document or some sort of reference?

Thanks,
Thomas

Re: drm/vmwgfx: Fix compat shader namespace

[Dan Carpenter]

On Fri, Jul 11, 2014 at 12:14:25AM +0200, Thomas Hellström wrote:
> 
> I agree, however that readability may be more important than the
> fastpath execution speed gained from this. But in my case not so
> much that I spontaneously feel like removing all branch prediction
> hints from the vmwgfx driver.

I'm just saying, let's not add new ones.

> 
> >
> >There are two rules of thumb for likely/unlikely:
> >
> >	1) Don't use it in the drivers/ directory.
> >	2) Or don't use it without benchmarking it.
> 
> Could you point me to a document or some sort of reference?

http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2012-March/025252.html

regards,
dan carpenter