Hi,
Stefan Rompf <[EMAIL PROTECTED]> writes:
> ok, seems we are getting near submission for kernel inclusion. If no new
> comments arise, the only thing missing from my side is documentation.
I understand it's supposed to be race and "race" free?
> + unsigned char operstate; /* RFC2863 operstate */
> + unsigned char link_mode; /* mapping policy to operstate */
Still no atomic type? Why? Hmm...
> + * The name carrier is inappropriate, these functions should really be
> + * called netif_lowerlayer_*() because they represent the state of any
> + * kind of lower layer not just hardware media.
Right.
> +static inline void netif_dormant_on(struct net_device *dev)
> +{
> + if (!test_and_set_bit(__LINK_STATE_DORMANT, &dev->state))
> + linkwatch_fire_event(dev);
> +}
This is of course much much more correct than those strange operations
on dev->char_type, though netif_dormant* is still misnamed - dormant
isn't one bit but rather a combination of bits: admin up, carrier
(lower level etc.) up and (I call it) protocol down.
This bit alone doesn't tell if it's dormant or, for example, simply down
(either admin or carrier).
While I haven't fully analyzed the remaining part of the patch... Tell
me, do you think adding more code can at last break the theory? Do you
think you can avoid *real* locking (as opposed to protecting a write to
a variable) especially when userspace is involved (i.e., there is no
serialization and everything is asynchronous)?
How do you protect against the scenario I have already outlined few times:
- kernel driver signals lower-level down
- kernel driver signals lower-level up (dormant = 1)
- slow userspace (in this case, could be a kernel module if strict
serialization of all operational status changes isn't required) doesn't
(yet) see the above events and sets "fully operational"?
Look, you (I mean Thomas and Jamal, too) can ignore that problem (or
"problem"), pretend it's fixed when it's not, exactly like you ignore
existence of my patch.
However, in case it's unintentional (hard to believe maybe but I trust
people), I will tell you this secret again - you basically have two
options:
1. Ignore the "races" (not real races) and let every layer do its
independent flag management (like in current kernels and with my,
of course nonexistent, patch) - all useless code goes.
2. *really* serialize all calls. For userspace (either strict
serialization nor locking isn't possible) all messages must contain
some sequential number and the kernel part must ignore late messages
from userspace (this is more complicated if the userspace is allowed
to set, say, carrier status - probably not the case today but who
knows for tomorrow).
As I already said, both are possible, with #2 orders of magnitude more
complicated and thus prone to bugs etc. - with no real gain. Kernel
bloat, too.
--
Krzysztof Halasa
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
