* [PATCH net-next] xsk: Return -EINVAL instead of -EBUSY after xsk_get_pool_from_qid() fails
@ 2021-06-02 3:10 Wang Hai
2021-06-02 8:29 ` Magnus Karlsson
0 siblings, 1 reply; 5+ messages in thread
From: Wang Hai @ 2021-06-02 3:10 UTC (permalink / raw)
To: bjorn, magnus.karlsson, jonathan.lemon, davem, kuba, ast, daniel,
hawk, john.fastabend
Cc: netdev, bpf, linux-kernel
xsk_get_pool_from_qid() fails not because the device's queues are busy,
but because the queue_id exceeds the current number of queues.
So when it fails, it is better to return -EINVAL instead of -EBUSY.
Signed-off-by: Wang Hai <wanghai38@huawei.com>
---
net/xdp/xsk_buff_pool.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/xdp/xsk_buff_pool.c b/net/xdp/xsk_buff_pool.c
index 8de01aaac4a0..30ece117117a 100644
--- a/net/xdp/xsk_buff_pool.c
+++ b/net/xdp/xsk_buff_pool.c
@@ -135,7 +135,7 @@ int xp_assign_dev(struct xsk_buff_pool *pool,
return -EINVAL;
if (xsk_get_pool_from_qid(netdev, queue_id))
- return -EBUSY;
+ return -EINVAL;
pool->netdev = netdev;
pool->queue_id = queue_id;
--
2.17.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] xsk: Return -EINVAL instead of -EBUSY after xsk_get_pool_from_qid() fails
2021-06-02 3:10 [PATCH net-next] xsk: Return -EINVAL instead of -EBUSY after xsk_get_pool_from_qid() fails Wang Hai
@ 2021-06-02 8:29 ` Magnus Karlsson
2021-06-02 8:38 ` Toke Høiland-Jørgensen
2021-06-02 9:27 ` wanghai (M)
0 siblings, 2 replies; 5+ messages in thread
From: Magnus Karlsson @ 2021-06-02 8:29 UTC (permalink / raw)
To: Wang Hai
Cc: Björn Töpel, Karlsson, Magnus, Jonathan Lemon,
David S. Miller, Jakub Kicinski, Alexei Starovoitov,
Daniel Borkmann, Jesper Dangaard Brouer, John Fastabend,
Network Development, bpf, open list
On Wed, Jun 2, 2021 at 6:02 AM Wang Hai <wanghai38@huawei.com> wrote:
>
> xsk_get_pool_from_qid() fails not because the device's queues are busy,
> but because the queue_id exceeds the current number of queues.
> So when it fails, it is better to return -EINVAL instead of -EBUSY.
>
> Signed-off-by: Wang Hai <wanghai38@huawei.com>
> ---
> net/xdp/xsk_buff_pool.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/xdp/xsk_buff_pool.c b/net/xdp/xsk_buff_pool.c
> index 8de01aaac4a0..30ece117117a 100644
> --- a/net/xdp/xsk_buff_pool.c
> +++ b/net/xdp/xsk_buff_pool.c
> @@ -135,7 +135,7 @@ int xp_assign_dev(struct xsk_buff_pool *pool,
> return -EINVAL;
>
> if (xsk_get_pool_from_qid(netdev, queue_id))
> - return -EBUSY;
> + return -EINVAL;
I guess your intent here is to return -EINVAL only when the queue_id
is larger than the number of active queues. But this patch also
changes the return code when the queue id is already in use and in
that case we should continue to return -EBUSY. As this function is
used by a number of drivers, the easiest way to accomplish this is to
introduce a test for queue_id out of bounds before this if-statement
and return -EINVAL there.
> pool->netdev = netdev;
> pool->queue_id = queue_id;
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] xsk: Return -EINVAL instead of -EBUSY after xsk_get_pool_from_qid() fails
2021-06-02 8:29 ` Magnus Karlsson
@ 2021-06-02 8:38 ` Toke Høiland-Jørgensen
2021-06-02 8:53 ` Magnus Karlsson
2021-06-02 9:27 ` wanghai (M)
1 sibling, 1 reply; 5+ messages in thread
From: Toke Høiland-Jørgensen @ 2021-06-02 8:38 UTC (permalink / raw)
To: Magnus Karlsson, Wang Hai
Cc: Björn Töpel, Karlsson, Magnus, Jonathan Lemon,
David S. Miller, Jakub Kicinski, Alexei Starovoitov,
Daniel Borkmann, Jesper Dangaard Brouer, John Fastabend,
Network Development, bpf, open list
Magnus Karlsson <magnus.karlsson@gmail.com> writes:
> On Wed, Jun 2, 2021 at 6:02 AM Wang Hai <wanghai38@huawei.com> wrote:
>>
>> xsk_get_pool_from_qid() fails not because the device's queues are busy,
>> but because the queue_id exceeds the current number of queues.
>> So when it fails, it is better to return -EINVAL instead of -EBUSY.
>>
>> Signed-off-by: Wang Hai <wanghai38@huawei.com>
>> ---
>> net/xdp/xsk_buff_pool.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/xdp/xsk_buff_pool.c b/net/xdp/xsk_buff_pool.c
>> index 8de01aaac4a0..30ece117117a 100644
>> --- a/net/xdp/xsk_buff_pool.c
>> +++ b/net/xdp/xsk_buff_pool.c
>> @@ -135,7 +135,7 @@ int xp_assign_dev(struct xsk_buff_pool *pool,
>> return -EINVAL;
>>
>> if (xsk_get_pool_from_qid(netdev, queue_id))
>> - return -EBUSY;
>> + return -EINVAL;
>
> I guess your intent here is to return -EINVAL only when the queue_id
> is larger than the number of active queues. But this patch also
> changes the return code when the queue id is already in use and in
> that case we should continue to return -EBUSY. As this function is
> used by a number of drivers, the easiest way to accomplish this is to
> introduce a test for queue_id out of bounds before this if-statement
> and return -EINVAL there.
Isn't the return code ABI by now, though?
-Toke
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] xsk: Return -EINVAL instead of -EBUSY after xsk_get_pool_from_qid() fails
2021-06-02 8:38 ` Toke Høiland-Jørgensen
@ 2021-06-02 8:53 ` Magnus Karlsson
0 siblings, 0 replies; 5+ messages in thread
From: Magnus Karlsson @ 2021-06-02 8:53 UTC (permalink / raw)
To: Toke Høiland-Jørgensen
Cc: Wang Hai, Björn Töpel, Karlsson, Magnus,
Jonathan Lemon, David S. Miller, Jakub Kicinski,
Alexei Starovoitov, Daniel Borkmann, Jesper Dangaard Brouer,
John Fastabend, Network Development, bpf, open list
On Wed, Jun 2, 2021 at 10:38 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Magnus Karlsson <magnus.karlsson@gmail.com> writes:
>
> > On Wed, Jun 2, 2021 at 6:02 AM Wang Hai <wanghai38@huawei.com> wrote:
> >>
> >> xsk_get_pool_from_qid() fails not because the device's queues are busy,
> >> but because the queue_id exceeds the current number of queues.
> >> So when it fails, it is better to return -EINVAL instead of -EBUSY.
> >>
> >> Signed-off-by: Wang Hai <wanghai38@huawei.com>
> >> ---
> >> net/xdp/xsk_buff_pool.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/net/xdp/xsk_buff_pool.c b/net/xdp/xsk_buff_pool.c
> >> index 8de01aaac4a0..30ece117117a 100644
> >> --- a/net/xdp/xsk_buff_pool.c
> >> +++ b/net/xdp/xsk_buff_pool.c
> >> @@ -135,7 +135,7 @@ int xp_assign_dev(struct xsk_buff_pool *pool,
> >> return -EINVAL;
> >>
> >> if (xsk_get_pool_from_qid(netdev, queue_id))
> >> - return -EBUSY;
> >> + return -EINVAL;
> >
> > I guess your intent here is to return -EINVAL only when the queue_id
> > is larger than the number of active queues. But this patch also
> > changes the return code when the queue id is already in use and in
> > that case we should continue to return -EBUSY. As this function is
> > used by a number of drivers, the easiest way to accomplish this is to
> > introduce a test for queue_id out of bounds before this if-statement
> > and return -EINVAL there.
>
> Isn't the return code ABI by now, though?
You are probably right and in that case this should not change at all.
It has been returning this for quite a while too as it is nothing new.
But I leave the final decision to other people on the list.
> -Toke
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] xsk: Return -EINVAL instead of -EBUSY after xsk_get_pool_from_qid() fails
2021-06-02 8:29 ` Magnus Karlsson
2021-06-02 8:38 ` Toke Høiland-Jørgensen
@ 2021-06-02 9:27 ` wanghai (M)
1 sibling, 0 replies; 5+ messages in thread
From: wanghai (M) @ 2021-06-02 9:27 UTC (permalink / raw)
To: Magnus Karlsson
Cc: Björn Töpel, Karlsson, Magnus, Jonathan Lemon,
David S. Miller, Jakub Kicinski, Alexei Starovoitov,
Daniel Borkmann, Jesper Dangaard Brouer, John Fastabend,
Network Development, bpf, open list
Sorry, I misunderstood here, this is a faulty patch and no changes are
needed here. Please ignore this patch
在 2021/6/2 16:29, Magnus Karlsson 写道:
> On Wed, Jun 2, 2021 at 6:02 AM Wang Hai <wanghai38@huawei.com> wrote:
>> xsk_get_pool_from_qid() fails not because the device's queues are busy,
>> but because the queue_id exceeds the current number of queues.
>> So when it fails, it is better to return -EINVAL instead of -EBUSY.
>>
>> Signed-off-by: Wang Hai <wanghai38@huawei.com>
>> ---
>> net/xdp/xsk_buff_pool.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/xdp/xsk_buff_pool.c b/net/xdp/xsk_buff_pool.c
>> index 8de01aaac4a0..30ece117117a 100644
>> --- a/net/xdp/xsk_buff_pool.c
>> +++ b/net/xdp/xsk_buff_pool.c
>> @@ -135,7 +135,7 @@ int xp_assign_dev(struct xsk_buff_pool *pool,
>> return -EINVAL;
>>
>> if (xsk_get_pool_from_qid(netdev, queue_id))
>> - return -EBUSY;
>> + return -EINVAL;
> I guess your intent here is to return -EINVAL only when the queue_id
> is larger than the number of active queues.
Yes, I just confirmed that this has been implemented in xp_assign_dev().
int xp_assign_dev()
{
...
err = xsk_reg_pool_at_qid(netdev, pool, queue_id);
if (err)
return err; //return -EINVAL;
...
}
> But this patch also
> changes the return code when the queue id is already in use and in
> that case we should continue to return -EBUSY. As this function is
> used by a number of drivers, the easiest way to accomplish this is to
> introduce a test for queue_id out of bounds before this if-statement
> and return -EINVAL there.
>
>> pool->netdev = netdev;
>> pool->queue_id = queue_id;
>> --
>> 2.17.1
>>
> .
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-06-02 9:27 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-02 3:10 [PATCH net-next] xsk: Return -EINVAL instead of -EBUSY after xsk_get_pool_from_qid() fails Wang Hai
2021-06-02 8:29 ` Magnus Karlsson
2021-06-02 8:38 ` Toke Høiland-Jørgensen
2021-06-02 8:53 ` Magnus Karlsson
2021-06-02 9:27 ` wanghai (M)
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).