Paul Mackerras wrote :
> Philippe De Muyter writes:
>
> > > This patch seems a bit strange and/or incomplete. Are we trying to
> > > get 2-byte alignment or 4-byte alignment of the payload? It seems
> >
> > Actually, we try to get a 4n+2 alignment for skb->data, to get the
> > ip-addresses
> > field 4bytes aligned.
> > I think the only thing wrong is the old comment that said :
> > /* Try to get the payload 4-byte aligned */
> > and that I did not change.
>
> Yes, the payload is the part after the protocol field, so the comment
> is still correct.
>
> > > that if the protocol field is uncompressed, we don't do anything to
> > > the alignment, but if it is compressed, we do this:
> > >
> > > > /* protocol is compressed */
> > > > - skb_push(skb, 1)[0] = 0;
> > > > + if ((unsigned long)skb->data & 1)
> > > > + skb_push(skb, 1)[0] = 0;
> > > > + else { /* Ditto, but realign the payload to 4-byte
> > > > boundary */
> > > > + short len = skb->len;
> > > > +
> > > > + skb_put(skb, 3);
> > > > + memmove(skb->data + 3, skb->data, len);
> > > > + skb_pull(skb, 2)[0] = 0;
> > >
> > > I'm puzzled that we are not testing ((unsigned long)skb->data & 2) if
> > > we are really trying to achieve 4-byte alignment. In fact, if the
> > > skb->data that we get from dev_alloc_skb is 4-byte aligned to start
> > > with, this will end up with the payload starting at the original
> > > skb->data + 6, i.e. 2-byte aligned but not 4-byte aligned AFAICS.
> > >
> > > Can we assume that dev_alloc_skb will give us a 4-byte aligned
> > > skb->data? If we can then I suggest we change 3 to 1 in the skb_put
> >
> > Are you not forgetting that the alignment of skb->data is changed (by the
> > existing code ! ) :
> > if (buf[0] != PPP_ALLSTATIONS)
> > skb_reserve(skb, 2 + (buf[0] & 1));
>
> No, I'm not forgetting. If we assume that skb->data starts out 4-byte
> aligned, then the only case in which we will have
>
> ((unsigned long)skb->data & 1) == 0
>
> is if we have protocol field compression (and a compressible protocol
> number, i.e. less than 0x100) but not address/control compression
> (which would be a weird combination, but legal). In that case, with
> your patch we will move the protocol byte to the original skb->data+5
> and have the payload at +6.
Actually, that's probably the case I had, but my fix gets the ip adresses
4byte aligned in my case : I had verified the address of the saddr field,
and I needed to shift the buffer by 3, not 1, to get it 4byte aligned.
>
> If there is any possibility that skb->data is not 4-byte aligned when
No, that's not the problem I had. skb->data was always 4-byte aligned
at allocation time.
> the skb is first allocated, I think that we should do something like
>
> if ((unsigned long)skb->data & 3)
> skb_reserve(skb, 4 - ((unsigned long)skb->data & 3));
>
> immediately after allocating it, and then just memmove the stuff up
> one byte (rather than 3) if it isn't aligned as we want.
>
> Paul.
>
Philippe
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
