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

On 6/25/24 15:39, Edward Cree wrote:

On 20/06/2024 07:32, Przemek Kitszel wrote:

quoted

On 6/20/24 07:47, edward.cree@amd.com wrote:

quoted

+    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

quoted

quoted

+    /* 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

quoted

quoted

+            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 :)

quoted

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

quoted

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