Re: [PATCH v6 net-next 3/9] net: ethtool: record custom RSS contexts in the XArray

[Przemek Kitszel <przemyslaw.kitszel@intel.com>]

On 6/25/24 15:39, Edward Cree wrote:
> On 20/06/2024 07:32, Przemek Kitszel wrote:
>> On 6/20/24 07:47, edward.cree@amd.com wrote:
>>> +    return struct_size((struct ethtool_rxfh_context *)0, data, flex_len);
>>
>> struct_size_t
> 
> Yup, will do, thanks for the suggestion.
> Don't think that existed yet when I wrote v1 :-D

yeah, but it's just a nitpick ATM

> 
>>> +    /* Update rss_ctx tracking */
>>> +    if (create) {
>>> +        /* Ideally this should happen before calling the driver,
>>> +         * so that we can fail more cleanly; but we don't have the
>>> +         * context ID until the driver picks it, so we have to
>>> +         * wait until after.
>>> +         */
>>> +        if (WARN_ON(xa_load(&dev->ethtool->rss_ctx, rxfh.rss_context))) {
>>> +            /* context ID reused, our tracking is screwed */
>>
>> why no error code set?
> 
> Because at this point the driver *has* created the context, it's
>   in the hardware.  If we wanted to return failure we'd have to
>   call the driver again to delete it, and that would still leave
>   an ugly case where that call fails.

driver is creating both HW context and ID at the same time, after
you call it from ethtool, eh :(

then my only concern is why do we want to keep old context instead of
update? (my only and last concern for this series by now)
say dumb driver always says "ctx=1" because it does not now better,
but wants to update the context

> 
>>
>>> +            kfree(ctx);
>>> +            goto out;
>>> +        }
>>> +        /* Allocate the exact ID the driver gave us */
>>> +        if (xa_is_err(xa_store(&dev->ethtool->rss_ctx, rxfh.rss_context,
>>> +                       ctx, GFP_KERNEL))) {
>>
>> this is racy - assuming it is possible that context was set by other
>> means (otherwisce you would not xa_load() a few lines above) -
>> a concurrent writer could have done this just after you xa_load() call.
> 
> I don't expect a concurrent writer - this is all under RTNL.
> The xa_load() is there in case we create two contexts
>   consecutively and the driver gives us the same ID both times.

thanks, makes sense with the other part of explanation :)

> 
>> so, instead of xa_load() + xa_store() just use xa_insert()
> 
> The reason for splitting it up is for the WARN_ON on the
>   xa_load().  I guess with xa_insert() it would have to be
>   WARN_ON(xa_insert() == -EBUSY)?

you need to handle both EBUSY and ENOMEM, both of which you are handling
by separate calls right now, but it is just stylistics at this point

> 
>> anyway I feel the pain of trying to support both driver-selected IDs
>> and your own