Re: [PATCH v1 06/10] powerpc/pseries/iommu: Add ddw_list_add() helper

[Alexey Kardashevskiy <aik@ozlabs.ru>]

On 28/08/2020 08:11, Leonardo Bras wrote:
> On Mon, 2020-08-24 at 13:46 +1000, Alexey Kardashevskiy wrote:
>>>  static int find_existing_ddw_windows(void)
>>>  {
>>>  	int len;
>>> @@ -887,18 +905,11 @@ static int find_existing_ddw_windows(void)
>>>  		if (!direct64)
>>>  			continue;
>>>  
>>> -		window = kzalloc(sizeof(*window), GFP_KERNEL);
>>> -		if (!window || len < sizeof(struct dynamic_dma_window_prop)) {
>>> +		window = ddw_list_add(pdn, direct64);
>>> +		if (!window || len < sizeof(*direct64)) {
>>
>> Since you are touching this code, it looks like the "len <
>> sizeof(*direct64)" part should go above to "if (!direct64)".
> 
> Sure, makes sense.
> It will be fixed for v2.
> 
>>
>>
>>
>>>  			kfree(window);
>>>  			remove_ddw(pdn, true);
>>> -			continue;
>>>  		}
>>> -
>>> -		window->device = pdn;
>>> -		window->prop = direct64;
>>> -		spin_lock(&direct_window_list_lock);
>>> -		list_add(&window->list, &direct_window_list);
>>> -		spin_unlock(&direct_window_list_lock);
>>>  	}
>>>  
>>>  	return 0;
>>> @@ -1261,7 +1272,8 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn)
>>>  	dev_dbg(&dev->dev, "created tce table LIOBN 0x%x for %pOF\n",
>>>  		  create.liobn, dn);
>>>  
>>> -	window = kzalloc(sizeof(*window), GFP_KERNEL);
>>> +	/* Add new window to existing DDW list */
>>
>> The comment seems to duplicate what the ddw_list_add name already suggests.
> 
> Ok, I will remove it then.
> 
>>> +	window = ddw_list_add(pdn, ddwprop);
>>>  	if (!window)
>>>  		goto out_clear_window;
>>>  
>>> @@ -1280,16 +1292,14 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn)
>>>  		goto out_free_window;
>>>  	}
>>>  
>>> -	window->device = pdn;
>>> -	window->prop = ddwprop;
>>> -	spin_lock(&direct_window_list_lock);
>>> -	list_add(&window->list, &direct_window_list);
>>> -	spin_unlock(&direct_window_list_lock);
>>
>> I'd leave these 3 lines here and in find_existing_ddw_windows() (which
>> would make  ddw_list_add -> ddw_prop_alloc). In general you want to have
>> less stuff to do on the failure path. kmalloc may fail and needs kfree
>> but you can safely delay list_add (which cannot fail) and avoid having
>> the lock help twice in the same function (one of them is hidden inside
>> ddw_list_add).
>> Not sure if this change is really needed after all. Thanks,
> 
> I understand this leads to better performance in case anything fails.
> Also, I think list_add happening in the end is less error-prone (in
> case the list is checked between list_add and a fail).

Performance was not in my mind at all.

I noticed you remove from a list with a lock help and it was not there
before and there is a bunch on labels on the exit path and started
looking for list_add() and if you do not double remove from the list.


> But what if we put it at the end?
> What is the chance of a kzalloc of 4 pointers (struct direct_window)
> failing after walk_system_ram_range?

This is not about chances really, it is about readability. If let's say
kmalloc failed, you just to the error exit label and simply call kfree()
on that pointer, kfree will do nothing if it is NULL already, simple.
list_del() does not have this simplicity.


> Is it not worthy doing that for making enable_ddw() easier to
> understand?

This is my goal here :)


> 
> Best regards,
> Leonardo
> 

-- 
Alexey