Hi Cathy,
Cathy Zhou wrote:
>> Change the link ID to name mapping such that the ID for name1 now
>> refers to name2. Note that the link's ID does not change, only the
>> name associated with it. The rationale behind the rename call is that
>> the persistent configuration API does not affect the link ID to name
>> mapping that's currently in memory.
>>
> One thing regarding the above API is whether the link management module
> needs to have the knowledge of whether the link is active or not or the
> consumer need to track that knowledge. For example:
>
> step 1. "dladm create-aggr -d bge1 aggr1" to create both the active and
> the persistent aggregation, so it calls:
>
> linkid = get_new_link("aggr1", CLASS_AGGR);
> configuration persistence functions to persist this link.
>
> step 2. "dladm delete-aggr -t aggr1" to delete it temporarily:
>
> Now the problems comes because the consumer needs to have the knowledge
> whether to call delete_linkid() or not.
>
I don't understand why dladm would need to call delete_linkid if you're
temporarily deleting a link. I think if you're temporarily deleting a
link, you'd want to keep the link ID to name mapping around so that the
link ID is not reused. Even if that wasn't a concern (say if guaranteed
that we would never reuse link IDs even after the mapping was
deleted), I still don't see why we couldn't bring the link down without
deleting the mapping.
If knowing the state of the link was important, I would say we should
track that using the configuration structures. We could have a field
indicating if a link is active. When the system reboots, the link
becomes active and the field would be changed to true.
> Therefore, I'd suggest get_new_linkid() and delete_linkid() to have a
> flag to indicate to create or delete linkids for which one (or both,
> active and persistent links).
>
I could do that, but I think it would be better to track that
information elsewhere.
>> We can add walkers if necessary.
>>
> The walkers should be iterate the linkids. Again, it can take the
> persistent/active flag as an argument.
>
Sure, I'll add a walker.
> The difference between the current proposal and the original ones is
> that we never need to inform management module other link attributes
> other than link id and link names for non-persistent links.
>
Right. And if the management modules need to cache any information
about non-persistent links, then it can cache whatever information it
needs as it sees fit.
>> All changes made with this API are temporary unless persisted. We
>> could implement these functions as wrappers around the door calls
>> you've already implemented, we'd just need to add one for the rename
>> function.
>>
>> Any configuration information that is temporary can be stored or
>> manipulated in any way the caller desires, except that the caller
>> cannot change the link ID to name mapping except through the above
>> API. If the caller desires to persist the information across reboots,
>> then the caller must use the following API:
>>
>> Persistent link configuration API
>>
>> get_new_conf()
>>
>> Returns a link configuration structure. Does not affect persistent
>> storage.
>>
>> set_conf_field(conf, field_name, value)
>>
>> Set the given field in the given structure to the given value. Does
>> not affect persist storage.
>>
>> get_conf_field(conf, field_name, value, vallen)
>>
>> Return the value for the given field in the given buffer.
>>
>> get_conf(...)
>>
>> Get the configuration structure for the given link. This is a copy of
>> what is in persistent storage. The argument could be either a link ID
>> or a link name. I lean towards a link ID.
>>
>> commit_conf(conf)
>>
>> Commit the given configuration structure to persistent storage.
>>
>> delete_conf(link)
>>
>> Delete the given link's configuration information from persistent storage
>>
>> free_conf(conf)
>>
>> Free the memory associated with a configuration structure returned by
>> either get_conf or get_new_conf. Does not affect persistent storage
>>
>> walk_confs()
>>
> What exactly information it iterates? The linkids? Then it can be
> achieved by the walkers in the linkid management API.
>
It walks the configuration data structure for each persistent link. I
don't think it can be achieved by the walkers in the linkid management
API because at boot time we need some way of loading the persistent link
ID to name mappings before we can use the link ID management API walker.
I.e.
in devfsadmd at boot
walk_confs()
for each conf
get_conf_field(conf, "id", &id);
get_conf_field(conf, "name", &name);
add_map(id, name); /* this function is part of devfsadmd and
not any
of the API's discussed in this mail */
>> Return a configuration struct for each persistent link. I expect that
>> devfsadmd would use this on boot to determine what link ID to name
>> mappings have been persisted.
>>
> In theory, as we are separating linkid management and configuration
> persistence APIs, the devfsadmd doesn't need to be informed of all the
> persistent link attributes at all, and the libdladm can simply do the
> persistence work by itself, and not through devfsadmd. But I can see two
> benefit that devfsadmd stores persistent link configuration information:
>
> a. those configuration are looked up from both libdladm and the kernel,
> so have the configuration in memory will helps the performance.
>
> b. the devfdadmd will need to be used to store and query the persistent
> configuration anyway, as requested from the door-upcall from the kernel,
> so it might be easy if the all persistence/query requests to be
> processed in the same place.
>
I agree with everything you say here, but, there's one problem. How do
you initialize devfsadmd's internal structures at boot? That's what
this walker is for.
>> Judging by the work you've already done in devfsadmd, we might already
>> have the implementation or something that can be adapted to the
>> implementation for a few of these functions (such as
>> [get|set]_conf_field).
>>
>> A few examples of usage:
>>
>> To configure a temporary link:
>>
>> id = get_new_linkid(name, class)
>> /* configure new link here */
>>
>> To persist a new link:
>>
>> id = get_new_linkid(name, class);
>> conf = get_new_conf();
>> set_conf_field(conf, "class", class);
>> set_conf_field(conf, "name", name);
>> set_conf_field(conf, "id", id);
>> commit_conf(conf);
>> free_conf(conf);
>>
> I think it is better make "id" to be the identifier of a persistent link
> configuration entry, and unique from other conf_field, so that when you
> do the commit_conf(), you know whether it is a new link or it is to
> update an existing link.
>
I'm not sure that is necessary because the ID is in the conf structure,
and I could extract it internally.
>> Note that until commit_conf is called, the new link will not survive
>> across reboots.
>>
>> To rename a temporary link:
>>
>> rename("net0", "net1");
>>
>> To rename a persistent link:
>>
>> get_conf("net5", &conf);
>> /* NOTE: If get_conf were to take an ID, then replace the above line
>> with:
>>
>> get_conf(name2id("net5"), &conf);
>>
>> Note that get_conf("net6") AFTER calling rename should have the exact
>> same effect as what is shown in the example.
>> */
>> rename("net5", "net6");
>> set_conf_field(conf, "name", "net6");
>> commit_conf(conf);
>> free_conf(conf);
>>
>> Note that the rename operation is not persisted until the commit_conf
>> call returns.
>>
>> To delete a link:
>>
>> delete_linkid("net6");
>> delete_conf("net6"); /* If the link has been persisted */
>>
>> I see this as a potential problem. Both steps have to be done if the
>> link has been persisted.
>
> If the consumer want to delete the completely, it needs to call both,
> otherwise I would consider it as a bug, but not a problem of our API.
>
OK
>> If the API user only does the delete_linkid, then the link ID to name
>> mapping is removed, but it still exists in persistent storage. For
>> example:
>>
>> delete_linkid("net6");
>> id = get_new_linkid("net10", class); /* Could return net6's link ID! */
>>
>> If the user persisted this new link ID, we'd have a link ID collision
>> on the next reboot.
>>
>> This also implies that there is no way to safely and temporarily
>> delete the link ID to name mapping for a persistent link. Since this
>> is an API, I think we could document this limitation.
>>
> I don't quite understand why.
>
If a link always has the same ID, even across reboots, then we will have
a collision in this case. At the next reboot, net6 is restored (because
it was persistent and was never removed form persistent storage) and
net10 comes up (because it was persisted) and both have the same link ID.
>> If the API user just calls delete_conf without delete_linkid, that
>> situation is less harmful. On reboot, since there is no persistent
>> configuration for net6, net6's link ID to name mapping is not restored
>> and net6's ID is now available.
>>
>> One thing I do not address here is locking. We should have a mutex
>> lock around the persistent configuration calls. I am not sure we need
>> special locking for the link ID management calls, I think the locking
>> you already have in place in devfsadmd would be sufficient.
>
> Yes. That is right.
>
OK.
> Thanks
> - Cathy
>> What do you think?
>>
>> Dan
>>
>>
>> _________________________________
>> clearview-discuss mailing list
>> clearview-discuss at opensolaris.org
>
