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

[Ben Pfaff]

That's a reasonable enough point of view.  Mine was slightly different,
but I can buy yours.  I'm going to understand the code either way, so I
applied your patch to master.

On Thu, Jan 10, 2013 at 05:57:45PM +0000, Zoltan Kiss wrote:
> 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.
> >
>