On Mon, Sep 12, 2016 at 6:28 PM, Alexei Starovoitov <alexei.starovoi...@gmail.com> wrote: > On Mon, Sep 12, 2016 at 05:03:25PM -0700, Tom Herbert wrote: >> On Mon, Sep 12, 2016 at 4:46 PM, Eric Dumazet <eric.duma...@gmail.com> wrote: >> > On Mon, 2016-09-12 at 16:07 -0700, Alexei Starovoitov wrote: >> > >> >> yep. there are various ways to shoot yourself in the foot with xdp. >> >> The simplest program that drops all the packets will make the box >> >> unpingable. >> > >> > Well, my comment was about XDP_TX only, not about XDP_DROP or driving a >> > scooter on 101 highway ;) >> > >> > This XDP_TX thing was one of the XDP marketing stuff, but there is >> > absolutely no documentation on it, warning users about possible >> > limitations/outcomes. >> > >> > BTW, I am not sure mlx4 implementation even works, vs BQL : >> > >> > mlx4_en_xmit_frame() does not call netdev_tx_sent_queue(), >> > but tx completion will call netdev_tx_completed_queue() -> crash >> > >> > Do we have one test to validate that a XDP_TX implementation is actually >> > correct ? >> > >> Obviously not for e1000 :-(. We really need some real test and >> performance results and analysis on the interaction between the stack >> data path and XDP data path. > > no. we don't need it for e1k and we cannot really do it. > <broken record mode on> this patch is for debugging of xdp programs only. > You can say this "only for a debugging" a thousand times and that still won't justify putting bad code into the kernel. Material issues have been raised with these patches, I have proposed a fix for one core issue, and we have requested a lot more testing. So, please, if you really want to move these patches forward start addressing the concerns being raised by reviewers.
Tom >> The fact that these changes are being >> passed of as something only needed for KCM is irrelevant, e1000 is a >> well deployed a NIC and there's no restriction that I see that would >> prevent any users from enabling this feature on real devices. > > e1k is not even manufactured any more. Probably the only place > where it can be found is computer history museum. > e1000e fairs slightly better, but it's a different nic and this > patch is not about it. >
