Re: [ovs-dev] [PATCH net-next v5 05/10] net: openvswitch: add emit_sample action
Adrián Moreno writes: > On Thu, Jun 27, 2024 at 09:30:08AM GMT, Aaron Conole wrote: >> Ilya Maximets writes: >> >> > On 6/27/24 12:15, Adrián Moreno wrote: >> >> On Thu, Jun 27, 2024 at 11:31:41AM GMT, Eelco Chaudron wrote: >> >>> >> >>> >> >>> On 27 Jun 2024, at 11:23, Ilya Maximets wrote: >> >>> >> On 6/27/24 11:14, Eelco Chaudron wrote: >> > >> > >> > On 27 Jun 2024, at 10:36, Ilya Maximets wrote: >> > >> >> On 6/27/24 09:52, Adrián Moreno wrote: >> >>> On Thu, Jun 27, 2024 at 09:06:46AM GMT, Eelco Chaudron wrote: >> >> >> On 26 Jun 2024, at 22:34, Adrián Moreno wrote: >> >> > On Wed, Jun 26, 2024 at 04:28:17PM GMT, Eelco Chaudron wrote: >> >> >> >> >> >> On 25 Jun 2024, at 22:51, Adrian Moreno wrote: >> >> >> >>> Add support for a new action: emit_sample. >> >>> >> >>> This action accepts a u32 group id and a variable-length >> >>> cookie and uses >> >>> the psample multicast group to make the packet available for >> >>> observability. >> >>> >> >>> The maximum length of the user-defined cookie is set to >> >>> 16, same as >> >>> tc_cookie, to discourage using cookies that will not be >> >>> offloadable. >> >> >> >> I’ll add the same comment as I had in the user space part, and >> >> that >> >> is that I feel from an OVS perspective this action should be >> >> called >> >> emit_local() instead of emit_sample() to make it Datapath >> >> independent. >> >> Or quoting the earlier comment: >> >> >> >> >> >> “I’ll start the discussion again on the naming. The name >> >> "emit_sample()" >> >> does not seem appropriate. This function's primary role >> >> is to copy the >> >> packet and send it to a local collector, which varies >> >> depending on the >> >> datapath. For the kernel datapath, this collector is >> >> psample, while for >> >> userspace, it will likely be some kind of probe. This >> >> action is distinct >> >> from the sample() action by design; it is a standalone >> >> action that can >> >> be combined with others. >> >> >> >> Furthermore, the action itself does not involve taking a sample; >> >> it >> >> consistently pushes the packet to the local collector. Therefore, >> >> I >> >> suggest renaming "emit_sample()" to "emit_local()". This >> >> same goes for >> >> all the derivative ATTR naming.” >> >> >> > >> > This is a blurry semantic area. >> > IMO, "sample" is the act of extracting (potentially a piece of) >> > someting, in this case, a packet. It is common to only >> > take some packets >> > as samples, so this action usually comes with some kind of >> > "rate", but >> > even if the rate is 1, it's still sampling in this context. >> > >> > OTOH, OVS kernel design tries to be super-modular and define small >> > combinable actions, so the rate or probability generation >> > is done with >> > another action which is (IMHO unfortunately) named "sample". >> > >> > With that interpretation of the term it would actually >> > make more sense >> > to rename "sample" to something like "random" (of course I'm not >> > suggestion we do it). "sample" without any nested action >> > that actually >> > sends the packet somewhere is not sampling, it's just >> > doing something or >> > not based on a probability. Where as "emit_sample" is >> > sampling even if >> > it's not nested inside a "sample". >> >> You're assuming we are extracting a packet for sampling, >> but this function >> can be used for various other purposes. For instance, it >> could handle the >> packet outside of the OVS pipeline through an eBPF program >> (so we are not >> taking a sample, but continue packet processing outside of the OVS >> pipeline). Calling it emit_sampling() in such cases could be very >> confusing. >> >> >> >> We can't change the implementation of the action once it is >> >> part of uAPI. >> >> We have to document where users can find these packets and we >> >> can't just >> >> change the destination later. >> > >> > I'm not suggesting we change the uAPI implementation, but we >> > could use the >> > emit_xxx() action with an eBPF probe on the action to perform >> > other tasks. >> > This is just an example. >> >> Yeah, but as Adrian said below, you could do that with any action and >> this doesn't change the semantics of the actio
Re: [ovs-dev] [PATCH net-next v5 05/10] net: openvswitch: add emit_sample action
On Thu, Jun 27, 2024 at 09:30:08AM GMT, Aaron Conole wrote: > Ilya Maximets writes: > > > On 6/27/24 12:15, Adrián Moreno wrote: > >> On Thu, Jun 27, 2024 at 11:31:41AM GMT, Eelco Chaudron wrote: > >>> > >>> > >>> On 27 Jun 2024, at 11:23, Ilya Maximets wrote: > >>> > On 6/27/24 11:14, Eelco Chaudron wrote: > > > > > > On 27 Jun 2024, at 10:36, Ilya Maximets wrote: > > > >> On 6/27/24 09:52, Adrián Moreno wrote: > >>> On Thu, Jun 27, 2024 at 09:06:46AM GMT, Eelco Chaudron wrote: > > > On 26 Jun 2024, at 22:34, Adrián Moreno wrote: > > > On Wed, Jun 26, 2024 at 04:28:17PM GMT, Eelco Chaudron wrote: > >> > >> > >> On 25 Jun 2024, at 22:51, Adrian Moreno wrote: > >> > >>> Add support for a new action: emit_sample. > >>> > >>> This action accepts a u32 group id and a variable-length cookie > >>> and uses > >>> the psample multicast group to make the packet available for > >>> observability. > >>> > >>> The maximum length of the user-defined cookie is set to 16, same > >>> as > >>> tc_cookie, to discourage using cookies that will not be > >>> offloadable. > >> > >> I’ll add the same comment as I had in the user space part, and that > >> is that I feel from an OVS perspective this action should be called > >> emit_local() instead of emit_sample() to make it Datapath > >> independent. > >> Or quoting the earlier comment: > >> > >> > >> “I’ll start the discussion again on the naming. The name > >> "emit_sample()" > >> does not seem appropriate. This function's primary role is to copy > >> the > >> packet and send it to a local collector, which varies depending on > >> the > >> datapath. For the kernel datapath, this collector is psample, > >> while for > >> userspace, it will likely be some kind of probe. This action is > >> distinct > >> from the sample() action by design; it is a standalone action that > >> can > >> be combined with others. > >> > >> Furthermore, the action itself does not involve taking a sample; it > >> consistently pushes the packet to the local collector. Therefore, I > >> suggest renaming "emit_sample()" to "emit_local()". This same goes > >> for > >> all the derivative ATTR naming.” > >> > > > > This is a blurry semantic area. > > IMO, "sample" is the act of extracting (potentially a piece of) > > someting, in this case, a packet. It is common to only take some > > packets > > as samples, so this action usually comes with some kind of "rate", > > but > > even if the rate is 1, it's still sampling in this context. > > > > OTOH, OVS kernel design tries to be super-modular and define small > > combinable actions, so the rate or probability generation is done > > with > > another action which is (IMHO unfortunately) named "sample". > > > > With that interpretation of the term it would actually make more > > sense > > to rename "sample" to something like "random" (of course I'm not > > suggestion we do it). "sample" without any nested action that > > actually > > sends the packet somewhere is not sampling, it's just doing > > something or > > not based on a probability. Where as "emit_sample" is sampling even > > if > > it's not nested inside a "sample". > > You're assuming we are extracting a packet for sampling, but this > function > can be used for various other purposes. For instance, it could > handle the > packet outside of the OVS pipeline through an eBPF program (so we > are not > taking a sample, but continue packet processing outside of the OVS > pipeline). Calling it emit_sampling() in such cases could be very > confusing. > >> > >> We can't change the implementation of the action once it is part of > >> uAPI. > >> We have to document where users can find these packets and we can't > >> just > >> change the destination later. > > > > I'm not suggesting we change the uAPI implementation, but we could use > > the > > emit_xxx() action with an eBPF probe on the action to perform other > > tasks. > > This is just an example. > > Yeah, but as Adrian said below, you could do that with any action and > this doesn't change the semantics of the action itself. > >>> > >>> Well this was just an example, what if we have some other need for getting > >>> a packet to userspace through emit_local() other than sampling? The >
Re: [ovs-dev] [PATCH net-next v5 05/10] net: openvswitch: add emit_sample action
On 27 Jun 2024, at 12:52, Ilya Maximets wrote: > On 6/27/24 12:15, Adrián Moreno wrote: >> On Thu, Jun 27, 2024 at 11:31:41AM GMT, Eelco Chaudron wrote: >>> >>> >>> On 27 Jun 2024, at 11:23, Ilya Maximets wrote: >>> On 6/27/24 11:14, Eelco Chaudron wrote: > > > On 27 Jun 2024, at 10:36, Ilya Maximets wrote: > >> On 6/27/24 09:52, Adrián Moreno wrote: >>> On Thu, Jun 27, 2024 at 09:06:46AM GMT, Eelco Chaudron wrote: On 26 Jun 2024, at 22:34, Adrián Moreno wrote: > On Wed, Jun 26, 2024 at 04:28:17PM GMT, Eelco Chaudron wrote: >> >> >> On 25 Jun 2024, at 22:51, Adrian Moreno wrote: >> >>> Add support for a new action: emit_sample. >>> >>> This action accepts a u32 group id and a variable-length cookie and >>> uses >>> the psample multicast group to make the packet available for >>> observability. >>> >>> The maximum length of the user-defined cookie is set to 16, same as >>> tc_cookie, to discourage using cookies that will not be offloadable. >> >> I’ll add the same comment as I had in the user space part, and that >> is that I feel from an OVS perspective this action should be called >> emit_local() instead of emit_sample() to make it Datapath >> independent. >> Or quoting the earlier comment: >> >> >> “I’ll start the discussion again on the naming. The name >> "emit_sample()" >> does not seem appropriate. This function's primary role is to copy >> the >> packet and send it to a local collector, which varies depending on >> the >> datapath. For the kernel datapath, this collector is psample, while >> for >> userspace, it will likely be some kind of probe. This action is >> distinct >> from the sample() action by design; it is a standalone action that >> can >> be combined with others. >> >> Furthermore, the action itself does not involve taking a sample; it >> consistently pushes the packet to the local collector. Therefore, I >> suggest renaming "emit_sample()" to "emit_local()". This same goes >> for >> all the derivative ATTR naming.” >> > > This is a blurry semantic area. > IMO, "sample" is the act of extracting (potentially a piece of) > someting, in this case, a packet. It is common to only take some > packets > as samples, so this action usually comes with some kind of "rate", but > even if the rate is 1, it's still sampling in this context. > > OTOH, OVS kernel design tries to be super-modular and define small > combinable actions, so the rate or probability generation is done with > another action which is (IMHO unfortunately) named "sample". > > With that interpretation of the term it would actually make more sense > to rename "sample" to something like "random" (of course I'm not > suggestion we do it). "sample" without any nested action that actually > sends the packet somewhere is not sampling, it's just doing something > or > not based on a probability. Where as "emit_sample" is sampling even if > it's not nested inside a "sample". You're assuming we are extracting a packet for sampling, but this function can be used for various other purposes. For instance, it could handle the packet outside of the OVS pipeline through an eBPF program (so we are not taking a sample, but continue packet processing outside of the OVS pipeline). Calling it emit_sampling() in such cases could be very confusing. >> >> We can't change the implementation of the action once it is part of uAPI. >> We have to document where users can find these packets and we can't just >> change the destination later. > > I'm not suggesting we change the uAPI implementation, but we could use the > emit_xxx() action with an eBPF probe on the action to perform other tasks. > This is just an example. Yeah, but as Adrian said below, you could do that with any action and this doesn't change the semantics of the action itself. >>> >>> Well this was just an example, what if we have some other need for getting >>> a packet to userspace through emit_local() other than sampling? The >>> emit_sample() action naming in this case makes no sense. >>> >>> Well, I guess that would be clearly abusing the action. You could say >>> that of anything really. You could hook into skb_consume and continue >>> processing the skb but that doesn't change the intended behavior of the >>> drop action. >>> >>> The intended behavior of the action is
Re: [ovs-dev] [PATCH net-next v5 05/10] net: openvswitch: add emit_sample action
Ilya Maximets writes: > On 6/27/24 12:15, Adrián Moreno wrote: >> On Thu, Jun 27, 2024 at 11:31:41AM GMT, Eelco Chaudron wrote: >>> >>> >>> On 27 Jun 2024, at 11:23, Ilya Maximets wrote: >>> On 6/27/24 11:14, Eelco Chaudron wrote: > > > On 27 Jun 2024, at 10:36, Ilya Maximets wrote: > >> On 6/27/24 09:52, Adrián Moreno wrote: >>> On Thu, Jun 27, 2024 at 09:06:46AM GMT, Eelco Chaudron wrote: On 26 Jun 2024, at 22:34, Adrián Moreno wrote: > On Wed, Jun 26, 2024 at 04:28:17PM GMT, Eelco Chaudron wrote: >> >> >> On 25 Jun 2024, at 22:51, Adrian Moreno wrote: >> >>> Add support for a new action: emit_sample. >>> >>> This action accepts a u32 group id and a variable-length cookie and >>> uses >>> the psample multicast group to make the packet available for >>> observability. >>> >>> The maximum length of the user-defined cookie is set to 16, same as >>> tc_cookie, to discourage using cookies that will not be offloadable. >> >> I’ll add the same comment as I had in the user space part, and that >> is that I feel from an OVS perspective this action should be called >> emit_local() instead of emit_sample() to make it Datapath >> independent. >> Or quoting the earlier comment: >> >> >> “I’ll start the discussion again on the naming. The name >> "emit_sample()" >> does not seem appropriate. This function's primary role is to copy >> the >> packet and send it to a local collector, which varies depending on >> the >> datapath. For the kernel datapath, this collector is psample, while >> for >> userspace, it will likely be some kind of probe. This action is >> distinct >> from the sample() action by design; it is a standalone action that >> can >> be combined with others. >> >> Furthermore, the action itself does not involve taking a sample; it >> consistently pushes the packet to the local collector. Therefore, I >> suggest renaming "emit_sample()" to "emit_local()". This same goes >> for >> all the derivative ATTR naming.” >> > > This is a blurry semantic area. > IMO, "sample" is the act of extracting (potentially a piece of) > someting, in this case, a packet. It is common to only take some > packets > as samples, so this action usually comes with some kind of "rate", but > even if the rate is 1, it's still sampling in this context. > > OTOH, OVS kernel design tries to be super-modular and define small > combinable actions, so the rate or probability generation is done with > another action which is (IMHO unfortunately) named "sample". > > With that interpretation of the term it would actually make more sense > to rename "sample" to something like "random" (of course I'm not > suggestion we do it). "sample" without any nested action that actually > sends the packet somewhere is not sampling, it's just doing something > or > not based on a probability. Where as "emit_sample" is sampling even if > it's not nested inside a "sample". You're assuming we are extracting a packet for sampling, but this function can be used for various other purposes. For instance, it could handle the packet outside of the OVS pipeline through an eBPF program (so we are not taking a sample, but continue packet processing outside of the OVS pipeline). Calling it emit_sampling() in such cases could be very confusing. >> >> We can't change the implementation of the action once it is part of uAPI. >> We have to document where users can find these packets and we can't just >> change the destination later. > > I'm not suggesting we change the uAPI implementation, but we could use the > emit_xxx() action with an eBPF probe on the action to perform other tasks. > This is just an example. Yeah, but as Adrian said below, you could do that with any action and this doesn't change the semantics of the action itself. >>> >>> Well this was just an example, what if we have some other need for getting >>> a packet to userspace through emit_local() other than sampling? The >>> emit_sample() action naming in this case makes no sense. >>> >>> Well, I guess that would be clearly abusing the action. You could say >>> that of anything really. You could hook into skb_consume and continue >>> processing the skb but that doesn't change the intended behavior of the >>> drop action. >>> >>> The intended behavior of the action is sampling, as is the inten
Re: [ovs-dev] [PATCH net-next v5 05/10] net: openvswitch: add emit_sample action
On 6/27/24 12:15, Adrián Moreno wrote: > On Thu, Jun 27, 2024 at 11:31:41AM GMT, Eelco Chaudron wrote: >> >> >> On 27 Jun 2024, at 11:23, Ilya Maximets wrote: >> >>> On 6/27/24 11:14, Eelco Chaudron wrote: On 27 Jun 2024, at 10:36, Ilya Maximets wrote: > On 6/27/24 09:52, Adrián Moreno wrote: >> On Thu, Jun 27, 2024 at 09:06:46AM GMT, Eelco Chaudron wrote: >>> >>> >>> On 26 Jun 2024, at 22:34, Adrián Moreno wrote: >>> On Wed, Jun 26, 2024 at 04:28:17PM GMT, Eelco Chaudron wrote: > > > On 25 Jun 2024, at 22:51, Adrian Moreno wrote: > >> Add support for a new action: emit_sample. >> >> This action accepts a u32 group id and a variable-length cookie and >> uses >> the psample multicast group to make the packet available for >> observability. >> >> The maximum length of the user-defined cookie is set to 16, same as >> tc_cookie, to discourage using cookies that will not be offloadable. > > I’ll add the same comment as I had in the user space part, and that > is that I feel from an OVS perspective this action should be called > emit_local() instead of emit_sample() to make it Datapath independent. > Or quoting the earlier comment: > > > “I’ll start the discussion again on the naming. The name > "emit_sample()" > does not seem appropriate. This function's primary role is to copy the > packet and send it to a local collector, which varies depending on the > datapath. For the kernel datapath, this collector is psample, while > for > userspace, it will likely be some kind of probe. This action is > distinct > from the sample() action by design; it is a standalone action that can > be combined with others. > > Furthermore, the action itself does not involve taking a sample; it > consistently pushes the packet to the local collector. Therefore, I > suggest renaming "emit_sample()" to "emit_local()". This same goes for > all the derivative ATTR naming.” > This is a blurry semantic area. IMO, "sample" is the act of extracting (potentially a piece of) someting, in this case, a packet. It is common to only take some packets as samples, so this action usually comes with some kind of "rate", but even if the rate is 1, it's still sampling in this context. OTOH, OVS kernel design tries to be super-modular and define small combinable actions, so the rate or probability generation is done with another action which is (IMHO unfortunately) named "sample". With that interpretation of the term it would actually make more sense to rename "sample" to something like "random" (of course I'm not suggestion we do it). "sample" without any nested action that actually sends the packet somewhere is not sampling, it's just doing something or not based on a probability. Where as "emit_sample" is sampling even if it's not nested inside a "sample". >>> >>> You're assuming we are extracting a packet for sampling, but this >>> function >>> can be used for various other purposes. For instance, it could handle >>> the >>> packet outside of the OVS pipeline through an eBPF program (so we are >>> not >>> taking a sample, but continue packet processing outside of the OVS >>> pipeline). Calling it emit_sampling() in such cases could be very >>> confusing. > > We can't change the implementation of the action once it is part of uAPI. > We have to document where users can find these packets and we can't just > change the destination later. I'm not suggesting we change the uAPI implementation, but we could use the emit_xxx() action with an eBPF probe on the action to perform other tasks. This is just an example. >>> >>> Yeah, but as Adrian said below, you could do that with any action and >>> this doesn't change the semantics of the action itself. >> >> Well this was just an example, what if we have some other need for getting >> a packet to userspace through emit_local() other than sampling? The >> emit_sample() action naming in this case makes no sense. >> >> Well, I guess that would be clearly abusing the action. You could say >> that of anything really. You could hook into skb_consume and continue >> processing the skb but that doesn't change the intended behavior of the >> drop action. >> >> The intended behavior of the action is sampling, as is the intended >> behavior of "psample". > > The original OVS_ACTION_ATTR_SAMPLE "Probabilitically executes actions", > that is it takes some packets from the whole packet stream and ex
Re: [ovs-dev] [PATCH net-next v5 05/10] net: openvswitch: add emit_sample action
On Thu, Jun 27, 2024 at 11:31:41AM GMT, Eelco Chaudron wrote: > > > On 27 Jun 2024, at 11:23, Ilya Maximets wrote: > > > On 6/27/24 11:14, Eelco Chaudron wrote: > >> > >> > >> On 27 Jun 2024, at 10:36, Ilya Maximets wrote: > >> > >>> On 6/27/24 09:52, Adrián Moreno wrote: > On Thu, Jun 27, 2024 at 09:06:46AM GMT, Eelco Chaudron wrote: > > > > > > On 26 Jun 2024, at 22:34, Adrián Moreno wrote: > > > >> On Wed, Jun 26, 2024 at 04:28:17PM GMT, Eelco Chaudron wrote: > >>> > >>> > >>> On 25 Jun 2024, at 22:51, Adrian Moreno wrote: > >>> > Add support for a new action: emit_sample. > > This action accepts a u32 group id and a variable-length cookie and > uses > the psample multicast group to make the packet available for > observability. > > The maximum length of the user-defined cookie is set to 16, same as > tc_cookie, to discourage using cookies that will not be offloadable. > >>> > >>> I’ll add the same comment as I had in the user space part, and that > >>> is that I feel from an OVS perspective this action should be called > >>> emit_local() instead of emit_sample() to make it Datapath independent. > >>> Or quoting the earlier comment: > >>> > >>> > >>> “I’ll start the discussion again on the naming. The name > >>> "emit_sample()" > >>> does not seem appropriate. This function's primary role is to copy the > >>> packet and send it to a local collector, which varies depending on the > >>> datapath. For the kernel datapath, this collector is psample, while > >>> for > >>> userspace, it will likely be some kind of probe. This action is > >>> distinct > >>> from the sample() action by design; it is a standalone action that can > >>> be combined with others. > >>> > >>> Furthermore, the action itself does not involve taking a sample; it > >>> consistently pushes the packet to the local collector. Therefore, I > >>> suggest renaming "emit_sample()" to "emit_local()". This same goes for > >>> all the derivative ATTR naming.” > >>> > >> > >> This is a blurry semantic area. > >> IMO, "sample" is the act of extracting (potentially a piece of) > >> someting, in this case, a packet. It is common to only take some > >> packets > >> as samples, so this action usually comes with some kind of "rate", but > >> even if the rate is 1, it's still sampling in this context. > >> > >> OTOH, OVS kernel design tries to be super-modular and define small > >> combinable actions, so the rate or probability generation is done with > >> another action which is (IMHO unfortunately) named "sample". > >> > >> With that interpretation of the term it would actually make more sense > >> to rename "sample" to something like "random" (of course I'm not > >> suggestion we do it). "sample" without any nested action that actually > >> sends the packet somewhere is not sampling, it's just doing something > >> or > >> not based on a probability. Where as "emit_sample" is sampling even if > >> it's not nested inside a "sample". > > > > You're assuming we are extracting a packet for sampling, but this > > function > > can be used for various other purposes. For instance, it could handle > > the > > packet outside of the OVS pipeline through an eBPF program (so we are > > not > > taking a sample, but continue packet processing outside of the OVS > > pipeline). Calling it emit_sampling() in such cases could be very > > confusing. > >>> > >>> We can't change the implementation of the action once it is part of uAPI. > >>> We have to document where users can find these packets and we can't just > >>> change the destination later. > >> > >> I'm not suggesting we change the uAPI implementation, but we could use the > >> emit_xxx() action with an eBPF probe on the action to perform other tasks. > >> This is just an example. > > > > Yeah, but as Adrian said below, you could do that with any action and > > this doesn't change the semantics of the action itself. > > Well this was just an example, what if we have some other need for getting > a packet to userspace through emit_local() other than sampling? The > emit_sample() action naming in this case makes no sense. > > Well, I guess that would be clearly abusing the action. You could say > that of anything really. You could hook into skb_consume and continue > processing the skb but that doesn't change the intended behavior of the > drop action. > > The intended behavior of the action is sampling, as is the intended > behavior of "psample". > >>> > >>> The original OVS_ACTION_ATTR_SAMPLE "Probabilitically executes actions", > >>> that is it takes some packets from the whole packet stream and executes > >>> actions of them. Without tying this
Re: [ovs-dev] [PATCH net-next v5 05/10] net: openvswitch: add emit_sample action
On 27 Jun 2024, at 11:23, Ilya Maximets wrote: > On 6/27/24 11:14, Eelco Chaudron wrote: >> >> >> On 27 Jun 2024, at 10:36, Ilya Maximets wrote: >> >>> On 6/27/24 09:52, Adrián Moreno wrote: On Thu, Jun 27, 2024 at 09:06:46AM GMT, Eelco Chaudron wrote: > > > On 26 Jun 2024, at 22:34, Adrián Moreno wrote: > >> On Wed, Jun 26, 2024 at 04:28:17PM GMT, Eelco Chaudron wrote: >>> >>> >>> On 25 Jun 2024, at 22:51, Adrian Moreno wrote: >>> Add support for a new action: emit_sample. This action accepts a u32 group id and a variable-length cookie and uses the psample multicast group to make the packet available for observability. The maximum length of the user-defined cookie is set to 16, same as tc_cookie, to discourage using cookies that will not be offloadable. >>> >>> I’ll add the same comment as I had in the user space part, and that >>> is that I feel from an OVS perspective this action should be called >>> emit_local() instead of emit_sample() to make it Datapath independent. >>> Or quoting the earlier comment: >>> >>> >>> “I’ll start the discussion again on the naming. The name "emit_sample()" >>> does not seem appropriate. This function's primary role is to copy the >>> packet and send it to a local collector, which varies depending on the >>> datapath. For the kernel datapath, this collector is psample, while for >>> userspace, it will likely be some kind of probe. This action is distinct >>> from the sample() action by design; it is a standalone action that can >>> be combined with others. >>> >>> Furthermore, the action itself does not involve taking a sample; it >>> consistently pushes the packet to the local collector. Therefore, I >>> suggest renaming "emit_sample()" to "emit_local()". This same goes for >>> all the derivative ATTR naming.” >>> >> >> This is a blurry semantic area. >> IMO, "sample" is the act of extracting (potentially a piece of) >> someting, in this case, a packet. It is common to only take some packets >> as samples, so this action usually comes with some kind of "rate", but >> even if the rate is 1, it's still sampling in this context. >> >> OTOH, OVS kernel design tries to be super-modular and define small >> combinable actions, so the rate or probability generation is done with >> another action which is (IMHO unfortunately) named "sample". >> >> With that interpretation of the term it would actually make more sense >> to rename "sample" to something like "random" (of course I'm not >> suggestion we do it). "sample" without any nested action that actually >> sends the packet somewhere is not sampling, it's just doing something or >> not based on a probability. Where as "emit_sample" is sampling even if >> it's not nested inside a "sample". > > You're assuming we are extracting a packet for sampling, but this function > can be used for various other purposes. For instance, it could handle the > packet outside of the OVS pipeline through an eBPF program (so we are not > taking a sample, but continue packet processing outside of the OVS > pipeline). Calling it emit_sampling() in such cases could be very > confusing. >>> >>> We can't change the implementation of the action once it is part of uAPI. >>> We have to document where users can find these packets and we can't just >>> change the destination later. >> >> I'm not suggesting we change the uAPI implementation, but we could use the >> emit_xxx() action with an eBPF probe on the action to perform other tasks. >> This is just an example. > > Yeah, but as Adrian said below, you could do that with any action and > this doesn't change the semantics of the action itself. Well this was just an example, what if we have some other need for getting a packet to userspace through emit_local() other than sampling? The emit_sample() action naming in this case makes no sense. Well, I guess that would be clearly abusing the action. You could say that of anything really. You could hook into skb_consume and continue processing the skb but that doesn't change the intended behavior of the drop action. The intended behavior of the action is sampling, as is the intended behavior of "psample". >>> >>> The original OVS_ACTION_ATTR_SAMPLE "Probabilitically executes actions", >>> that is it takes some packets from the whole packet stream and executes >>> actions of them. Without tying this to observability purposes the name >>> makes sense as the first definition of the word is "to take a representative >>> part or a single item from a larger whole or group". >>> >>> Now, our new action doesn't have this particular semantic in a way that >>> it doesn't take a part of a whole packet stream but rather using the >>> part al
Re: [ovs-dev] [PATCH net-next v5 05/10] net: openvswitch: add emit_sample action
On 6/27/24 11:14, Eelco Chaudron wrote: > > > On 27 Jun 2024, at 10:36, Ilya Maximets wrote: > >> On 6/27/24 09:52, Adrián Moreno wrote: >>> On Thu, Jun 27, 2024 at 09:06:46AM GMT, Eelco Chaudron wrote: On 26 Jun 2024, at 22:34, Adrián Moreno wrote: > On Wed, Jun 26, 2024 at 04:28:17PM GMT, Eelco Chaudron wrote: >> >> >> On 25 Jun 2024, at 22:51, Adrian Moreno wrote: >> >>> Add support for a new action: emit_sample. >>> >>> This action accepts a u32 group id and a variable-length cookie and uses >>> the psample multicast group to make the packet available for >>> observability. >>> >>> The maximum length of the user-defined cookie is set to 16, same as >>> tc_cookie, to discourage using cookies that will not be offloadable. >> >> I’ll add the same comment as I had in the user space part, and that >> is that I feel from an OVS perspective this action should be called >> emit_local() instead of emit_sample() to make it Datapath independent. >> Or quoting the earlier comment: >> >> >> “I’ll start the discussion again on the naming. The name "emit_sample()" >> does not seem appropriate. This function's primary role is to copy the >> packet and send it to a local collector, which varies depending on the >> datapath. For the kernel datapath, this collector is psample, while for >> userspace, it will likely be some kind of probe. This action is distinct >> from the sample() action by design; it is a standalone action that can >> be combined with others. >> >> Furthermore, the action itself does not involve taking a sample; it >> consistently pushes the packet to the local collector. Therefore, I >> suggest renaming "emit_sample()" to "emit_local()". This same goes for >> all the derivative ATTR naming.” >> > > This is a blurry semantic area. > IMO, "sample" is the act of extracting (potentially a piece of) > someting, in this case, a packet. It is common to only take some packets > as samples, so this action usually comes with some kind of "rate", but > even if the rate is 1, it's still sampling in this context. > > OTOH, OVS kernel design tries to be super-modular and define small > combinable actions, so the rate or probability generation is done with > another action which is (IMHO unfortunately) named "sample". > > With that interpretation of the term it would actually make more sense > to rename "sample" to something like "random" (of course I'm not > suggestion we do it). "sample" without any nested action that actually > sends the packet somewhere is not sampling, it's just doing something or > not based on a probability. Where as "emit_sample" is sampling even if > it's not nested inside a "sample". You're assuming we are extracting a packet for sampling, but this function can be used for various other purposes. For instance, it could handle the packet outside of the OVS pipeline through an eBPF program (so we are not taking a sample, but continue packet processing outside of the OVS pipeline). Calling it emit_sampling() in such cases could be very confusing. >> >> We can't change the implementation of the action once it is part of uAPI. >> We have to document where users can find these packets and we can't just >> change the destination later. > > I'm not suggesting we change the uAPI implementation, but we could use the > emit_xxx() action with an eBPF probe on the action to perform other tasks. > This is just an example. Yeah, but as Adrian said below, you could do that with any action and this doesn't change the semantics of the action itself. > >>> Well, I guess that would be clearly abusing the action. You could say >>> that of anything really. You could hook into skb_consume and continue >>> processing the skb but that doesn't change the intended behavior of the >>> drop action. >>> >>> The intended behavior of the action is sampling, as is the intended >>> behavior of "psample". >> >> The original OVS_ACTION_ATTR_SAMPLE "Probabilitically executes actions", >> that is it takes some packets from the whole packet stream and executes >> actions of them. Without tying this to observability purposes the name >> makes sense as the first definition of the word is "to take a representative >> part or a single item from a larger whole or group". >> >> Now, our new action doesn't have this particular semantic in a way that >> it doesn't take a part of a whole packet stream but rather using the >> part already taken. However, it is directly tied to the parent >> OVS_ACTION_ATTR_SAMPLE action, since it reports probability of that parent >> action. If there is no parent, then probability is assumed to be 100%, >> but that's just a corner case. The name of a psample module has the >> same semantics in its name, it doesn't sample on it's own, but it is
Re: [ovs-dev] [PATCH net-next v5 05/10] net: openvswitch: add emit_sample action
On 27 Jun 2024, at 10:36, Ilya Maximets wrote: > On 6/27/24 09:52, Adrián Moreno wrote: >> On Thu, Jun 27, 2024 at 09:06:46AM GMT, Eelco Chaudron wrote: >>> >>> >>> On 26 Jun 2024, at 22:34, Adrián Moreno wrote: >>> On Wed, Jun 26, 2024 at 04:28:17PM GMT, Eelco Chaudron wrote: > > > On 25 Jun 2024, at 22:51, Adrian Moreno wrote: > >> Add support for a new action: emit_sample. >> >> This action accepts a u32 group id and a variable-length cookie and uses >> the psample multicast group to make the packet available for >> observability. >> >> The maximum length of the user-defined cookie is set to 16, same as >> tc_cookie, to discourage using cookies that will not be offloadable. > > I’ll add the same comment as I had in the user space part, and that > is that I feel from an OVS perspective this action should be called > emit_local() instead of emit_sample() to make it Datapath independent. > Or quoting the earlier comment: > > > “I’ll start the discussion again on the naming. The name "emit_sample()" > does not seem appropriate. This function's primary role is to copy the > packet and send it to a local collector, which varies depending on the > datapath. For the kernel datapath, this collector is psample, while for > userspace, it will likely be some kind of probe. This action is distinct > from the sample() action by design; it is a standalone action that can > be combined with others. > > Furthermore, the action itself does not involve taking a sample; it > consistently pushes the packet to the local collector. Therefore, I > suggest renaming "emit_sample()" to "emit_local()". This same goes for > all the derivative ATTR naming.” > This is a blurry semantic area. IMO, "sample" is the act of extracting (potentially a piece of) someting, in this case, a packet. It is common to only take some packets as samples, so this action usually comes with some kind of "rate", but even if the rate is 1, it's still sampling in this context. OTOH, OVS kernel design tries to be super-modular and define small combinable actions, so the rate or probability generation is done with another action which is (IMHO unfortunately) named "sample". With that interpretation of the term it would actually make more sense to rename "sample" to something like "random" (of course I'm not suggestion we do it). "sample" without any nested action that actually sends the packet somewhere is not sampling, it's just doing something or not based on a probability. Where as "emit_sample" is sampling even if it's not nested inside a "sample". >>> >>> You're assuming we are extracting a packet for sampling, but this function >>> can be used for various other purposes. For instance, it could handle the >>> packet outside of the OVS pipeline through an eBPF program (so we are not >>> taking a sample, but continue packet processing outside of the OVS >>> pipeline). Calling it emit_sampling() in such cases could be very >>> confusing. > > We can't change the implementation of the action once it is part of uAPI. > We have to document where users can find these packets and we can't just > change the destination later. I'm not suggesting we change the uAPI implementation, but we could use the emit_xxx() action with an eBPF probe on the action to perform other tasks. This is just an example. >> Well, I guess that would be clearly abusing the action. You could say >> that of anything really. You could hook into skb_consume and continue >> processing the skb but that doesn't change the intended behavior of the >> drop action. >> >> The intended behavior of the action is sampling, as is the intended >> behavior of "psample". > > The original OVS_ACTION_ATTR_SAMPLE "Probabilitically executes actions", > that is it takes some packets from the whole packet stream and executes > actions of them. Without tying this to observability purposes the name > makes sense as the first definition of the word is "to take a representative > part or a single item from a larger whole or group". > > Now, our new action doesn't have this particular semantic in a way that > it doesn't take a part of a whole packet stream but rather using the > part already taken. However, it is directly tied to the parent > OVS_ACTION_ATTR_SAMPLE action, since it reports probability of that parent > action. If there is no parent, then probability is assumed to be 100%, > but that's just a corner case. The name of a psample module has the > same semantics in its name, it doesn't sample on it's own, but it is > assuming that sampling was performed as it relays the rate of it. > > And since we're directly tied here with both OVS_ACTION_ATTR_SAMPLE and > the psample module, the emit_sample() name makes sense to me. This is the part I don't like. emit_sample() should be treated a
Re: [ovs-dev] [PATCH net-next v5 05/10] net: openvswitch: add emit_sample action
On 6/27/24 09:52, Adrián Moreno wrote: > On Thu, Jun 27, 2024 at 09:06:46AM GMT, Eelco Chaudron wrote: >> >> >> On 26 Jun 2024, at 22:34, Adrián Moreno wrote: >> >>> On Wed, Jun 26, 2024 at 04:28:17PM GMT, Eelco Chaudron wrote: On 25 Jun 2024, at 22:51, Adrian Moreno wrote: > Add support for a new action: emit_sample. > > This action accepts a u32 group id and a variable-length cookie and uses > the psample multicast group to make the packet available for > observability. > > The maximum length of the user-defined cookie is set to 16, same as > tc_cookie, to discourage using cookies that will not be offloadable. I’ll add the same comment as I had in the user space part, and that is that I feel from an OVS perspective this action should be called emit_local() instead of emit_sample() to make it Datapath independent. Or quoting the earlier comment: “I’ll start the discussion again on the naming. The name "emit_sample()" does not seem appropriate. This function's primary role is to copy the packet and send it to a local collector, which varies depending on the datapath. For the kernel datapath, this collector is psample, while for userspace, it will likely be some kind of probe. This action is distinct from the sample() action by design; it is a standalone action that can be combined with others. Furthermore, the action itself does not involve taking a sample; it consistently pushes the packet to the local collector. Therefore, I suggest renaming "emit_sample()" to "emit_local()". This same goes for all the derivative ATTR naming.” >>> >>> This is a blurry semantic area. >>> IMO, "sample" is the act of extracting (potentially a piece of) >>> someting, in this case, a packet. It is common to only take some packets >>> as samples, so this action usually comes with some kind of "rate", but >>> even if the rate is 1, it's still sampling in this context. >>> >>> OTOH, OVS kernel design tries to be super-modular and define small >>> combinable actions, so the rate or probability generation is done with >>> another action which is (IMHO unfortunately) named "sample". >>> >>> With that interpretation of the term it would actually make more sense >>> to rename "sample" to something like "random" (of course I'm not >>> suggestion we do it). "sample" without any nested action that actually >>> sends the packet somewhere is not sampling, it's just doing something or >>> not based on a probability. Where as "emit_sample" is sampling even if >>> it's not nested inside a "sample". >> >> You're assuming we are extracting a packet for sampling, but this function >> can be used for various other purposes. For instance, it could handle the >> packet outside of the OVS pipeline through an eBPF program (so we are not >> taking a sample, but continue packet processing outside of the OVS >> pipeline). Calling it emit_sampling() in such cases could be very >> confusing. We can't change the implementation of the action once it is part of uAPI. We have to document where users can find these packets and we can't just change the destination later. >> > > Well, I guess that would be clearly abusing the action. You could say > that of anything really. You could hook into skb_consume and continue > processing the skb but that doesn't change the intended behavior of the > drop action. > > The intended behavior of the action is sampling, as is the intended > behavior of "psample". The original OVS_ACTION_ATTR_SAMPLE "Probabilitically executes actions", that is it takes some packets from the whole packet stream and executes actions of them. Without tying this to observability purposes the name makes sense as the first definition of the word is "to take a representative part or a single item from a larger whole or group". Now, our new action doesn't have this particular semantic in a way that it doesn't take a part of a whole packet stream but rather using the part already taken. However, it is directly tied to the parent OVS_ACTION_ATTR_SAMPLE action, since it reports probability of that parent action. If there is no parent, then probability is assumed to be 100%, but that's just a corner case. The name of a psample module has the same semantics in its name, it doesn't sample on it's own, but it is assuming that sampling was performed as it relays the rate of it. And since we're directly tied here with both OVS_ACTION_ATTR_SAMPLE and the psample module, the emit_sample() name makes sense to me. > >>> Having said that, I don't have a super strong favor for "emit_sample". I'm >>> OK with "emit_local" or "emit_packet" or even just "emit". The 'local' or 'packet' variants are not descriptive enough on what we're trying to achieve and do not explain why the probability is attached to the action, i.e. do not explain the link between this action and the OVS_ACTION_ATTR_SAMPLE. emit_Psa
Re: [ovs-dev] [PATCH net-next v5 05/10] net: openvswitch: add emit_sample action
On Thu, Jun 27, 2024 at 09:06:46AM GMT, Eelco Chaudron wrote: > > > On 26 Jun 2024, at 22:34, Adrián Moreno wrote: > > > On Wed, Jun 26, 2024 at 04:28:17PM GMT, Eelco Chaudron wrote: > >> > >> > >> On 25 Jun 2024, at 22:51, Adrian Moreno wrote: > >> > >>> Add support for a new action: emit_sample. > >>> > >>> This action accepts a u32 group id and a variable-length cookie and uses > >>> the psample multicast group to make the packet available for > >>> observability. > >>> > >>> The maximum length of the user-defined cookie is set to 16, same as > >>> tc_cookie, to discourage using cookies that will not be offloadable. > >> > >> I’ll add the same comment as I had in the user space part, and that > >> is that I feel from an OVS perspective this action should be called > >> emit_local() instead of emit_sample() to make it Datapath independent. > >> Or quoting the earlier comment: > >> > >> > >> “I’ll start the discussion again on the naming. The name "emit_sample()" > >> does not seem appropriate. This function's primary role is to copy the > >> packet and send it to a local collector, which varies depending on the > >> datapath. For the kernel datapath, this collector is psample, while for > >> userspace, it will likely be some kind of probe. This action is distinct > >> from the sample() action by design; it is a standalone action that can > >> be combined with others. > >> > >> Furthermore, the action itself does not involve taking a sample; it > >> consistently pushes the packet to the local collector. Therefore, I > >> suggest renaming "emit_sample()" to "emit_local()". This same goes for > >> all the derivative ATTR naming.” > >> > > > > This is a blurry semantic area. > > IMO, "sample" is the act of extracting (potentially a piece of) > > someting, in this case, a packet. It is common to only take some packets > > as samples, so this action usually comes with some kind of "rate", but > > even if the rate is 1, it's still sampling in this context. > > > > OTOH, OVS kernel design tries to be super-modular and define small > > combinable actions, so the rate or probability generation is done with > > another action which is (IMHO unfortunately) named "sample". > > > > With that interpretation of the term it would actually make more sense > > to rename "sample" to something like "random" (of course I'm not > > suggestion we do it). "sample" without any nested action that actually > > sends the packet somewhere is not sampling, it's just doing something or > > not based on a probability. Where as "emit_sample" is sampling even if > > it's not nested inside a "sample". > > You're assuming we are extracting a packet for sampling, but this function > can be used for various other purposes. For instance, it could handle the > packet outside of the OVS pipeline through an eBPF program (so we are not > taking a sample, but continue packet processing outside of the OVS > pipeline). Calling it emit_sampling() in such cases could be very > confusing. > Well, I guess that would be clearly abusing the action. You could say that of anything really. You could hook into skb_consume and continue processing the skb but that doesn't change the intended behavior of the drop action. The intended behavior of the action is sampling, as is the intended behavior of "psample". > > Having said that, I don't have a super strong favor for "emit_sample". I'm > > OK with "emit_local" or "emit_packet" or even just "emit". > > I don't think any term will fully satisfy everyone so I hope we can find > > a reasonable compromise. > > My preference would be emit_local() as we hand it off to some local > datapath entity. > I'm OK removing the controversial term. Let's see what others think. > >>> Signed-off-by: Adrian Moreno > >>> --- > >>> Documentation/netlink/specs/ovs_flow.yaml | 17 + > >>> include/uapi/linux/openvswitch.h | 28 ++ > >>> net/openvswitch/Kconfig | 1 + > >>> net/openvswitch/actions.c | 45 +++ > >>> net/openvswitch/flow_netlink.c| 33 - > >>> 5 files changed, 123 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/Documentation/netlink/specs/ovs_flow.yaml > >>> b/Documentation/netlink/specs/ovs_flow.yaml > >>> index 4fdfc6b5cae9..a7ab5593a24f 100644 > >>> --- a/Documentation/netlink/specs/ovs_flow.yaml > >>> +++ b/Documentation/netlink/specs/ovs_flow.yaml > >>> @@ -727,6 +727,12 @@ attribute-sets: > >>> name: dec-ttl > >>> type: nest > >>> nested-attributes: dec-ttl-attrs > >>> + - > >>> +name: emit-sample > >>> +type: nest > >>> +nested-attributes: emit-sample-attrs > >>> +doc: | > >>> + Sends a packet sample to psample for external observation. > >>>- > >>> name: tunnel-key-attrs > >>> enum-name: ovs-tunnel-key-attr > >>> @@ -938,6 +944,17 @@ attribute-sets: > >>>- > >>> name: gbp > >>> t
Re: [ovs-dev] [PATCH net-next v5 05/10] net: openvswitch: add emit_sample action
On 6/26/24 22:07, Adrián Moreno wrote:
> On Wed, Jun 26, 2024 at 12:51:19PM GMT, Ilya Maximets wrote:
>> On 6/25/24 22:51, Adrian Moreno wrote:
>>> Add support for a new action: emit_sample.
>>>
>>> This action accepts a u32 group id and a variable-length cookie and uses
>>> the psample multicast group to make the packet available for
>>> observability.
>>>
>>> The maximum length of the user-defined cookie is set to 16, same as
>>> tc_cookie, to discourage using cookies that will not be offloadable.
>>>
>>> Signed-off-by: Adrian Moreno
>>> ---
>>> Documentation/netlink/specs/ovs_flow.yaml | 17 +
>>> include/uapi/linux/openvswitch.h | 28 ++
>>> net/openvswitch/Kconfig | 1 +
>>> net/openvswitch/actions.c | 45 +++
>>> net/openvswitch/flow_netlink.c| 33 -
>>> 5 files changed, 123 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/netlink/specs/ovs_flow.yaml
>>> b/Documentation/netlink/specs/ovs_flow.yaml
>>> index 4fdfc6b5cae9..a7ab5593a24f 100644
>>> --- a/Documentation/netlink/specs/ovs_flow.yaml
>>> +++ b/Documentation/netlink/specs/ovs_flow.yaml
>>> @@ -727,6 +727,12 @@ attribute-sets:
>>> name: dec-ttl
>>> type: nest
>>> nested-attributes: dec-ttl-attrs
>>> + -
>>> +name: emit-sample
>>> +type: nest
>>> +nested-attributes: emit-sample-attrs
>>> +doc: |
>>> + Sends a packet sample to psample for external observation.
>>>-
>>> name: tunnel-key-attrs
>>> enum-name: ovs-tunnel-key-attr
>>> @@ -938,6 +944,17 @@ attribute-sets:
>>>-
>>> name: gbp
>>> type: u32
>>> + -
>>> +name: emit-sample-attrs
>>> +enum-name: ovs-emit-sample-attr
>>> +name-prefix: ovs-emit-sample-attr-
>>> +attributes:
>>> + -
>>> +name: group
>>> +type: u32
>>> + -
>>> +name: cookie
>>> +type: binary
>>>
>>> operations:
>>>name-prefix: ovs-flow-cmd-
>>> diff --git a/include/uapi/linux/openvswitch.h
>>> b/include/uapi/linux/openvswitch.h
>>> index efc82c318fa2..8cfa1b3f6b06 100644
>>> --- a/include/uapi/linux/openvswitch.h
>>> +++ b/include/uapi/linux/openvswitch.h
>>> @@ -914,6 +914,31 @@ struct check_pkt_len_arg {
>>> };
>>> #endif
>>>
>>> +#define OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE 16
>>> +/**
>>> + * enum ovs_emit_sample_attr - Attributes for %OVS_ACTION_ATTR_EMIT_SAMPLE
>>> + * action.
>>> + *
>>> + * @OVS_EMIT_SAMPLE_ATTR_GROUP: 32-bit number to identify the source of the
>>> + * sample.
>>> + * @OVS_EMIT_SAMPLE_ATTR_COOKIE: A variable-length binary cookie that
>>> contains
>>> + * user-defined metadata. The maximum length is
>>> OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE
>>> + * bytes.
>>> + *
>>> + * Sends the packet to the psample multicast group with the specified
>>> group and
>>> + * cookie. It is possible to combine this action with the
>>> + * %OVS_ACTION_ATTR_TRUNC action to limit the size of the packet being
>>> emitted.
>>> + */
>>> +enum ovs_emit_sample_attr {
>>> + OVS_EMIT_SAMPLE_ATTR_GROUP = 1, /* u32 number. */
>>> + OVS_EMIT_SAMPLE_ATTR_COOKIE,/* Optional, user specified cookie. */
>>> +
>>> + /* private: */
>>> + __OVS_EMIT_SAMPLE_ATTR_MAX
>>> +};
>>> +
>>> +#define OVS_EMIT_SAMPLE_ATTR_MAX (__OVS_EMIT_SAMPLE_ATTR_MAX - 1)
>>> +
>>> /**
>>> * enum ovs_action_attr - Action types.
>>> *
>>> @@ -966,6 +991,8 @@ struct check_pkt_len_arg {
>>> * of l3 tunnel flag in the tun_flags field of OVS_ACTION_ATTR_ADD_MPLS
>>> * argument.
>>> * @OVS_ACTION_ATTR_DROP: Explicit drop action.
>>> + * @OVS_ACTION_ATTR_EMIT_SAMPLE: Send a sample of the packet to external
>>> + * observers via psample.
>>> *
>>> * Only a single header can be set with a single %OVS_ACTION_ATTR_SET.
>>> Not all
>>> * fields within a header are modifiable, e.g. the IPv4 protocol and
>>> fragment
>>> @@ -1004,6 +1031,7 @@ enum ovs_action_attr {
>>> OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */
>>> OVS_ACTION_ATTR_DEC_TTL, /* Nested OVS_DEC_TTL_ATTR_*. */
>>> OVS_ACTION_ATTR_DROP, /* u32 error code. */
>>> + OVS_ACTION_ATTR_EMIT_SAMPLE, /* Nested OVS_EMIT_SAMPLE_ATTR_*. */
>>>
>>> __OVS_ACTION_ATTR_MAX,/* Nothing past this will be accepted
>>>* from userspace. */
>>> diff --git a/net/openvswitch/Kconfig b/net/openvswitch/Kconfig
>>> index 29a7081858cd..2535f3f9f462 100644
>>> --- a/net/openvswitch/Kconfig
>>> +++ b/net/openvswitch/Kconfig
>>> @@ -10,6 +10,7 @@ config OPENVSWITCH
>>>(NF_CONNTRACK && ((!NF_DEFRAG_IPV6 || NF_DEFRAG_IPV6) && \
>>> (!NF_NAT || NF_NAT) && \
>>> (!NETFILTER_CONNCOUNT ||
>>> NETFILTER_CONNCOUNT)))
>>> + depends on PSAMPLE || !PSAMPLE
>>> select LIBCRC32C
>>> select MPLS
>>> select NET_MPLS_GSO
>>> diff --git a/net/openv
Re: [ovs-dev] [PATCH net-next v5 05/10] net: openvswitch: add emit_sample action
On 26 Jun 2024, at 22:34, Adrián Moreno wrote:
> On Wed, Jun 26, 2024 at 04:28:17PM GMT, Eelco Chaudron wrote:
>>
>>
>> On 25 Jun 2024, at 22:51, Adrian Moreno wrote:
>>
>>> Add support for a new action: emit_sample.
>>>
>>> This action accepts a u32 group id and a variable-length cookie and uses
>>> the psample multicast group to make the packet available for
>>> observability.
>>>
>>> The maximum length of the user-defined cookie is set to 16, same as
>>> tc_cookie, to discourage using cookies that will not be offloadable.
>>
>> I’ll add the same comment as I had in the user space part, and that
>> is that I feel from an OVS perspective this action should be called
>> emit_local() instead of emit_sample() to make it Datapath independent.
>> Or quoting the earlier comment:
>>
>>
>> “I’ll start the discussion again on the naming. The name "emit_sample()"
>> does not seem appropriate. This function's primary role is to copy the
>> packet and send it to a local collector, which varies depending on the
>> datapath. For the kernel datapath, this collector is psample, while for
>> userspace, it will likely be some kind of probe. This action is distinct
>> from the sample() action by design; it is a standalone action that can
>> be combined with others.
>>
>> Furthermore, the action itself does not involve taking a sample; it
>> consistently pushes the packet to the local collector. Therefore, I
>> suggest renaming "emit_sample()" to "emit_local()". This same goes for
>> all the derivative ATTR naming.”
>>
>
> This is a blurry semantic area.
> IMO, "sample" is the act of extracting (potentially a piece of)
> someting, in this case, a packet. It is common to only take some packets
> as samples, so this action usually comes with some kind of "rate", but
> even if the rate is 1, it's still sampling in this context.
>
> OTOH, OVS kernel design tries to be super-modular and define small
> combinable actions, so the rate or probability generation is done with
> another action which is (IMHO unfortunately) named "sample".
>
> With that interpretation of the term it would actually make more sense
> to rename "sample" to something like "random" (of course I'm not
> suggestion we do it). "sample" without any nested action that actually
> sends the packet somewhere is not sampling, it's just doing something or
> not based on a probability. Where as "emit_sample" is sampling even if
> it's not nested inside a "sample".
You're assuming we are extracting a packet for sampling, but this function
can be used for various other purposes. For instance, it could handle the
packet outside of the OVS pipeline through an eBPF program (so we are not
taking a sample, but continue packet processing outside of the OVS
pipeline). Calling it emit_sampling() in such cases could be very
confusing.
> Having said that, I don't have a super strong favor for "emit_sample". I'm
> OK with "emit_local" or "emit_packet" or even just "emit".
> I don't think any term will fully satisfy everyone so I hope we can find
> a reasonable compromise.
My preference would be emit_local() as we hand it off to some local
datapath entity.
>>> Signed-off-by: Adrian Moreno
>>> ---
>>> Documentation/netlink/specs/ovs_flow.yaml | 17 +
>>> include/uapi/linux/openvswitch.h | 28 ++
>>> net/openvswitch/Kconfig | 1 +
>>> net/openvswitch/actions.c | 45 +++
>>> net/openvswitch/flow_netlink.c| 33 -
>>> 5 files changed, 123 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/netlink/specs/ovs_flow.yaml
>>> b/Documentation/netlink/specs/ovs_flow.yaml
>>> index 4fdfc6b5cae9..a7ab5593a24f 100644
>>> --- a/Documentation/netlink/specs/ovs_flow.yaml
>>> +++ b/Documentation/netlink/specs/ovs_flow.yaml
>>> @@ -727,6 +727,12 @@ attribute-sets:
>>> name: dec-ttl
>>> type: nest
>>> nested-attributes: dec-ttl-attrs
>>> + -
>>> +name: emit-sample
>>> +type: nest
>>> +nested-attributes: emit-sample-attrs
>>> +doc: |
>>> + Sends a packet sample to psample for external observation.
>>>-
>>> name: tunnel-key-attrs
>>> enum-name: ovs-tunnel-key-attr
>>> @@ -938,6 +944,17 @@ attribute-sets:
>>>-
>>> name: gbp
>>> type: u32
>>> + -
>>> +name: emit-sample-attrs
>>> +enum-name: ovs-emit-sample-attr
>>> +name-prefix: ovs-emit-sample-attr-
>>> +attributes:
>>> + -
>>> +name: group
>>> +type: u32
>>> + -
>>> +name: cookie
>>> +type: binary
>>>
>>> operations:
>>>name-prefix: ovs-flow-cmd-
>>> diff --git a/include/uapi/linux/openvswitch.h
>>> b/include/uapi/linux/openvswitch.h
>>> index efc82c318fa2..8cfa1b3f6b06 100644
>>> --- a/include/uapi/linux/openvswitch.h
>>> +++ b/include/uapi/linux/openvswitch.h
>>> @@ -914,6 +914,31 @@ struct check_pkt_len_arg {
>>> };
>>> #endif
>>>
>>> +#define
Re: [ovs-dev] [PATCH net-next v5 05/10] net: openvswitch: add emit_sample action
On Wed, Jun 26, 2024 at 04:28:17PM GMT, Eelco Chaudron wrote:
>
>
> On 25 Jun 2024, at 22:51, Adrian Moreno wrote:
>
> > Add support for a new action: emit_sample.
> >
> > This action accepts a u32 group id and a variable-length cookie and uses
> > the psample multicast group to make the packet available for
> > observability.
> >
> > The maximum length of the user-defined cookie is set to 16, same as
> > tc_cookie, to discourage using cookies that will not be offloadable.
>
> I’ll add the same comment as I had in the user space part, and that
> is that I feel from an OVS perspective this action should be called
> emit_local() instead of emit_sample() to make it Datapath independent.
> Or quoting the earlier comment:
>
>
> “I’ll start the discussion again on the naming. The name "emit_sample()"
> does not seem appropriate. This function's primary role is to copy the
> packet and send it to a local collector, which varies depending on the
> datapath. For the kernel datapath, this collector is psample, while for
> userspace, it will likely be some kind of probe. This action is distinct
> from the sample() action by design; it is a standalone action that can
> be combined with others.
>
> Furthermore, the action itself does not involve taking a sample; it
> consistently pushes the packet to the local collector. Therefore, I
> suggest renaming "emit_sample()" to "emit_local()". This same goes for
> all the derivative ATTR naming.”
>
This is a blurry semantic area.
IMO, "sample" is the act of extracting (potentially a piece of)
someting, in this case, a packet. It is common to only take some packets
as samples, so this action usually comes with some kind of "rate", but
even if the rate is 1, it's still sampling in this context.
OTOH, OVS kernel design tries to be super-modular and define small
combinable actions, so the rate or probability generation is done with
another action which is (IMHO unfortunately) named "sample".
With that interpretation of the term it would actually make more sense
to rename "sample" to something like "random" (of course I'm not
suggestion we do it). "sample" without any nested action that actually
sends the packet somewhere is not sampling, it's just doing something or
not based on a probability. Where as "emit_sample" is sampling even if
it's not nested inside a "sample".
Having said that, I don't have a super strong favor for "emit_sample". I'm
OK with "emit_local" or "emit_packet" or even just "emit".
I don't think any term will fully satisfy everyone so I hope we can find
a reasonable compromise.
> > Signed-off-by: Adrian Moreno
> > ---
> > Documentation/netlink/specs/ovs_flow.yaml | 17 +
> > include/uapi/linux/openvswitch.h | 28 ++
> > net/openvswitch/Kconfig | 1 +
> > net/openvswitch/actions.c | 45 +++
> > net/openvswitch/flow_netlink.c| 33 -
> > 5 files changed, 123 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/netlink/specs/ovs_flow.yaml
> > b/Documentation/netlink/specs/ovs_flow.yaml
> > index 4fdfc6b5cae9..a7ab5593a24f 100644
> > --- a/Documentation/netlink/specs/ovs_flow.yaml
> > +++ b/Documentation/netlink/specs/ovs_flow.yaml
> > @@ -727,6 +727,12 @@ attribute-sets:
> > name: dec-ttl
> > type: nest
> > nested-attributes: dec-ttl-attrs
> > + -
> > +name: emit-sample
> > +type: nest
> > +nested-attributes: emit-sample-attrs
> > +doc: |
> > + Sends a packet sample to psample for external observation.
> >-
> > name: tunnel-key-attrs
> > enum-name: ovs-tunnel-key-attr
> > @@ -938,6 +944,17 @@ attribute-sets:
> >-
> > name: gbp
> > type: u32
> > + -
> > +name: emit-sample-attrs
> > +enum-name: ovs-emit-sample-attr
> > +name-prefix: ovs-emit-sample-attr-
> > +attributes:
> > + -
> > +name: group
> > +type: u32
> > + -
> > +name: cookie
> > +type: binary
> >
> > operations:
> >name-prefix: ovs-flow-cmd-
> > diff --git a/include/uapi/linux/openvswitch.h
> > b/include/uapi/linux/openvswitch.h
> > index efc82c318fa2..8cfa1b3f6b06 100644
> > --- a/include/uapi/linux/openvswitch.h
> > +++ b/include/uapi/linux/openvswitch.h
> > @@ -914,6 +914,31 @@ struct check_pkt_len_arg {
> > };
> > #endif
> >
> > +#define OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE 16
> > +/**
> > + * enum ovs_emit_sample_attr - Attributes for %OVS_ACTION_ATTR_EMIT_SAMPLE
> > + * action.
> > + *
> > + * @OVS_EMIT_SAMPLE_ATTR_GROUP: 32-bit number to identify the source of the
> > + * sample.
> > + * @OVS_EMIT_SAMPLE_ATTR_COOKIE: A variable-length binary cookie that
> > contains
> > + * user-defined metadata. The maximum length is
> > OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE
> > + * bytes.
> > + *
> > + * Sends the packet to the psample multicast group with the specified
> > group and
> > + * cookie. It is possible
Re: [ovs-dev] [PATCH net-next v5 05/10] net: openvswitch: add emit_sample action
On Wed, Jun 26, 2024 at 12:51:19PM GMT, Ilya Maximets wrote:
> On 6/25/24 22:51, Adrian Moreno wrote:
> > Add support for a new action: emit_sample.
> >
> > This action accepts a u32 group id and a variable-length cookie and uses
> > the psample multicast group to make the packet available for
> > observability.
> >
> > The maximum length of the user-defined cookie is set to 16, same as
> > tc_cookie, to discourage using cookies that will not be offloadable.
> >
> > Signed-off-by: Adrian Moreno
> > ---
> > Documentation/netlink/specs/ovs_flow.yaml | 17 +
> > include/uapi/linux/openvswitch.h | 28 ++
> > net/openvswitch/Kconfig | 1 +
> > net/openvswitch/actions.c | 45 +++
> > net/openvswitch/flow_netlink.c| 33 -
> > 5 files changed, 123 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/netlink/specs/ovs_flow.yaml
> > b/Documentation/netlink/specs/ovs_flow.yaml
> > index 4fdfc6b5cae9..a7ab5593a24f 100644
> > --- a/Documentation/netlink/specs/ovs_flow.yaml
> > +++ b/Documentation/netlink/specs/ovs_flow.yaml
> > @@ -727,6 +727,12 @@ attribute-sets:
> > name: dec-ttl
> > type: nest
> > nested-attributes: dec-ttl-attrs
> > + -
> > +name: emit-sample
> > +type: nest
> > +nested-attributes: emit-sample-attrs
> > +doc: |
> > + Sends a packet sample to psample for external observation.
> >-
> > name: tunnel-key-attrs
> > enum-name: ovs-tunnel-key-attr
> > @@ -938,6 +944,17 @@ attribute-sets:
> >-
> > name: gbp
> > type: u32
> > + -
> > +name: emit-sample-attrs
> > +enum-name: ovs-emit-sample-attr
> > +name-prefix: ovs-emit-sample-attr-
> > +attributes:
> > + -
> > +name: group
> > +type: u32
> > + -
> > +name: cookie
> > +type: binary
> >
> > operations:
> >name-prefix: ovs-flow-cmd-
> > diff --git a/include/uapi/linux/openvswitch.h
> > b/include/uapi/linux/openvswitch.h
> > index efc82c318fa2..8cfa1b3f6b06 100644
> > --- a/include/uapi/linux/openvswitch.h
> > +++ b/include/uapi/linux/openvswitch.h
> > @@ -914,6 +914,31 @@ struct check_pkt_len_arg {
> > };
> > #endif
> >
> > +#define OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE 16
> > +/**
> > + * enum ovs_emit_sample_attr - Attributes for %OVS_ACTION_ATTR_EMIT_SAMPLE
> > + * action.
> > + *
> > + * @OVS_EMIT_SAMPLE_ATTR_GROUP: 32-bit number to identify the source of the
> > + * sample.
> > + * @OVS_EMIT_SAMPLE_ATTR_COOKIE: A variable-length binary cookie that
> > contains
> > + * user-defined metadata. The maximum length is
> > OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE
> > + * bytes.
> > + *
> > + * Sends the packet to the psample multicast group with the specified
> > group and
> > + * cookie. It is possible to combine this action with the
> > + * %OVS_ACTION_ATTR_TRUNC action to limit the size of the packet being
> > emitted.
> > + */
> > +enum ovs_emit_sample_attr {
> > + OVS_EMIT_SAMPLE_ATTR_GROUP = 1, /* u32 number. */
> > + OVS_EMIT_SAMPLE_ATTR_COOKIE,/* Optional, user specified cookie. */
> > +
> > + /* private: */
> > + __OVS_EMIT_SAMPLE_ATTR_MAX
> > +};
> > +
> > +#define OVS_EMIT_SAMPLE_ATTR_MAX (__OVS_EMIT_SAMPLE_ATTR_MAX - 1)
> > +
> > /**
> > * enum ovs_action_attr - Action types.
> > *
> > @@ -966,6 +991,8 @@ struct check_pkt_len_arg {
> > * of l3 tunnel flag in the tun_flags field of OVS_ACTION_ATTR_ADD_MPLS
> > * argument.
> > * @OVS_ACTION_ATTR_DROP: Explicit drop action.
> > + * @OVS_ACTION_ATTR_EMIT_SAMPLE: Send a sample of the packet to external
> > + * observers via psample.
> > *
> > * Only a single header can be set with a single %OVS_ACTION_ATTR_SET.
> > Not all
> > * fields within a header are modifiable, e.g. the IPv4 protocol and
> > fragment
> > @@ -1004,6 +1031,7 @@ enum ovs_action_attr {
> > OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */
> > OVS_ACTION_ATTR_DEC_TTL, /* Nested OVS_DEC_TTL_ATTR_*. */
> > OVS_ACTION_ATTR_DROP, /* u32 error code. */
> > + OVS_ACTION_ATTR_EMIT_SAMPLE, /* Nested OVS_EMIT_SAMPLE_ATTR_*. */
> >
> > __OVS_ACTION_ATTR_MAX,/* Nothing past this will be accepted
> >* from userspace. */
> > diff --git a/net/openvswitch/Kconfig b/net/openvswitch/Kconfig
> > index 29a7081858cd..2535f3f9f462 100644
> > --- a/net/openvswitch/Kconfig
> > +++ b/net/openvswitch/Kconfig
> > @@ -10,6 +10,7 @@ config OPENVSWITCH
> >(NF_CONNTRACK && ((!NF_DEFRAG_IPV6 || NF_DEFRAG_IPV6) && \
> > (!NF_NAT || NF_NAT) && \
> > (!NETFILTER_CONNCOUNT ||
> > NETFILTER_CONNCOUNT)))
> > + depends on PSAMPLE || !PSAMPLE
> > select LIBCRC32C
> > select MPLS
> > select NET_MPLS_GSO
> > diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions
Re: [ovs-dev] [PATCH net-next v5 05/10] net: openvswitch: add emit_sample action
On 25 Jun 2024, at 22:51, Adrian Moreno wrote:
> Add support for a new action: emit_sample.
>
> This action accepts a u32 group id and a variable-length cookie and uses
> the psample multicast group to make the packet available for
> observability.
>
> The maximum length of the user-defined cookie is set to 16, same as
> tc_cookie, to discourage using cookies that will not be offloadable.
I’ll add the same comment as I had in the user space part, and that
is that I feel from an OVS perspective this action should be called
emit_local() instead of emit_sample() to make it Datapath independent.
Or quoting the earlier comment:
“I’ll start the discussion again on the naming. The name "emit_sample()"
does not seem appropriate. This function's primary role is to copy the
packet and send it to a local collector, which varies depending on the
datapath. For the kernel datapath, this collector is psample, while for
userspace, it will likely be some kind of probe. This action is distinct
from the sample() action by design; it is a standalone action that can
be combined with others.
Furthermore, the action itself does not involve taking a sample; it
consistently pushes the packet to the local collector. Therefore, I
suggest renaming "emit_sample()" to "emit_local()". This same goes for
all the derivative ATTR naming.”
> Signed-off-by: Adrian Moreno
> ---
> Documentation/netlink/specs/ovs_flow.yaml | 17 +
> include/uapi/linux/openvswitch.h | 28 ++
> net/openvswitch/Kconfig | 1 +
> net/openvswitch/actions.c | 45 +++
> net/openvswitch/flow_netlink.c| 33 -
> 5 files changed, 123 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/netlink/specs/ovs_flow.yaml
> b/Documentation/netlink/specs/ovs_flow.yaml
> index 4fdfc6b5cae9..a7ab5593a24f 100644
> --- a/Documentation/netlink/specs/ovs_flow.yaml
> +++ b/Documentation/netlink/specs/ovs_flow.yaml
> @@ -727,6 +727,12 @@ attribute-sets:
> name: dec-ttl
> type: nest
> nested-attributes: dec-ttl-attrs
> + -
> +name: emit-sample
> +type: nest
> +nested-attributes: emit-sample-attrs
> +doc: |
> + Sends a packet sample to psample for external observation.
>-
> name: tunnel-key-attrs
> enum-name: ovs-tunnel-key-attr
> @@ -938,6 +944,17 @@ attribute-sets:
>-
> name: gbp
> type: u32
> + -
> +name: emit-sample-attrs
> +enum-name: ovs-emit-sample-attr
> +name-prefix: ovs-emit-sample-attr-
> +attributes:
> + -
> +name: group
> +type: u32
> + -
> +name: cookie
> +type: binary
>
> operations:
>name-prefix: ovs-flow-cmd-
> diff --git a/include/uapi/linux/openvswitch.h
> b/include/uapi/linux/openvswitch.h
> index efc82c318fa2..8cfa1b3f6b06 100644
> --- a/include/uapi/linux/openvswitch.h
> +++ b/include/uapi/linux/openvswitch.h
> @@ -914,6 +914,31 @@ struct check_pkt_len_arg {
> };
> #endif
>
> +#define OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE 16
> +/**
> + * enum ovs_emit_sample_attr - Attributes for %OVS_ACTION_ATTR_EMIT_SAMPLE
> + * action.
> + *
> + * @OVS_EMIT_SAMPLE_ATTR_GROUP: 32-bit number to identify the source of the
> + * sample.
> + * @OVS_EMIT_SAMPLE_ATTR_COOKIE: A variable-length binary cookie that
> contains
> + * user-defined metadata. The maximum length is
> OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE
> + * bytes.
> + *
> + * Sends the packet to the psample multicast group with the specified group
> and
> + * cookie. It is possible to combine this action with the
> + * %OVS_ACTION_ATTR_TRUNC action to limit the size of the packet being
> emitted.
Although this include file is kernel-related, it will probably be re-used for
other datapaths, so should we be more general here?
> + */
> +enum ovs_emit_sample_attr {
> + OVS_EMIT_SAMPLE_ATTR_GROUP = 1, /* u32 number. */
> + OVS_EMIT_SAMPLE_ATTR_COOKIE,/* Optional, user specified cookie. */
As we start a new set of attributes maybe it would be good starting it off in
alphabetical order?
> +
> + /* private: */
> + __OVS_EMIT_SAMPLE_ATTR_MAX
> +};
> +
> +#define OVS_EMIT_SAMPLE_ATTR_MAX (__OVS_EMIT_SAMPLE_ATTR_MAX - 1)
> +
> /**
> * enum ovs_action_attr - Action types.
> *
> @@ -966,6 +991,8 @@ struct check_pkt_len_arg {
> * of l3 tunnel flag in the tun_flags field of OVS_ACTION_ATTR_ADD_MPLS
> * argument.
> * @OVS_ACTION_ATTR_DROP: Explicit drop action.
> + * @OVS_ACTION_ATTR_EMIT_SAMPLE: Send a sample of the packet to external
> + * observers via psample.
> *
> * Only a single header can be set with a single %OVS_ACTION_ATTR_SET. Not
> all
> * fields within a header are modifiable, e.g. the IPv4 protocol and fragment
> @@ -1004,6 +1031,7 @@ enum ovs_action_attr {
> OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */
> OVS_ACTION_ATTR_DEC_TTL, /* Nested OVS_DEC_TTL_ATTR_*.
Re: [ovs-dev] [PATCH net-next v5 05/10] net: openvswitch: add emit_sample action
On 6/25/24 22:51, Adrian Moreno wrote:
> Add support for a new action: emit_sample.
>
> This action accepts a u32 group id and a variable-length cookie and uses
> the psample multicast group to make the packet available for
> observability.
>
> The maximum length of the user-defined cookie is set to 16, same as
> tc_cookie, to discourage using cookies that will not be offloadable.
>
> Signed-off-by: Adrian Moreno
> ---
> Documentation/netlink/specs/ovs_flow.yaml | 17 +
> include/uapi/linux/openvswitch.h | 28 ++
> net/openvswitch/Kconfig | 1 +
> net/openvswitch/actions.c | 45 +++
> net/openvswitch/flow_netlink.c| 33 -
> 5 files changed, 123 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/netlink/specs/ovs_flow.yaml
> b/Documentation/netlink/specs/ovs_flow.yaml
> index 4fdfc6b5cae9..a7ab5593a24f 100644
> --- a/Documentation/netlink/specs/ovs_flow.yaml
> +++ b/Documentation/netlink/specs/ovs_flow.yaml
> @@ -727,6 +727,12 @@ attribute-sets:
> name: dec-ttl
> type: nest
> nested-attributes: dec-ttl-attrs
> + -
> +name: emit-sample
> +type: nest
> +nested-attributes: emit-sample-attrs
> +doc: |
> + Sends a packet sample to psample for external observation.
>-
> name: tunnel-key-attrs
> enum-name: ovs-tunnel-key-attr
> @@ -938,6 +944,17 @@ attribute-sets:
>-
> name: gbp
> type: u32
> + -
> +name: emit-sample-attrs
> +enum-name: ovs-emit-sample-attr
> +name-prefix: ovs-emit-sample-attr-
> +attributes:
> + -
> +name: group
> +type: u32
> + -
> +name: cookie
> +type: binary
>
> operations:
>name-prefix: ovs-flow-cmd-
> diff --git a/include/uapi/linux/openvswitch.h
> b/include/uapi/linux/openvswitch.h
> index efc82c318fa2..8cfa1b3f6b06 100644
> --- a/include/uapi/linux/openvswitch.h
> +++ b/include/uapi/linux/openvswitch.h
> @@ -914,6 +914,31 @@ struct check_pkt_len_arg {
> };
> #endif
>
> +#define OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE 16
> +/**
> + * enum ovs_emit_sample_attr - Attributes for %OVS_ACTION_ATTR_EMIT_SAMPLE
> + * action.
> + *
> + * @OVS_EMIT_SAMPLE_ATTR_GROUP: 32-bit number to identify the source of the
> + * sample.
> + * @OVS_EMIT_SAMPLE_ATTR_COOKIE: A variable-length binary cookie that
> contains
> + * user-defined metadata. The maximum length is
> OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE
> + * bytes.
> + *
> + * Sends the packet to the psample multicast group with the specified group
> and
> + * cookie. It is possible to combine this action with the
> + * %OVS_ACTION_ATTR_TRUNC action to limit the size of the packet being
> emitted.
> + */
> +enum ovs_emit_sample_attr {
> + OVS_EMIT_SAMPLE_ATTR_GROUP = 1, /* u32 number. */
> + OVS_EMIT_SAMPLE_ATTR_COOKIE,/* Optional, user specified cookie. */
> +
> + /* private: */
> + __OVS_EMIT_SAMPLE_ATTR_MAX
> +};
> +
> +#define OVS_EMIT_SAMPLE_ATTR_MAX (__OVS_EMIT_SAMPLE_ATTR_MAX - 1)
> +
> /**
> * enum ovs_action_attr - Action types.
> *
> @@ -966,6 +991,8 @@ struct check_pkt_len_arg {
> * of l3 tunnel flag in the tun_flags field of OVS_ACTION_ATTR_ADD_MPLS
> * argument.
> * @OVS_ACTION_ATTR_DROP: Explicit drop action.
> + * @OVS_ACTION_ATTR_EMIT_SAMPLE: Send a sample of the packet to external
> + * observers via psample.
> *
> * Only a single header can be set with a single %OVS_ACTION_ATTR_SET. Not
> all
> * fields within a header are modifiable, e.g. the IPv4 protocol and fragment
> @@ -1004,6 +1031,7 @@ enum ovs_action_attr {
> OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */
> OVS_ACTION_ATTR_DEC_TTL, /* Nested OVS_DEC_TTL_ATTR_*. */
> OVS_ACTION_ATTR_DROP, /* u32 error code. */
> + OVS_ACTION_ATTR_EMIT_SAMPLE, /* Nested OVS_EMIT_SAMPLE_ATTR_*. */
>
> __OVS_ACTION_ATTR_MAX,/* Nothing past this will be accepted
> * from userspace. */
> diff --git a/net/openvswitch/Kconfig b/net/openvswitch/Kconfig
> index 29a7081858cd..2535f3f9f462 100644
> --- a/net/openvswitch/Kconfig
> +++ b/net/openvswitch/Kconfig
> @@ -10,6 +10,7 @@ config OPENVSWITCH
> (NF_CONNTRACK && ((!NF_DEFRAG_IPV6 || NF_DEFRAG_IPV6) && \
>(!NF_NAT || NF_NAT) && \
>(!NETFILTER_CONNCOUNT ||
> NETFILTER_CONNCOUNT)))
> + depends on PSAMPLE || !PSAMPLE
> select LIBCRC32C
> select MPLS
> select NET_MPLS_GSO
> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index 964225580824..1f555cbba312 100644
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
> @@ -24,6 +24,11 @@
> #include
> #include
> #include
> +
> +#if IS_ENABLED(CONFIG_PSAMPLE)
> +#include
> +#endif
> +
> #include
>
> #include "datapath.h
[ovs-dev] [PATCH net-next v5 05/10] net: openvswitch: add emit_sample action
Add support for a new action: emit_sample.
This action accepts a u32 group id and a variable-length cookie and uses
the psample multicast group to make the packet available for
observability.
The maximum length of the user-defined cookie is set to 16, same as
tc_cookie, to discourage using cookies that will not be offloadable.
Signed-off-by: Adrian Moreno
---
Documentation/netlink/specs/ovs_flow.yaml | 17 +
include/uapi/linux/openvswitch.h | 28 ++
net/openvswitch/Kconfig | 1 +
net/openvswitch/actions.c | 45 +++
net/openvswitch/flow_netlink.c| 33 -
5 files changed, 123 insertions(+), 1 deletion(-)
diff --git a/Documentation/netlink/specs/ovs_flow.yaml
b/Documentation/netlink/specs/ovs_flow.yaml
index 4fdfc6b5cae9..a7ab5593a24f 100644
--- a/Documentation/netlink/specs/ovs_flow.yaml
+++ b/Documentation/netlink/specs/ovs_flow.yaml
@@ -727,6 +727,12 @@ attribute-sets:
name: dec-ttl
type: nest
nested-attributes: dec-ttl-attrs
+ -
+name: emit-sample
+type: nest
+nested-attributes: emit-sample-attrs
+doc: |
+ Sends a packet sample to psample for external observation.
-
name: tunnel-key-attrs
enum-name: ovs-tunnel-key-attr
@@ -938,6 +944,17 @@ attribute-sets:
-
name: gbp
type: u32
+ -
+name: emit-sample-attrs
+enum-name: ovs-emit-sample-attr
+name-prefix: ovs-emit-sample-attr-
+attributes:
+ -
+name: group
+type: u32
+ -
+name: cookie
+type: binary
operations:
name-prefix: ovs-flow-cmd-
diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
index efc82c318fa2..8cfa1b3f6b06 100644
--- a/include/uapi/linux/openvswitch.h
+++ b/include/uapi/linux/openvswitch.h
@@ -914,6 +914,31 @@ struct check_pkt_len_arg {
};
#endif
+#define OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE 16
+/**
+ * enum ovs_emit_sample_attr - Attributes for %OVS_ACTION_ATTR_EMIT_SAMPLE
+ * action.
+ *
+ * @OVS_EMIT_SAMPLE_ATTR_GROUP: 32-bit number to identify the source of the
+ * sample.
+ * @OVS_EMIT_SAMPLE_ATTR_COOKIE: A variable-length binary cookie that contains
+ * user-defined metadata. The maximum length is OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE
+ * bytes.
+ *
+ * Sends the packet to the psample multicast group with the specified group and
+ * cookie. It is possible to combine this action with the
+ * %OVS_ACTION_ATTR_TRUNC action to limit the size of the packet being emitted.
+ */
+enum ovs_emit_sample_attr {
+ OVS_EMIT_SAMPLE_ATTR_GROUP = 1, /* u32 number. */
+ OVS_EMIT_SAMPLE_ATTR_COOKIE,/* Optional, user specified cookie. */
+
+ /* private: */
+ __OVS_EMIT_SAMPLE_ATTR_MAX
+};
+
+#define OVS_EMIT_SAMPLE_ATTR_MAX (__OVS_EMIT_SAMPLE_ATTR_MAX - 1)
+
/**
* enum ovs_action_attr - Action types.
*
@@ -966,6 +991,8 @@ struct check_pkt_len_arg {
* of l3 tunnel flag in the tun_flags field of OVS_ACTION_ATTR_ADD_MPLS
* argument.
* @OVS_ACTION_ATTR_DROP: Explicit drop action.
+ * @OVS_ACTION_ATTR_EMIT_SAMPLE: Send a sample of the packet to external
+ * observers via psample.
*
* Only a single header can be set with a single %OVS_ACTION_ATTR_SET. Not all
* fields within a header are modifiable, e.g. the IPv4 protocol and fragment
@@ -1004,6 +1031,7 @@ enum ovs_action_attr {
OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */
OVS_ACTION_ATTR_DEC_TTL, /* Nested OVS_DEC_TTL_ATTR_*. */
OVS_ACTION_ATTR_DROP, /* u32 error code. */
+ OVS_ACTION_ATTR_EMIT_SAMPLE, /* Nested OVS_EMIT_SAMPLE_ATTR_*. */
__OVS_ACTION_ATTR_MAX,/* Nothing past this will be accepted
* from userspace. */
diff --git a/net/openvswitch/Kconfig b/net/openvswitch/Kconfig
index 29a7081858cd..2535f3f9f462 100644
--- a/net/openvswitch/Kconfig
+++ b/net/openvswitch/Kconfig
@@ -10,6 +10,7 @@ config OPENVSWITCH
(NF_CONNTRACK && ((!NF_DEFRAG_IPV6 || NF_DEFRAG_IPV6) && \
(!NF_NAT || NF_NAT) && \
(!NETFILTER_CONNCOUNT ||
NETFILTER_CONNCOUNT)))
+ depends on PSAMPLE || !PSAMPLE
select LIBCRC32C
select MPLS
select NET_MPLS_GSO
diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index 964225580824..1f555cbba312 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -24,6 +24,11 @@
#include
#include
#include
+
+#if IS_ENABLED(CONFIG_PSAMPLE)
+#include
+#endif
+
#include
#include "datapath.h"
@@ -1299,6 +1304,37 @@ static int execute_dec_ttl(struct sk_buff *skb, struct
sw_flow_key *key)
return 0;
}
+static void execute_emit_sample(struct datapath *dp, struct sk_buff *skb,
+ const struct sw_flow_key *key,
+ con
