On Tue, Sep 29, 2020 at 10:52:48AM -0400, Ion Badulescu wrote:
Hello,
I ran into some packet reordering using a plain vanilla 5.4.49 kernel and the Amazon AWS ena driver. The external symptom was that every now and again, one or more larger packets would be delivered to the UDP socket after some smaller packets, even though the smaller packets were sent after the larger packets. They were also delivered late to a packet socket sniffer, which initially made me suspect an RSS bug in the ena hardware. Eventually, I modified the ena driver to stamp each packet (by overwriting its ethernet source mac field) with the id of the RSS queue that delivered it, and with the value of a per-RSS-queue counter that incremented for each packet received in that queue. That hack showed RSS functioning properly, and also showed packets received earlier (with a smaller counter value) being delivered after packets with a larger counter value. It established that the reordering was in fact happening inside the kernel network core.
The breakthrough came from realizing that the ena driver defaults its rx_copybreak to 256, which matched perfectly the boundary between the delayed large packets and the smaller packets being delivered first. After changing ena's rx_copybreak to zero, the reordering issue disappeared.
After a lot of hair pulling, I think I figured out what the issue is -- and it's confined to the 5.4 stable tree. Commit 323ebb61e32b4 (present in 5.4) introduced queueing for GRO_NORMAL packets received via napi_gro_frags() -> napi_frags_finish(). Commit 6570bc79c0df (NOT present in 5.4) extended the same GRO_NORMAL queueing to packets received via napi_gro_receive() -> napi_skb_finish(). Without 6570bc79c0df, packets received via napi_gro_receive() can get delivered ahead of the earlier, queued up packets received via napi_gro_frags(). And this is precisely what happens in the ena driver with packets smaller than rx_copybreak vs packets larger than rx_copybreak.
Interestingly, the 5.4 stable tree does contain a backport of the upstream c80794323e commit, which purports to fix issues introduced by 323ebb61e32b4 and 6570bc79c0df. But 6570bc79c0df itself is missing...
I don't yet have a 5.4-stable patch, since I wanted to first raise the issue publicly and confirm I'm not missing something obvious. The patch would probably involve massaging 6570bc79c0df quite a bit, to avoid colliding with the already-present c80794323e fix.
You might want to also cc: the networking mailing list for this type of stuff :)
thanks,
greg k-h