On Wed, Sep 19, 2018 at 3:58 AM Jakub Kicinski
<jakub.kicin...@netronome.com> wrote:
>
> On Tue, 18 Sep 2018 10:22:11 -0700, Y Song wrote:
> > > +/* The umem is stored both in the _rx struct and the _tx struct as we do
> > > + * not know if the device has more tx queues than rx, or the opposite.
> > > + * This might also change during run time.
> > > + */
> > > +static void xdp_reg_umem_at_qid(struct net_device *dev, struct xdp_umem
> > > *umem,
> > > + u16 queue_id)
> > > +{
> > > + if (queue_id < dev->real_num_rx_queues)
> > > + dev->_rx[queue_id].umem = umem;
> > > + if (queue_id < dev->real_num_tx_queues)
> > > + dev->_tx[queue_id].umem = umem;
> > > +}
> > > +
> > > +static struct xdp_umem *xdp_get_umem_from_qid(struct net_device *dev,
> > > + u16 queue_id)
> > > +{
> > > + if (queue_id < dev->real_num_rx_queues)
> > > + return dev->_rx[queue_id].umem;
> > > + if (queue_id < dev->real_num_tx_queues)
> > > + return dev->_tx[queue_id].umem;
> > > +
> > > + return NULL;
> > > +}
> > > +
> > > +static void xdp_clear_umem_at_qid(struct net_device *dev, u16 queue_id)
> > > +{
> > > + /* Zero out the entry independent on how many queues are
> > > configured
> > > + * at this point in time, as it might be used in the future.
> > > + */
> > > + if (queue_id < dev->num_rx_queues)
> > > + dev->_rx[queue_id].umem = NULL;
> > > + if (queue_id < dev->num_tx_queues)
> > > + dev->_tx[queue_id].umem = NULL;
> > > +}
> > > +
> >
> > I am sure whether the following scenario can happen or not.
> > Could you clarify?
> > 1. suppose initially we have num_rx_queues = num_tx_queues = 10
> > xdp_reg_umem_at_qid() set umem1 to queue_id = 8
> > 2. num_tx_queues is changed to 5
> > 3. xdp_clear_umem_at_qid() is called for queue_id = 8,
> > and dev->_rx[8].umum = 0.
> > 4. xdp_reg_umem_at_qid() is called gain to set for queue_id = 8
> > dev->_rx[8].umem = umem2
> > 5. num_tx_queues is changed to 10
> > Now dev->_rx[8].umem != dev->_tx[8].umem, is this possible and is it
> > a problem?
>
> Plus IIRC the check of qid vs real_num_[rt]x_queues in xsk_bind() is
> not under rtnl_lock so it doesn't count for much. Why not do all the
> checks against num_[rt]x_queues here, instead of real_..?
You are correct, two separate rtnl_lock regions is broken. Will spin a
v2 tomorrow when I am back in the office.
Thanks Jakub for catching this. I really appreciate you reviewing my code.
/Magnus
