Re: RE packet: fix reserve calculation - net/packet/af_packet.c

[James Sakalaukus]

On Fri, Sep 7, 2018 at 1:34 PM, David Miller  wrote:
> From: Willem de Bruijn 
> Date: Fri, 7 Sep 2018 11:50:02 -0400
>
>> If you are not seeing these problems with other protocols, I must be
>> misreading that code.
>
> Right, the ethernet header is only guaranteed to be 2 byte aligned on
> transmit.
>
> Nothing ever gave larger alignment guarantees.
>
> This is the _only_ reasonable situation.
>
> If we made the ethernet header to be 4 byte aligned, that means the
> ipv4 addresses in the ipv4 header are not 4 byte aligned so we would
> do unaligned memory accesses when building the headers which are
> either expensive or trap depending upon the architecture.

Understood.

I thought SOCK_RAW made no assumptions with respect to the network
layer headers.  Previously, the buffer was allocated and then data was
copied into it without giving thought to alignment.  Sounds like I got
lucky with the alignment and I should have expected 2-bytes from day
1.

I've been using this device with only SOCK_RAW on a real time network,
and I have not even tried to use SOCK_STREAM or SOCK_DGRAM.
So, I just tried to use TCP on the device, and the buffer alignment is
in fact 2-bytes inside my transmit function, and my DMA core does not
work.  So...I guess I need to bring my hardware into the 21st century
or do a copy in the driver transmit function.

Thanks for your time

Re: RE packet: fix reserve calculation - net/packet/af_packet.c

[James Sakalaukus]

On Fri, Sep 7, 2018 at 11:50 AM, Willem de Bruijn  wrote:
> Hi James,
>
> Thanks for the report. In the future please always include
> netdev@vger.kernel.org in technical discussions.
>
> On Fri, Sep 7, 2018 at 1:00 AM James Sakalaukus  wrote:
>>
>> Hello Willem and David,
>>
>> I have an unpolished Ethernet driver for a PCIe FPGA subsystem, and
>> the following commit has the side effect of moving the data alignment
>> for SOCK_RAW packets.
>>
>>
>> commit b84bbaf7a6c8cca24f8acf25a2c8e46913a947ba
>> net/packet/af_packet.c
>>
>> These changes to packet_snd() moves the data and tail pointers
>> backwards by net_device->hard_header_len, which is nominally ETH_HLEN.
>> The .ndo_start_xmit driver function now gets a struct sk_buff with
>> data alignment on a 2-byte boundary.  My DMA core is not happy about
>> it.
>>
>>
>> commit 9aad13b087ab0a588cd68259de618f100053360e
>>
>> This commit changed the previous fix from a skb_push to skb_reserve.
>> The functionality from my end did not change though.  .ndo_start_xmit
>> still gets a struct sk_buff with 2 byte alignment.
>>
>>
>> This may be causing problems for other network drivers with DMA
>> alignment requirements, but maybe its just me.
>
> This is the crux of the question.
>
>
> Did the PACKET_TX_RING variant work with your device?
>
> A quick scan seems to indicate that it is common to allocate a linear
> buffer and then reserve hard_header_len aligned up to 16B
> (HH_MOD_LEN). The actual alignment of both link layer and network
> header then depends on the alignment with which kmalloc returned. It
> is probably safe to assume that the buddy allocator returns a multiple
> of 4B at least for allocations of this size. Then the network layer
> header is 4B aligned. But for Ethernet, the skb_push in eth_header()
> would make the link layer header 2B aligned.
>
> If you are not seeing these problems with other protocols, I must be
> misreading that code.
>
> I will take a closer look.
>

I actually have not tested the device with any protocols other than
SOCK_RAW.  This device is on a real time network that does not use
standard network layer protocols.
PACKET_TX_RING is something I haven't had time to delve into.

It may be the case that other protocols always hand over a 2-byte
aligned buffer.  All I know for sure is that I was getting a 4-byte
aligned buffer before the update.


>> I've only been working
>> on the driver for about 6 months, so its not released, though I would
>> like to put it out there for others.  I updated my distro kernel this
>> week, and then proceeded to beat my head against the wall trying to
>> figure out why my driver mysteriously stopped working.
>>
>>
>> Thanks for your time,
>>
>> James Sakalaukus
>> ja...@sakalaukus.com