Den tors 26 juli 2018 kl 23:44 skrev Jakub Kicinski <jakub.kicin...@netronome.com>: > > Hi! >
Thanks for spending your time on this, Jakub. I'm (temporarily) back for a week, so you can expect faster replies now... > This set tries to make the core take care of error checking for the > drivers. In particular making sure that the AF_XDP UMEM is not installed > on queues which don't exist (or are disabled) and that changing queue > (AKA ethtool channel) count cannot disable queues with active AF_XDF > zero-copy sockets. > > I'm sending as an RFC because I'm not entirely sure what the desired > behaviour is here. Is it Okay to install AF_XDP on queues which don't > exist? I presume not? Your presumption is correct. The idea with the real_num_rx_queues/real_num_tx_queues check in xsk_bind, is to bail out if the queue doesn't exist at bind call. Note that we *didn't* add any code to avoid the bound queue from being removed via set channel (your patch 6). Our idea was that if you remove a queue, the ingress frames would simply stop flowing, and the queue config change checking was "out-of-band". I think I prefer your approach, i.e. not allowing the channels/queues to change if they're bound to an AF_XDP socket. However, your xdp_umem_query used in ethtool only works for ZC enabled drivers, not for the existing non-ZC/copy case. If we'd like to go the route of disabling ethtool_set_channels for an AF_XDP enabled queue this functionality needs to move the query into netdev core, so we have a consistent behavior. > Are the AF_XDP queue_ids referring to TX queues > as well as RX queues in case of the driver? I presume not? We've had a lot of discussions about this internally. Ideally, we'd like to give driver implementors the most freedom, and not enforcing a certain queue scheme for Tx. OTOH it makes it weird for the userland application *not* to have the same id, e.g. if a userland application would like to get stats or configure the AF_XDP bound Tx queue -- which id is it? Should the Tx queue id for an xsk be exposed in sysfs? If the id is *not* the same, would it be OK to change the number of channels and Tx would continue to operate correctly? A related question; An xsk with *only* Tx, should it be constrained by the number of (enabled) Rx queues? I'd be happy to hear some more opinions/thoughts on this... > Should > we try to prevent disabling queues which have non zero-copy sockets > installed as well? :S > Yes, the ZC/non-ZC case must be consistent IMO. See comment above. > Anyway, if any of those patches seem useful and reasonable, please let > me know I will repost as non-RFC. > I definitely think patch 2 and 3 (and probably 1) should go as non-RFC! Thanks for spotting that we're not holding the rtnl lock when checking the # queues (patch 4)! Björn > Jakub Kicinski (6): > net: update real_num_rx_queues even when !CONFIG_SYSFS > xsk: refactor xdp_umem_assign_dev() > xsk: don't allow umem replace at stack level > xsk: don't allow installing UMEM beyond the number of queues > ethtool: rename local variable max -> curr > ethtool: don't allow disabling queues with umem installed > > include/linux/netdevice.h | 16 +++++++-- > net/core/ethtool.c | 19 ++++++---- > net/xdp/xdp_umem.c | 73 ++++++++++++++++++++++++--------------- > 3 files changed, 71 insertions(+), 37 deletions(-) > > -- > 2.17.1 >
