Netdev Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH bpf-next] libbpf: remove zc variable as it is not used
@ 2019-08-16 10:26 Magnus Karlsson
  2019-08-16 15:37 ` Yonghong Song
  0 siblings, 1 reply; 6+ messages in thread
From: Magnus Karlsson @ 2019-08-16 10:26 UTC (permalink / raw)
  To: magnus.karlsson, bjorn.topel, ast, daniel, netdev; +Cc: bpf, yhs

The zc is not used in the xsk part of libbpf, so let us remove it. Not
good to have dead code lying around.

Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
Reported-by: Yonghong Song <yhs@fb.com>
---
 tools/lib/bpf/xsk.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
index 680e630..9687da9 100644
--- a/tools/lib/bpf/xsk.c
+++ b/tools/lib/bpf/xsk.c
@@ -65,7 +65,6 @@ struct xsk_socket {
 	int xsks_map_fd;
 	__u32 queue_id;
 	char ifname[IFNAMSIZ];
-	bool zc;
 };
 
 struct xsk_nl_info {
@@ -608,8 +607,6 @@ int xsk_socket__create(struct xsk_socket **xsk_ptr, const char *ifname,
 		goto out_mmap_tx;
 	}
 
-	xsk->zc = opts.flags & XDP_OPTIONS_ZEROCOPY;
-
 	if (!(xsk->config.libbpf_flags & XSK_LIBBPF_FLAGS__INHIBIT_PROG_LOAD)) {
 		err = xsk_setup_xdp_prog(xsk);
 		if (err)
-- 
2.7.4


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH bpf-next] libbpf: remove zc variable as it is not used
  2019-08-16 10:26 [PATCH bpf-next] libbpf: remove zc variable as it is not used Magnus Karlsson
@ 2019-08-16 15:37 ` Yonghong Song
  2019-08-16 22:01   ` Jonathan Lemon
  0 siblings, 1 reply; 6+ messages in thread
From: Yonghong Song @ 2019-08-16 15:37 UTC (permalink / raw)
  To: Magnus Karlsson, bjorn.topel, ast, daniel, netdev; +Cc: bpf



On 8/16/19 3:26 AM, Magnus Karlsson wrote:
> The zc is not used in the xsk part of libbpf, so let us remove it. Not
> good to have dead code lying around.
> 
> Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
> Reported-by: Yonghong Song <yhs@fb.com> > ---
>   tools/lib/bpf/xsk.c | 3 ---
>   1 file changed, 3 deletions(-)
> 
> diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
> index 680e630..9687da9 100644
> --- a/tools/lib/bpf/xsk.c
> +++ b/tools/lib/bpf/xsk.c
> @@ -65,7 +65,6 @@ struct xsk_socket {
>   	int xsks_map_fd;
>   	__u32 queue_id;
>   	char ifname[IFNAMSIZ];
> -	bool zc;
>   };
>   
>   struct xsk_nl_info {
> @@ -608,8 +607,6 @@ int xsk_socket__create(struct xsk_socket **xsk_ptr, const char *ifname,
>   		goto out_mmap_tx;
>   	}
>   
> -	xsk->zc = opts.flags & XDP_OPTIONS_ZEROCOPY;

Since opts.flags usage is removed. Do you think it makes sense to
remove
         optlen = sizeof(opts);
         err = getsockopt(xsk->fd, SOL_XDP, XDP_OPTIONS, &opts, &optlen);
         if (err) {
                 err = -errno;
                 goto out_mmap_tx;
         }
as well since nobody then uses opts?

> -
>   	if (!(xsk->config.libbpf_flags & XSK_LIBBPF_FLAGS__INHIBIT_PROG_LOAD)) {
>   		err = xsk_setup_xdp_prog(xsk);
>   		if (err)
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH bpf-next] libbpf: remove zc variable as it is not used
  2019-08-16 15:37 ` Yonghong Song
@ 2019-08-16 22:01   ` Jonathan Lemon
  2019-08-19  6:35     ` Magnus Karlsson
  0 siblings, 1 reply; 6+ messages in thread
From: Jonathan Lemon @ 2019-08-16 22:01 UTC (permalink / raw)
  To: Yonghong Song; +Cc: Magnus Karlsson, bjorn.topel, ast, daniel, netdev, bpf



