|
On Thu, Nov 21, 2002 at 04:02:03PM +0100, Roberto Nibali wrote:
Hi Ratz,
> >thanks for your comments. I have a new version of the patch attached.
>
> Mucho better! I assume the patch is against ipvs-1.0.7. I wasn't sure
> because you named your patch ipvs-0.1.7-*.
Yes, it is called ipvs-0.1.7-sync_proc.2.patch. Version 2 of the
sync_proc patch for ipvs-0.1.7. If I called it ipvs-0.1.7.patch then I
wouldn't know which patch for ipvs-0.1.7 it was.
I have attached ipvs-0.1.7-sync_proc.3.patch fixing the minor spelling
problem you pointed out. Spelling has never been my strong point.
> >They are just the internal states, I have no idea why the numbers 0 4
> >and 8 were chosen, but it also doesn't seem to be particularly
> >imporatant either. I just moved these few lines from one part of ip_vs.h
> >to another.
>
> The names confused me a bit but I also asked because in your first patch,
> you must have forgotten the chunk where you took them from.
Yes.
> >>>#define IP_VS_SYNC_PORT 8848 /* multicast port */
> >>
> >>I wonder if this should also be made selectable?
> >
> >Good point.
>
> Can be done in the next round, when this patch is in.
Good plan, this patch is big enough as it is.
> >>>+/proc/sys/net/ipv4/vs/sync_msg_max_size sets the maximum size of
> >>>messages
> >>>+sent by the synchronisation daemon in bytes. The default is 1228 and the
> >>>+useful range is 52 through to 6172.
> >>
> >>Why not 52 up to 9k?
> >
> >
> >Because the packet structure only has one byte for the number of sync
> >conn entries in a packet. Thus a maximum of 255 sync conn entries in a
> >packet. The sync conn entries have two vareties, 24 byte
> >and 48 byte. AFIK 24 byte are much more common, thus there is an
> >effective limit on the size of a packet around 6k. Not my design.
>
> I see.
>
> >Actually I think 6172 should be 6148 but it really doesn't make much
> >difference. 4 bytes header + 255 * 24 byte sync conn + 24 bytes of pad
> >in case the last sync conn is 48 bytes. The 24 byte pad isn't actuallty
> >sent over the wire, unless the last entry turns out to be a 48byte
> >entry.
>
> Yep, that's my impression too.
>
> >However, I think this logig is wrong, and we can allow a packet size of
> >up to 4 + 255 * 48 = 12244 bytes to be set. I have updated the patch
> >accordingly. Although one notes that if most (all) the entries are the
> >24 byte variety, you still are stuck at around 6k because of the 255
> >sync conn limit. So it really doesn't make much difference IMHO.
>
> Right. We have to see what Julian and Wensong think about all this.
>
> >By useful, I mean that the code will actually work. Values outside that
> >range are invalid. Perhaps vaild would be a better word to use than
> >useful.
>
> Definitely, thanks.
>
> >>>+syncronisation will occur on once the 3rd packet is recieved, and every
> >>
> >>Holy cow Horms, where you stoned or did I miss recent advances in
> >>English?
> >
> >I've reworded this, hopefully it is clearer now.
>
> Yes :). Sorry for being so picky and a general pain in the butt.
I think the previous version was correct, but very wordy.
> >+struct ip_vs_sync_mesg {
> >+ __u8 nr_conns;
> >+ __u8 reserved;
> >+ __u16 size;
> >+
> >+ /* ip_vs_sync_conn entries start here */
> >+};
>
> Hehe, I know why nr_conns is __u8. struct ip_vs_sync_mesg will be 4 bytes,
> no alignment, no dirty L1 cache lines, simply a nice happy chuck for the
> stack, but there is certainly real truth behind this. :)
Well yes and no. The reserved value is never used (in fact it isn't even
initialised). So the structure could easily have been.
struct ip_vs_sync_mesg {
__u16 nr_conns;
__u16 size;
/* ip_vs_sync_conn entries start here */
};
I suspect the reason is more along the lines of aiming for ~1500 byte
packets, in which case nr_conns can't even reach 255, so 1 byte is
fine. Unfortunately as the header has no version info, changing
it now could result in some strange behaviour if LVS was running
different versions on different hosts. Though, perhaps this
isn't as much of a problem as I percieve.
> >-#define VS_STATE_INPUT 0
> >-#define VS_STATE_OUTPUT 4
> >-#define VS_STATE_INPUT_ONLY 8
> >-
>
> This I couldn't find in the former patch of yours, that's why I was
> confused. I didn't have the code handy:
You confuse me sometimes too :)
> ratz@zar:~/down/ipvs > grep VS_STATE_INPUT_ONLY ipvs-0.1.7-sync_proc.*
> ipvs-0.1.7-sync_proc.2.patch:+#define VS_STATE_INPUT_ONLY 8
> ipvs-0.1.7-sync_proc.2.patch:-#define VS_STATE_INPUT_ONLY 8
> ipvs-0.1.7-sync_proc.patch:+#define VS_STATE_INPUT_ONLY 8
> ratz@zar:~/down/ipvs >
>
> >- len = (cp->flags & IP_VS_CONN_F_SEQ_MASK) ? FULL_CONN_SIZE :
> >- SIMPLE_CONN_SIZE;
> >+ len = (cp->flags & IP_VS_CONN_F_SEQ_MASK) ?
> >IP_VS_SYNC_FULL_CONN_SIZE :
> >+ IP_VS_SYNC_SIMPLE_CONN_SIZE;
> > m = curr_sb->mesg;
> > s = (struct ip_vs_sync_conn *)curr_sb->head;
> >
> >@@ -237,7 +174,8 @@
> > curr_sb->head += len;
> >
> > /* check if there is a space for next one */
> >- if (curr_sb->head+FULL_CONN_SIZE > curr_sb->end) {
> >+ if (curr_sb->head+IP_VS_SYNC_FULL_CONN_SIZE > curr_sb->end ||
> >+ m->nr_conns == 255) {
>
> Something I saw which is not only in your patch: Why don't we define
> something stupid like (does not work of course, but you get the point):
>
> #define NR_CONNS (1 << (8 * sizeof((struct ip_vs_sync_mesg *)->nr_conns)))
> - 1
I'm not sure what you are getting at there. A convenience macro
for accessing blah->nr_conns ?
> >+/proc/sys/net/ipv4/vs/sync_threshold sets the synchronisation threshold \-
> >+the minimum number of packets a connection needs to receive before it will
> >+be synchronised. The default is 3 and the vaild range is from 0 up to
>
> s/vaild/valid/
Good grief!
--
Horms
|
