Sowmini.Varadhan at Sun.COM wrote: >> usr/src/lib/libdladm/common/linkprop.c >> -------------------------------------- >> >> * 2027: I find it surprising that this is the applicable range for >> Ethernet. For one, I'd expect to be able to lower the MTU of an >> Ethernet link. Another issue is the limit of 9000. Is that a >> universal hard limit for all Ethernet links? E.g., why shouldn't I >> be allowed to write a DL_ETHER driver which I can play around with >> MTUs higher than 9000 without having to recompile libdladm? > > The actual minimum size of an ethernet frame is 64 bytes. > (Destination Address (6 bytes) + Source Address (6 bytes) + > Frame Type (2 bytes) + Data (46 bytes) + CRC Checksum (4 bytes)). > However, all of of our drivers clamp the mtu to > 1500 - see CR 6618011.
Right, so given that the clamping to a minimum of 1500 by bge is a bug, why would libdladm legitimize that be stating that the minimum for all Ethernet links is 1500? If there are different possible values per driver, then perhaps something is missing to obtain those possible values per-link? > As for the 9000 limit 9,000 bytes is the conventional jumbo frame > size. There are issues to going to Super Jumbo Frame: see > http://en.wikipedia.org/wiki/Jumbo_Frames Right, I know that it's the convention from jumbo frame. What I worry about is 3rd party driver developers working on something outside of the ON consolidation (so they don't have the option of recompiling libdladm), perhaps "super jumbo frame". They can't do that, since libdladm artificially limits the MTU to 9000. This doesn't seem like a good thing to me. >> * 2047: Comment explaining the structure of buf and how this size is >> determined. For example, I can't guess what MAXLINELEN is doing in >> there. > > I've added comments, and in the process of doing so, realized that > MAXLINELEN was actually unnecessary. Okay, thanks. > >> * 2053: This looks very fragile. It might be easier to follow if you >> had a valptr pointer which you initially set to >> >> (buf + sizeof (char *) * DLADM_MAX_PROP_VALCNT) >> >> before the loop, and incremented independently by DLADM_PROP_VAL_MAX >> in the loop. You could then just set prop_vals[i] to valptr. > > Actually this code is replicated in multiple parts of libdladm, and > I'd like to leave it as is, so that it can all be fixed in one shot later, > maybe even captured into a common function. Okay. >> usr/src/uts/common/io/bge/bge_main2.c >> ------------------------------------- >> >> * 1062-1256: This function (and its friend bge_get_priv_prop()) would >> benefit from a table of per-property default values to reduce the >> explosion of nearly duplicate code. > > I had a discussion with the NIC-China team about this. Having a table > is not a general solution for all drivers, because, for example, in > the case of e1000g, the hardware bits in the driver have to be read > to figure out the default. Even in the bge case (where all supported > hardware types have the same defaults in many cases) there are dependancies > on whether the hardware is Copper/Fiber (i.e., CHIP_FLAG_SERDES checks). > We'll think about this further, but so far, there doesn't seem any clear-cut > way to shrink this logic.. Okay. >> * 3450: The reference to mac_add_name() is out of place here. Driver >> developers shouldn't need to to care about a private implementation >> detail of the mac module. > > fixed (as a result of other changes to this interface). > See comments about mac_register_priv_prop Great! > >> usr/src/uts/common/io/bge/bge_ndd.c >> ----------------------------------- >> >> * 93: I don't understand the reference to the parameter being writable >> when this is a get function. Can you explain? > > relic from the previous ndd code which used to use a single-character > encoding at the start of the ndd param name to track writability of the > property. Comment has been fixed. Okay. >> usr/src/uts/common/io/mac/mac.c >> ------------------------------- >> >> * 1059,1064: I don't think that this does any good. Only Solaris >> developers use DEBUG kernels, and this message is targetted at >> administrators who either use ndd or have written scripts that use >> ndd. The message is also not useful in itself. There's no way to >> correlate the message with a specific command that was used, so >> someone will see this in syslog and wonder where it came from. I >> would suggest hacking something into the ndd command itself so that >> the thing issuing the command gets something printed to its stdio, >> and not a syslog message on a debug kernel. > > This is a hot topic for debate. Fixing it in ndd is not a good solution > because we don't want to print the message for drivers that have not > been converted yet, and besides, there are applications like sunvts > that issue the ndd ioctls directly, which would also benefit from > the message. Dropping the message completely is also not totally > satisfactory, as Garrett pointed out, because it's probably good > to alert people about the change if possible. Leaving the message > in non-debug kernels will result in syslog flooding, so that's not > a good solution either. I chose the compromise of putting the > message in debug kernels only, but perhaps completely dropping > the message is a better approach. Given that the message itself doesn't allow one to correlate it with any ioctl or command that issued it, I wouldn't mind if it wasn't printed at all. > mac.c: >> * 1883,1888: The idea that these are registered by >> mac_register_priv_prop(), and unregistered using mac_unregister() is >> asymetric. It would be cleaner if drivers could provide a list of >> private properties in mac_register_t using mac_register(), much like >> they provide a list of callbacks. > > mac_ndd.c: >> * 516: I don't see why this can't be replaced by a table that's passed >> in mac_register_t. Is there a reason why we chose on registering >> individual properties like this? > > Actually, you may have missed the subsequent email discussion where > the mac_register_priv_prop interface was changed. See > > http://mail.opensolaris.org/pipermail/brussels-dev/2008-March/000795.html Yeah, I saw that, and that was certainly an improvement over the one property per register call. > However, as you point out, even this interface can be optimized further > by just passing the table as part of mac_register. So the comment > is accepted. Exactly, I didn't see the need for a separate registration mechanism. Thanks for fixing this. (I assume that you'll update the PSARC case or file an addendum to document these changes) > >> usr/src/uts/common/io/mac/mac_ndd.c >> ----------------------------------- >> >> * 67-161: These are all Ethernet specific. The mac module is designed >> to be Media agnostic, which is why we introduced a MAC-Type plugin >> architecture. It looks like there's some missing architecture to >> make use of the plugin interfaces and the mac_ether plugin, and it >> doesn't look like it would be difficult to implement that. Plugins >> already register a table of media-specific statistics, and this is >> not at all that different. It's also shouldn't be all that >> different from supporting a table of private properties registered >> by the driver. >> >> * 163: This isn't the size, but a count. (MAC_MII_PROP_COUNT) > > I can easily make this to be part of the mactype plugin, but there > is one danger to this though: the ndd interface is currently only > used to diddle the MII props of ethernet interfaces. When we put > in hooks for the ndd mapping table into the mactype_register_t we > provide a way for other linktypes to use ndd to map from ndd > invocations to dladm invocations. I don't see anyone reasonably wanting > to use this backdoor, but it does provide another new > way to use ndd. I think as long as you document the interface properly (at a minimum by using clear comments in the code, which you've done in mac.h), then this shouldn't be a problem. I think this issue is outweighed by keeping the core of GLDv3 Media-agnostic. I see that you've done this in your updated webrev, and I like the result. >> * 194: why 5? >> * 243: do we know that this isn't past the end of the dblk? > > As I was parsing the above two comments, I realised there's a possible > bug in line 243: that should be 6, and it comes from the count of > space, (, ) and terminating nulls in > > (void) snprintf(cp, len, "%s (%s)", name, rwtag); > > I've clarified this with comments. Great. >> * 340: this ASSERT() looks bogus, but maybe I'm not looking closely >> enough. What's to prevent random joe admin from panicking the >> system by typing ndd -get ... on a driver which doesn't supply a >> getprop() callback? > > Actually we check for this in mac_ioctl (which checks the mc_callbacks), > so the ASSERT should hold. Ah, yes, you're correct. >> * 381: This is fragile, and will break when someone adds another >> 64-bit property and doesn't realize that this code exists. > > The important point to keep in mind is that the ndd<->driver interface > itself can only set/get 32 bit values which is another limitation > to using ndd for drivers. Which is why the speed is scaled down for > kstat/ndd reporting. Since we don't expect to add any more parameters > to ndd itself the above danger does not hold for this path. Okay. One minor nit on the updated webrev is that mactype_alloc() initializes optional fields to appropriate values, so you don't need to update the plugins that don't support ndd compatibility properties to set mtr_mapping and mtr_mappingcount explicitly to NULL and 0. They're both allocated that way by default. Thanks, -Seb