On 16 Aug 2019, at 8:37, Yonghong Song wrote:

> On 8/16/19 3:26 AM, Magnus Karlsson wrote:
>> The zc is not used in the xsk part of libbpf, so let us remove it. 
>> Not
>> good to have dead code lying around.
>>
>> Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
>> Reported-by: Yonghong Song <yhs@fb.com> > ---
>>   tools/lib/bpf/xsk.c | 3 ---
>>   1 file changed, 3 deletions(-)
>>
>> diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
>> index 680e630..9687da9 100644
>> --- a/tools/lib/bpf/xsk.c
>> +++ b/tools/lib/bpf/xsk.c
>> @@ -65,7 +65,6 @@ struct xsk_socket {
>>   	int xsks_map_fd;
>>   	__u32 queue_id;
>>   	char ifname[IFNAMSIZ];
>> -	bool zc;
>>   };
>>
>>   struct xsk_nl_info {
>> @@ -608,8 +607,6 @@ int xsk_socket__create(struct xsk_socket 
>> **xsk_ptr, const char *ifname,
>>   		goto out_mmap_tx;
>>   	}
>>
>> -	xsk->zc = opts.flags & XDP_OPTIONS_ZEROCOPY;
>
> Since opts.flags usage is removed. Do you think it makes sense to
> remove
>          optlen = sizeof(opts);
>          err = getsockopt(xsk->fd, SOL_XDP, XDP_OPTIONS, &opts, 
> &optlen);
>          if (err) {
>                  err = -errno;
>                  goto out_mmap_tx;
>          }
> as well since nobody then uses opts?

IIRC, this was added specifically in 
2761ed4b6e192820760d5ba913834b2ba05fd08c
so that userland code could know whether the socket was operating in 
zero-copy
mode or not.
-- 
Jonathan

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH bpf-next] libbpf: remove zc variable as it is not used
  2019-08-16 22:01   ` Jonathan Lemon
@ 2019-08-19  6:35     ` Magnus Karlsson
  2019-08-19  6:47       ` Maxim Mikityanskiy
  0 siblings, 1 reply; 6+ messages in thread
From: Magnus Karlsson @ 2019-08-19  6:35 UTC (permalink / raw)
  To: Jonathan Lemon, Maxim Mikityanskiy
  Cc: Yonghong Song, Magnus Karlsson, Björn Töpel,
	Alexei Starovoitov, Daniel Borkmann, Network Development, bpf

On Sat, Aug 17, 2019 at 12:02 AM Jonathan Lemon
<jonathan.lemon@gmail.com> wrote:
>
>
>
> On 16 Aug 2019, at 8:37, Yonghong Song wrote:
>
> > On 8/16/19 3:26 AM, Magnus Karlsson wrote:
> >> The zc is not used in the xsk part of libbpf, so let us remove it.
> >> Not
> >> good to have dead code lying around.
> >>
> >> Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
> >> Reported-by: Yonghong Song <yhs@fb.com> > ---
> >>   tools/lib/bpf/xsk.c | 3 ---
> >>   1 file changed, 3 deletions(-)
> >>
> >> diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
> >> index 680e630..9687da9 100644
> >> --- a/tools/lib/bpf/xsk.c
> >> +++ b/tools/lib/bpf/xsk.c
> >> @@ -65,7 +65,6 @@ struct xsk_socket {
> >>      int xsks_map_fd;
> >>      __u32 queue_id;
> >>      char ifname[IFNAMSIZ];
> >> -    bool zc;
> >>   };
> >>
> >>   struct xsk_nl_info {
> >> @@ -608,8 +607,6 @@ int xsk_socket__create(struct xsk_socket
> >> **xsk_ptr, const char *ifname,
> >>              goto out_mmap_tx;
> >>      }
> >>
> >> -    xsk->zc = opts.flags & XDP_OPTIONS_ZEROCOPY;
> >
> > Since opts.flags usage is removed. Do you think it makes sense to
> > remove
> >          optlen = sizeof(opts);
> >          err = getsockopt(xsk->fd, SOL_XDP, XDP_OPTIONS, &opts,
> > &optlen);
> >          if (err) {
> >                  err = -errno;
> >                  goto out_mmap_tx;
> >          }
> > as well since nobody then uses opts?
>
> IIRC, this was added specifically in
> 2761ed4b6e192820760d5ba913834b2ba05fd08c
> so that userland code could know whether the socket was operating in
> zero-copy
> mode or not.

Thanks for reminding me Jonathan.

Roping in Maxim here since he wrote the patch. Was this something you
planned on using but the functionality that needed it was removed? The
patch set did go through a number of changes in the libbpf area, if I
remember correctly.

There are two options: either we remove it, or we add an interface in
xsk.h so that people can use it. I vote for the latter since I think
it could be useful. The sample app could use it at least :-).

/Magnus

> --
> Jonathan

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH bpf-next] libbpf: remove zc variable as it is not used
  2019-08-19  6:35     ` Magnus Karlsson
