On 17-04-22 07:24 PM, Jakub Kicinski wrote:
> On Thu, 20 Apr 2017 18:50:21 -0700, Jeff Kirsher wrote:
>> +static int ixgbe_xdp_queues(struct ixgbe_adapter *adapter)
>> +{
>> + if (nr_cpu_ids > MAX_XDP_QUEUES)
>> + return 0;
>> +
>> + return adapter->xdp_prog ? nr_cpu_ids : 0;
>> +}
>
> Nit: AFAICT ixgbe_xdp_setup() will guarantee xdp_prog is not set if
> there are too many CPU ids.
Sure being a bit paranoid I guess.
>
>> @@ -6120,10 +6193,21 @@ static int ixgbe_setup_all_tx_resources(struct
>> ixgbe_adapter *adapter)
>> e_err(probe, "Allocation for Tx Queue %u failed\n", i);
>> goto err_setup_tx;
>> }
>> + for (j = 0; j < adapter->num_xdp_queues; j++) {
>> + err = ixgbe_setup_tx_resources(adapter->xdp_ring[j]);
>> + if (!err)
>> + continue;
>> +
>> + e_err(probe, "Allocation for Tx Queue %u failed\n", j);
>> + goto err_setup_tx;
>> + }
>> +
>>
>
> Nit: extra line here
OK well I guess we can fix this if we need a respin anyways.
>
>> @@ -9557,7 +9739,21 @@ static int ixgbe_xdp_setup(struct net_device *dev,
>> struct bpf_prog *prog)
>> return -EINVAL;
>> }
>>
>> + if (nr_cpu_ids > MAX_XDP_QUEUES)
>> + return -ENOMEM;
>> +
>> old_prog = xchg(&adapter->xdp_prog, prog);
>> +
>> + /* If transitioning XDP modes reconfigure rings */
>> + if (!!prog != !!old_prog) {
>> + int err = ixgbe_setup_tc(dev, netdev_get_num_tc(dev));
>> +
>> + if (err) {
>> + rcu_assign_pointer(adapter->xdp_prog, old_prog);
>> + return -EINVAL;
>> + }
>> + }
>> +
>> for (i = 0; i < adapter->num_rx_queues; i++)
>> xchg(&adapter->rx_ring[i]->xdp_prog, adapter->xdp_prog);
>>
>
> In case of disabling XDP I assume ixgbe_setup_tc() will free the rings
> before the xdp_prog on the rings is swapped to NULL. Is there anything
> preventing TX in that time window? I think usual ordering would be to
> install the prog after reconfig but uninstall before.
>
Well in the ixgbe_setup_tc() case we set the rx_ring->xdp_prog in
ixgbe_setup_rx_resorources(), while the dma engine is disabled, so the for
loop is just doing another set on the rx_ring assigning it to the program
already set previously.
Its not really buggy its just extra useless work so I'll change it to this,
if (!!prog != !!old_prog) {
...
} else {
for ( ... )
swap xdp prog
}
Nice spot, thanks for reviewing. And I missed a build error so I'll roll these
fixes in a resend.
Thanks,
John
