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
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
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
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:
- Don't use it in the drivers/ directory.
- Or don't use it without benchmarking it.
Could you point me to a document or some sort of reference?
Thanks, Thomas
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:
- Don't use it in the drivers/ directory.
- 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...
regards, dan carpenter
dri-devel@lists.freedesktop.org
-
Dan Carpenter -
Thomas Hellström