@ 2019-08-19  6:47       ` Maxim Mikityanskiy
  2019-08-19  7:35         ` Magnus Karlsson
  0 siblings, 1 reply; 6+ messages in thread
From: Maxim Mikityanskiy @ 2019-08-19  6:47 UTC (permalink / raw)
  To: Magnus Karlsson, Jonathan Lemon
  Cc: Yonghong Song, Magnus Karlsson, Björn Töpel,
	Alexei Starovoitov, Daniel Borkmann, Network Development, bpf

On 2019-08-19 09:35, Magnus Karlsson wrote:
> On Sat, Aug 17, 2019 at 12:02 AM Jonathan Lemon
> <jonathan.lemon@gmail.com> wrote:
>>
>>
>>
>> On 16 Aug 2019, at 8:37, Yonghong Song wrote:
>>
>>> On 8/16/19 3:26 AM, Magnus Karlsson wrote:
>>>> The zc is not used in the xsk part of libbpf, so let us remove it.
>>>> Not
>>>> good to have dead code lying around.
>>>>
>>>> Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
>>>> Reported-by: Yonghong Song <yhs@fb.com> > ---
>>>>    tools/lib/bpf/xsk.c | 3 ---
>>>>    1 file changed, 3 deletions(-)
>>>>
>>>> diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
>>>> index 680e630..9687da9 100644
>>>> --- a/tools/lib/bpf/xsk.c
>>>> +++ b/tools/lib/bpf/xsk.c
>>>> @@ -65,7 +65,6 @@ struct xsk_socket {
>>>>       int xsks_map_fd;
>>>>       __u32 queue_id;
>>>>       char ifname[IFNAMSIZ];
>>>> -    bool zc;
>>>>    };
>>>>
>>>>    struct xsk_nl_info {
>>>> @@ -608,8 +607,6 @@ int xsk_socket__create(struct xsk_socket
>>>> **xsk_ptr, const char *ifname,
>>>>               goto out_mmap_tx;
>>>>       }
>>>>
>>>> -    xsk->zc = opts.flags & XDP_OPTIONS_ZEROCOPY;
>>>
>>> Since opts.flags usage is removed. Do you think it makes sense to
>>> remove
>>>           optlen = sizeof(opts);
>>>           err = getsockopt(xsk->fd, SOL_XDP, XDP_OPTIONS, &opts,
>>> &optlen);
>>>           if (err) {
>>>                   err = -errno;
>>>                   goto out_mmap_tx;
>>>           }
>>> as well since nobody then uses opts?
>>
>> IIRC, this was added specifically in
>> 2761ed4b6e192820760d5ba913834b2ba05fd08c
>> so that userland code could know whether the socket was operating in
>> zero-copy
>> mode or not.
> 
> Thanks for reminding me Jonathan.
> 
> Roping in Maxim here since he wrote the patch. Was this something you
> planned on using but the functionality that needed it was removed? The
> patch set did go through a number of changes in the libbpf area, if I
> remember correctly.
> 
> There are two options: either we remove it, or we add an interface in
> xsk.h so that people can use it. I vote for the latter since I think
> it could be useful. The sample app could use it at least :-).

+1, let's expose it and make xdpsock read and print it. I added this 
flag to libbpf when I added the ability to query it from the kernel, but 
I didn't pay attention that struct xsk_socket was private to libbpf, and 
xsk->zc couldn't be accessed externally.

> /Magnus
> 
>> --
>> Jonathan


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH bpf-next] libbpf: remove zc variable as it is not used
  2019-08-19  6:47       ` Maxim Mikityanskiy
@ 2019-08-19  7:35         ` Magnus Karlsson
  0 siblings, 0 replies; 6+ messages in thread
