Re: [Bonding-devel] [PATCH] [bonding 2.4] Add balance-xor-ip bonding mode

Re: [Bonding-devel] [PATCH] [bonding 2.4] Add balance-xor-ip bonding mode

[Amir Noam]

On Wednesday 07 January 2004 10:58 pm, Per Hedeland wrote:
>         struct bonding *bond = bond_dev->priv; 
>         struct ethhdr *data = (struct ethhdr *)skb->data; 
>         struct slave *slave, *start_at; 
> -       int slave_no; 
> +       int slave_no = 0; 
>         int i; 
> +       __u32 u; 
>  
>         read_lock(&bond->lock);
>

Please use u32 instead of __u32.


> +static int bond_xmit_xor_mac(struct sk_buff *skb, struct net_device *bond_dev) 
> +{ 
> +       return bond_xmit_xor(skb, bond_dev, 0); 
> +} 
> + 
> +static int bond_xmit_xor_ip(struct sk_buff *skb, struct net_device *bond_dev) 
> +{ 
> +       return bond_xmit_xor(skb, bond_dev, 1); 
> +} 
> +

hmm...

I don't like this. The reason we give different tx function pointers
to dev->hard_start_xmit in different bonding mode is to make the tx
path as fast as possible. Otherwise we might as well use a single tx
function that chooses its exact operation based on the bonding mode.

It might be better to have some code duplication if it results in
faster tx, but I'm not sure what's the optimal solution in this case.

-- 
Amir

Re: [Bonding-devel] [PATCH] [bonding 2.4] Add balance-xor-ip bonding mode

[Per Hedeland]

Amir Noam <amir.noam@intel.com> wrote:

>Please use u32 instead of __u32.

OK.

>hmm...
>
>I don't like this. The reason we give different tx function pointers
>to dev->hard_start_xmit in different bonding mode is to make the tx
>path as fast as possible. Otherwise we might as well use a single tx
>function that chooses its exact operation based on the bonding mode.
>
>It might be better to have some code duplication if it results in
>faster tx, but I'm not sure what's the optimal solution in this case.

Well, I don't really have an opinion since I don't have a good idea
about the cost of a function call relative to "everything else" that is
happening here. I don't see a way to do "limited" duplication without
using function calls though, but I'm quite happy to make it two entirely
separate functions for MAC vs IP. Please advise.

--Per Hedeland
per@hedeland.org

Re: [Bonding-devel] [PATCH] [bonding 2.4] Add balance-xor-ip bonding mode

[Amir Noam]

On Thursday 08 January 2004 06:43 pm, Per Hedeland wrote:
> Amir Noam <amir.noam@intel.com> wrote:
> >I don't like this. The reason we give different tx function
> > pointers to dev->hard_start_xmit in different bonding mode is to
> > make the tx path as fast as possible. Otherwise we might as well
> > use a single tx function that chooses its exact operation based
> > on the bonding mode.
> >
> >It might be better to have some code duplication if it results in
> >faster tx, but I'm not sure what's the optimal solution in this
> > case.
>
> Well, I don't really have an opinion since I don't have a good idea
> about the cost of a function call relative to "everything else"
> that is happening here. I don't see a way to do "limited"
> duplication without using function calls though, but I'm quite
> happy to make it two entirely separate functions for MAC vs IP.
> Please advise.

A possible way to have "limited" duplication would be to have two 
seperate xmit functions, that call an inline function for the shared 
code. This might be good enough, performance-wise, while avoiding 
some code duplication.

But, like I've said, I'm not sure wout the best solution is. I'd like 
to hear what Jay (and others) thinks about it.

-- 
Amir

Re: [Bonding-devel] [PATCH] [bonding 2.4] Add balance-xor-ip bonding mode

[Per Hedeland]

Amir Noam <amir.noam@intel.com> wrote:
>
>A possible way to have "limited" duplication would be to have two 
>seperate xmit functions, that call an inline function for the shared 
>code. This might be good enough, performance-wise, while avoiding 
>some code duplication.

Yes, I actually thought of that (after the previous post:-) - it would
at least avoid some *source* code duplication. And the reasonable thing
to break out into such a function would be the logic to find the slave
to xmit on given slave_no (perhaps also the actual xmit), I guess.

However when looking at this I realized that the xor mode has a pretty
serious bug/flaw in the fault-tolerance department: The modulus is done
with the total number of slaves, and slaves that are down are only
skipped in that find-slave logic. I.e. if you (e.g.) have a bond with 3
links where link 1 is down, link 2 will get 2/3 of the traffic and link
3 get 1/3 - not good I think.

Since fixing this would likely change the interface to the above
function, I guess it would be nice to do it at the same time as (or
before) the balance-xor-ip addition. Assuming I'm not the only one that
thinks it needs fixing, that is.:-)

The superficially obvious fix is to do the modulus with only the number
of slaves that are up, but I'm not sure about the best actual
implementation. That number isn't currently maintained, and traversing
the list of slaves just to find it for every packet doesn't make sense
of course. It seems straightforward to add another field to the bonding
struct for it, and maintain that in the monitoring functions, though.

But in addition to that, the actual slave selection logic would have to
always check the link state of each slave until it has found the
slave_no'th one that is up (though one could optimize for the case of
all slaves being up) - whereas currently it only starts checking at the
slave_no'th one, and if all slaves are up there is only one check. I'm
not sure if this is acceptable?

The only alternative I see is to also maintain a list of the slaves that
are up, but then we're perhaps getting too much complexity for these
"simple" modes. Any comments, other alternatives?

>But, like I've said, I'm not sure wout the best solution is. I'd like 
>to hear what Jay (and others) thinks about it.

Well, Jay said he would be out of email access for three weeks -
starting tomorrow and promising he would look over updated patches
first, but maybe he didn't have the time - or, given that you
objected:-), he (correctly) thought that a second update wouldn't arrive
in time.

I'm in no particular hurry to get this into the "official" source
though, since I'll be using an older kernel that I've already applied
the (original) patch to for a while anyway - it's just that it gets old
pretty quickly to keep reworking the patches against moving (and not
very easily accessible) targets...

--Per Hedeland
per@hedeland.org