Re: [PATCH net-next] net: sched: flower: refactor reoffload for concurrent access

[Vlad Buslov <vladbu@mellanox.com>]

On Tue 23 Apr 2019 at 00:02, Jakub Kicinski <jakub.kicinski@netronome.com> wrote:
> On Mon, 22 Apr 2019 10:21:34 +0300, Vlad Buslov wrote:
>> Recent changes that introduced unlocked flower did not properly account for
>> case when reoffload is initiated concurrently with filter updates. To fix
>> the issue, extend flower with 'hw_filters' list that is used to store
>> filters that don't have 'skip_hw' flag set. Filter is added to the list
>> when it is inserted to hardware and only removed from it after being
>> unoffloaded from all drivers that parent block is attached to. This ensures
>> that concurrent reoffload can still access filter that is being deleted and
>> prevents race condition when driver callback can be removed when filter is
>> no longer accessible trough idr, but is still present in hardware.
>> 
>> Refactor fl_change() to respect new filter reference counter and to release
>> filter reference with __fl_put() in case of error, instead of directly
>> deallocating filter memory. This allows for concurrent access to filter
>> from fl_reoffload() and protects it with reference counting. Refactor
>> fl_reoffload() to iterate over hw_filters list instead of idr. Implement
>> fl_get_next_hw_filter() helper function that is used to iterate over
>> hw_filters list with reference counting and skips filters that are being
>> concurrently deleted.
>> 
>> Fixes: 92149190067d ("net: sched: flower: set unlocked flag for flower proto ops")
>> Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
>> Reported-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>
> Perhaps it'd be good to add an ASSERT_RTNL() (maybe with a comment?) 
> to fl_reoffload()?

Good idea.

>
>> @@ -382,6 +395,8 @@ static void fl_hw_destroy_filter(struct tcf_proto *tp, struct cls_fl_filter *f,
>>  
>>  	tc_setup_cb_call(block, TC_SETUP_CLSFLOWER, &cls_flower, false);
>>  	spin_lock(&tp->lock);
>> +	if (!list_empty(&f->hw_list))
>> +		list_del_init(&f->hw_list);
>
> Mm. I thought list_del_init() on an empty list should be fine?

Is it? Implementation of list_del_init() doesn't seem to check if list
is empty before re-initializing its pointers. Technically it seems like
it can work because the implementation will just set pointers of empty
list to point to itself (which is how empty list head is defined), but
should we assume this is intended behavior and not just implementation
detail? I don't see anything in comments for this function that suggests
that it is okay to call list_del_init() on empty list head.

>
>>  	tcf_block_offload_dec(block, &f->flags);
>>  	spin_unlock(&tp->lock);
>>