From: Magnus Karlsson @ 2019-08-19  7:35 UTC (permalink / raw)
  To: Maxim Mikityanskiy
  Cc: Jonathan Lemon, Yonghong Song, Magnus Karlsson,
	Björn Töpel, Alexei Starovoitov, Daniel Borkmann,
	Network Development, bpf

On Mon, Aug 19, 2019 at 8:47 AM Maxim Mikityanskiy <maximmi@mellanox.com> wrote:
>
> On 2019-08-19 09:35, Magnus Karlsson wrote:
> > On Sat, Aug 17, 2019 at 12:02 AM Jonathan Lemon
> > <jonathan.lemon@gmail.com> wrote:
> >>
> >>
> >>
> >> On 16 Aug 2019, at 8:37, Yonghong Song wrote:
> >>
> >>> On 8/16/19 3:26 AM, Magnus Karlsson wrote:
> >>>> The zc is not used in the xsk part of libbpf, so let us remove it.
> >>>> Not
> >>>> good to have dead code lying around.
> >>>>
> >>>> Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
> >>>> Reported-by: Yonghong Song <yhs@fb.com> > ---
> >>>>    tools/lib/bpf/xsk.c | 3 ---
> >>>>    1 file changed, 3 deletions(-)
> >>>>
> >>>> diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
> >>>> index 680e630..9687da9 100644
> >>>> --- a/tools/lib/bpf/xsk.c
> >>>> +++ b/tools/lib/bpf/xsk.c
> >>>> @@ -65,7 +65,6 @@ struct xsk_socket {
> >>>>       int xsks_map_fd;
> >>>>       __u32 queue_id;
> >>>>       char ifname[IFNAMSIZ];
> >>>> -    bool zc;
> >>>>    };
> >>>>
> >>>>    struct xsk_nl_info {
> >>>> @@ -608,8 +607,6 @@ int xsk_socket__create(struct xsk_socket
> >>>> **xsk_ptr, const char *ifname,
> >>>>               goto out_mmap_tx;
> >>>>       }
> >>>>
> >>>> -    xsk->zc = opts.flags & XDP_OPTIONS_ZEROCOPY;
> >>>
> >>> Since opts.flags usage is removed. Do you think it makes sense to
> >>> remove
> >>>           optlen = sizeof(opts);
> >>>           err = getsockopt(xsk->fd, SOL_XDP, XDP_OPTIONS, &opts,
> >>> &optlen);
> >>>           if (err) {
> >>>                   err = -errno;
> >>>                   goto out_mmap_tx;
> >>>           }
> >>> as well since nobody then uses opts?
> >>
> >> IIRC, this was added specifically in
> >> 2761ed4b6e192820760d5ba913834b2ba05fd08c
> >> so that userland code could know whether the socket was operating in
> >> zero-copy
> >> mode or not.
> >
> > Thanks for reminding me Jonathan.
> >
> > Roping in Maxim here since he wrote the patch. Was this something you
> > planned on using but the functionality that needed it was removed? The
> > patch set did go through a number of changes in the libbpf area, if I
> > remember correctly.
> >
> > There are two options: either we remove it, or we add an interface in
> > xsk.h so that people can use it. I vote for the latter since I think
> > it could be useful. The sample app could use it at least :-).
>
> +1, let's expose it and make xdpsock read and print it. I added this
> flag to libbpf when I added the ability to query it from the kernel, but
> I didn't pay attention that struct xsk_socket was private to libbpf, and
> xsk->zc couldn't be accessed externally.

Good. I will then add it.

Please discard this patch. I will add this interface in a new patch.

/Magnus

> > /Magnus
> >
> >> --
> >> Jonathan
>

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-16 10:26 [PATCH bpf-next] libbpf: remove zc variable as it is not used Magnus Karlsson
2019-08-16 15:37 ` Yonghong Song
2019-08-16 22:01   ` Jonathan Lemon
2019-08-19  6:35     ` Magnus Karlsson
2019-08-19  6:47       ` Maxim Mikityanskiy
2019-08-19  7:35         ` Magnus Karlsson

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org netdev@archiver.kernel.org
	public-inbox-index netdev


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox