[ovs-dev] [PATCH 3/3] bond: Fix comment on bond_entry::tag

[Zoltan Kiss]

Hi,

The question is whether my change in the comment is reasonable or not. I 
modified that comment because it confused me first: it suggested that 
the hash tag holds the information that links the hash entry to a slave, 
which is wrong, as the pointer to the slave does that. Based on the hash 
entry's tag you can't lookup the slave (only indirectly, as the tag 
identifies the entry, and therefore the associated slave), the two tags 
are unrelated. But you can find the facet. Therefore saying it creates 
"entry<->facet association" sounds more reasonable to me.

Zoli

On 09/01/13 20:40, Ben Pfaff wrote:
> On Mon, Jan 07, 2013 at 10:39:56PM +0000, Zoltan Kiss wrote:
>> On 07/01/13 19:33, Ben Pfaff wrote:
>>> On Sat, Jan 05, 2013 at 09:42:16PM +0000, Zoltan Kiss wrote:
>>>> The hash entry tag connects to facet(s), not slaves.
>>>>
>>>>   struct bond_entry {
>>>>       struct bond_slave *slave;   /* Assigned slave, NULL if unassigned. */
>>>>       uint64_t tx_bytes;          /* Count of bytes recently transmitted. */
>>>> -    tag_type tag;               /* Tag for entry<->slave association. */
>>>> +    tag_type tag;               /* Tag for entry<->facet association. */
>>>>       struct list list_node;      /* In bond_slave's 'entries' list. */
>>>
>>> I think that the comment is actually correct.  The tag changes
>>> whenever a bond_entry moves from one bond_slave to another.  It goes
>>> without saying that this is also tied to a facet, since that's only
>>> actual use for tags.
>>>
>>> Some other comments in bond.c are clearly wrong.  I'll send out a fix.
>>>
>> My understanding is that 3 kind of objects have tags: facets, slaves
>> and hash entries. Slave and hash tags linking them to facet(s) tags,
>> but although hash tags changing when rebalance happens and new slave
>> chosen, they have nothing to do about the slave's tag. My original
>> assumption was that the tag link the hash to the slave, and the
>> first fix I've sent reflected this.
>> But your commit message here:
>>
>> http://openvswitch.org/cgi-bin/gitweb.cgi?p=openvswitch;a=commitdiff;h=865f22b3b3cb953c48ed30dd21f16ea3dd53f04c
>>
>> "According to the hash value for a flow, to make it easy to
>> invalidate all of the flows that hash into the same bucket."
>>
>> ... suggest me that although the hash tag changes at the moment only
>> if rebalance happens, that might be not true in the future. Or do I
>> misunderstand something in the concept?
>
> Somehow, I'm not sure how, I'm just not following the question here.
> Maybe you can rephrase it, or step back somehow.
>